Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESM support #784

Merged
merged 3 commits into from
Mar 14, 2023
Merged

ESM support #784

merged 3 commits into from
Mar 14, 2023

Conversation

btakita
Copy link
Contributor

@btakita btakita commented Feb 3, 2023

fixes #783

Changes

Please describe both what is changing and why this is important. Include:

  • Endpoints added, deleted, deprecated, or changed
  • Classes and methods added, deleted, deprecated, or changed
  • Screenshots of new or changed UI, if applicable
  • A summary of usage if this is a new feature or change to a public API (this should also be added to relevant documentation once released)
  • Any alternative designs or approaches considered

References

Please include relevant links supporting this change such as a:

  • support ticket
  • community post
  • StackOverflow post
  • support forum thread

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

  • This change adds unit test coverage
  • This change adds integration test coverage

Checklist

@btakita btakita requested a review from a team as a code owner February 3, 2023 03:55
@@ -0,0 +1,11 @@
import { createRequire } from 'module';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that is makes much difference, but could you change this to:

import mod from './index.js';

export const { ManagementClient, AuthenticationClient } = mod;
export default { ManagementClient, AuthenticationClient }

(and check it still fixes your issue)

This works for Node 19 and looks a bit simpler - also i don't think we need the doc (I should probably remove this from the other index too)

@@ -3,6 +3,7 @@
"version": "3.1.2",
"description": "SDK for Auth0 API v2",
"main": "src/index.js",
"module": "src/index.mjs",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use conditional exports for this, I believe module is the "old" way of specifying esm exports for bundlers, and the standard node way is exports

Suggested change
"module": "src/index.mjs",
"exports": {
"import": "./src/index.mjs",
"require": "./src/index.js"
},

(and also check that it resolves your issue - thanks!)

Copy link
Contributor Author

@btakita btakita Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it makes sense to use exports...however it will require a major version update for this library if you are following semver rules. I went with the more conservative route of adding "module" but if you are willing to push a major version update, then we can switch it to exports.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @btakita - possibly a stupid question - why do you think it would be a breaking change?

Copy link
Contributor Author

@btakita btakita Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it has to do with preventing any other entry points besides those defined in "exports". found in https://nodejs.org/api/packages.html#package-entry-points

I suppose that if all of the modules in the package were defined within exports, then it may not be a breaking change. I'm not sure though. I'll look more into an authoritative source on whether it would still be a breaking change if all modules in the package were defined in exports.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I didn't know that - let's stick with "module" then

Copy link
Contributor

@adamjmcgrath adamjmcgrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @btakita

@adamjmcgrath adamjmcgrath merged commit a143a36 into auth0:master Mar 14, 2023
@adamjmcgrath adamjmcgrath mentioned this pull request Mar 14, 2023
@langell
Copy link

langell commented Apr 5, 2023

I'm still having issues with the auth0 package when I deploy to an AWS lambda using serverless framework and webpack.
I'm using ESM syntax to pull in the auth0 package

import { AuthenticationClient } from 'auth0';

This is the error

{ "errorType": "Runtime.ImportModuleError", "errorMessage": "Error: Cannot find module './management'\nRequire stack:\n- /builds/*************/auth-services/user-registration-service/node_modules/auth0/src/index.mjs", "stack": [ "Runtime.ImportModuleError: Error: Cannot find module './management'", "Require stack:", "- *************/auth-services/user-registration-service/node_modules/auth0/src/index.mjs", " at _loadUserApp (file:///var/runtime/index.mjs:996:17)", " at Object.UserFunction.js.module.exports.load (file:///var/runtime/index.mjs:1031:21)", " at start (file:///var/runtime/index.mjs:1194:23)", " at file:///var/runtime/index.mjs:1200:1" ] }

@adamjmcgrath
Copy link
Contributor

Hi @langell - we didn't add package exports because this would have been a breaking change.

You probably need to do something like import * as auth0 from 'auth0'; to make it work in environments that require package exports. (we intend to resolve this in the next major)

@langell
Copy link

langell commented Apr 5, 2023

ok I'll give that a try, thanks for the speedy response :-)

@langell
Copy link

langell commented Apr 6, 2023

@adamjmcgrath, do you have a roadmap date for when you will release the package exports?

@adamjmcgrath
Copy link
Contributor

@langell - I don't have a timeline I'm afraid

Although looking at your stacktrace again, I don't think this is the issue as index.mjs is being discovered, the require('./management') bit is failing. If you can reproduce this locally, feel free to share details and I can take a look.

@langell
Copy link

langell commented Apr 6, 2023

@adamjmcgrath, I'll get you the details later today. I will be able to duplicate this locally I think
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESM support
3 participants