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 module support #884

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

esm module support #884

wants to merge 1 commit into from

Conversation

btakita
Copy link

@btakita btakita commented Feb 3, 2023

fixes: #655

By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Description

Describe the purpose of this PR along with any background information and the impacts of the proposed change. For the benefit of the community, please do not assume prior context.

Provide details that support your chosen implementation, including: breaking changes, alternatives considered, changes to the API, etc.

If the UI is being changed, please provide screenshots.

References

Include any links supporting this change such as a:

  • GitHub Issue/PR number addressed or fixed
  • Auth0 Community post
  • StackOverflow post
  • Support forum thread
  • Related pull requests/issues from other repos

If there are no references, simply delete this section.

Testing

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.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not the default branch

@trim21
Copy link

trim21 commented May 9, 2023

any progress on this PR?

@btakita
Copy link
Author

btakita commented May 10, 2023

This PR will cause issues with Rollup builds as Rollup does not support createRequire in node.js.

rollup/rollup#4597
rollup/rollup#4274

@Trim if you are using ESM in the runtime or esbuild/vite, the best option for now is to use createRequire to load this library.

See auth0/node-auth0#832
auth0/node-auth0#828

For this library to support valid ESM, a different approach is probably needed, as Rollup does not appear it will support node.js createRequire in the near future, possibly ever. A rewrite of this library in ESM with a CJS shim (or dropping CJS) or using deasync, are options that I have identified. There may be other options that I'm not aware of.

@trim21
Copy link

trim21 commented May 10, 2023

This PR will cause issues with Rollup builds as Rollup does not support createRequire in node.js.

rollup/rollup#4597 rollup/rollup#4274

This can be fixed by conditional exports

https://nodejs.org/api/packages.html#conditional-exports

nodejs import conditions are ["import", "node"], and by default rollup use ['default', 'module', 'import']

So just set this shim as package.json#exports['.'].import.node instead of package.json#module , node and rollup will both be happy.

https://github.com/rollup/plugins/blob/master/packages/node-resolve/README.md#exportconditions

Of course rewrite in esm is a better option, but I think this shim still has its value and could work.

@@ -3,6 +3,7 @@
"version": "9.0.0",
"description": "JSON Web Token implementation (symmetric and asymmetric)",
"main": "index.js",
"module": "index.mjs",
Copy link

@trim21 trim21 May 10, 2023

Choose a reason for hiding this comment

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

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

Copy link

@trim21 trim21 May 10, 2023

Choose a reason for hiding this comment

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

I tested this locally with node and bundler rollup/webpack/esbuild, all works fine.

// index.mjs
import * as jwt from 'jsonwebtoken'

console.log(jwt)

rollup

rollup use condition ['default', 'module', 'import'], so it will resolve to ./index.js.

Previous rollup config with node-resolve and cjs plugins will still work. rollup without these plugins was broken and still be broken.

// rollup.config.mjs
import commonjs from '@rollup/plugin-commonjs';
import {nodeResolve} from '@rollup/plugin-node-resolve';

export default {
  plugins: [nodeResolve(), commonjs()],
  input: './index.mjs',
  output: {
    file: './build/bundle.min.js',
    format: 'iife',
    name: 'bundle'
  }
}

webpack support this out-of-box

I'm not sure what's its condition, but at least it include module and resolve to index.js

esbuild

a little bit complex here.

  1. if esbuild is called with --platform=node, it will use condition ['default', 'import', 'node', 'module'] or ['default', 'require', 'node', 'module'] (depend on source code), so the only important thing is to put "module": "./index.js", in the top of exports object, otherwise esbuild will resolve to condition import.node (index.mjs).

(yes, exports as json field are ordered, bundlers or runtime will try to resolve from top to end)

  1. if --platform=node is not given, condition won't include node, and it can only resolve to module or require, that's index.js.

vite

vite doesn't use node condition.

Summary

don't add package.json#module, use exports with conditions.

So no bundler's behavior will change, what's broken is still broken, and now nodejs can have a better esm support from this package

@trim21
Copy link

trim21 commented May 10, 2023

I'm encountering another problem.

I'm using esbuild as esm loader in nodejs runtime with typescript, but cjs only exports make it impossible to write a valid type definition at @types/jsonwebtoken

For a cjs only package, the correct way to type it is export = .... but jsonwebtoken is widely used, so @types/jsonwebtoken also need to export many interface as options, like export interface {}.

But there is no way to use export assignment with esm exports in typescript.

So have a (at least nodejs) esm would help this problem.

DefinitelyTyped/DefinitelyTyped#65458

@trim21
Copy link

trim21 commented May 10, 2023

@btakita I hope my comments explained the suggestion clearly.

@btakita
Copy link
Author

btakita commented Jul 30, 2023

@btakita I hope my comments explained the suggestion clearly.

@trim21 I lost track of this tread. I'll take at look at your suggestions.

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.

Please add ES6 module/ESM support
5 participants