-
Notifications
You must be signed in to change notification settings - Fork 339
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
Add support for node hybrid cjs and esm modules #130
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frederikprijck Could you check now if things work better?
This is somewhat tricky to setup 😅
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇♂️ |
I'm still interested in this PR just in case 😄 |
Another idea, now that ESM is widely supported in all versions of Node.js. Why not drop CommonJS compatibility and remove the build step entirely? |
I'm all for it, if that's the direction @frederikprijck agrees to take this module in I'll update this PR |
Fixes error "'resolution-mode' assertions are only supported when `moduleResolution` is `node16` or `nodenext`" with `got`. Details: <sindresorhus/got#2051> Also enable library checks for TypeScript validation now that typings are improved. Native ESM support details: <https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#esm-nodejs> Requires using `.js` extensions for relative imports. `got` is also now ESM-only -> fix import path. Something's broken with `jwt-decode`'s exports, so using `.default` is required (ref: <auth0/jwt-decode#103> and <auth0/jwt-decode#130>).
package.json
Outdated
"module": "build/jwt-decode.esm.js", | ||
"types": "index.d.ts", | ||
"module": "build/jwt-decode.mjs", | ||
"types": "index.d.mts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field should match the main
field.
"types": "index.d.mts", | |
"types": "index.d.ts", |
package.json
Outdated
"types": { | ||
"require": "./index.d.ts", | ||
"import": "./index.d.mts", | ||
"default": "./index.d.mts" | ||
}, | ||
"require": "./build/jwt-decode.cjs.js", | ||
"import": "./build/jwt-decode.mjs", | ||
"default": "./build/jwt-decode.mjs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default export should match the type
field in package.json
. The absence of type
, means it’s commonjs
.
Also the value that matches the default can be omitted.
"types": { | |
"require": "./index.d.ts", | |
"import": "./index.d.mts", | |
"default": "./index.d.mts" | |
}, | |
"require": "./build/jwt-decode.cjs.js", | |
"import": "./build/jwt-decode.mjs", | |
"default": "./build/jwt-decode.mjs" | |
"types": { | |
"import": "./index.d.mts", | |
"default": "./index.d.ts" | |
}, | |
"import": "./build/jwt-decode.mjs", | |
"default": "./build/jwt-decode.cjs.js" |
I was going to make a PR to fix the type definitions. Then I found this PR goes a step further by adding native dual publishing. Thank you for this great work @perrin4869! Some additional context: TypeScript 4.7 added the CJS (JavaScript) const jwtDecode = require('jwt-decode');
jwtDecode.default(token); CJS (TypeScript) import jwtDecode = require('jwt-decode');
jwtDecode.default(token); ESM (same syntax for JavaScript and TypeScript) import jwtDecode from 'jwt-decode';
jwtDecode.default(token); Without my suggestions, these issues still exist for TypeScript’s |
@remcohaszing Thanks for the review!! That was really useful! Updated according to your suggestions 😃 |
Wouldn't we need the |
Would it not be better to have the default as |
Any updates or thoughts on this PR @frederikprijck? Also as stated by @jonkoops 👇🏽
Why still support for AMD?AMD is more or less dead, so the thought of having the standalone which takes into account AMD and fallbacks to windows makes no sense, most people can polyfill this themselves a section in the docs would suffice here imo. Another ApproachAnother approach here could have been:
|
There are no plans to do this. Other libraries have done this before, and that hasn;t been appreciated, see node-fetch/node-fetch#1263 as an example. Additionally, this is not a node library, but a browser library. So the support of node for ESM isn't what should drive our decision. This library works on node, but wasn't created with node in mind (as mentioned in the readme). As I know this library is used in node, we have no plans to drop support for that (and I am happy to improve the support by using node's atob instead of our own polyfill now that node 14 is no longer supported). Just know that it's not the main reason of its existence. That's ofcourse not to say that browsers can't support ESM. Regardless, we have improved all of this in #151 . Feel free to provide feedback on that PR. Given the potential breaking change of this PR, we didn't want to ship the PR as is but include it in our beta and implement it slightly differently (but pretty comparable). Regardless, appreciate the PR and work here @perrin4869.
I mean, I considered it. But for a library that's downloaded 5 millions times a week, I see no reason to drop support for something that isn't causing issues. I mean, the download numbers shouldnt drive our decisions, but it's hard to know who still uses AMD or who doesn't and we see not strong need to drop AMD. However, we did simplified the standalone file and rely on rollup to handle the UMD bundle with AMD. Closing this PR as we should have covered that in that branch. |
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
Adds support for loading this module natively as
esm
from node.js.This also makes it possible to consume this module with the new
nodenext
andnode16
typescript module resolvers.This is not possible in the current state of affairs, because the
.d.ts
file is written foresm
modules while typescript will attempt to load the commonjs version of the package.References
https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/
https://nodejs.org/api/packages.html#exports