-
-
Notifications
You must be signed in to change notification settings - Fork 903
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
feat: native Node.js ES Modules (wrapper approach) #423
Conversation
@TrySound maybe you could also have a look at this one? |
e522289
to
d06b79c
Compare
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.
Look good to me. Though eventually bundlers will support "exports" field and commonjs can become a problem again. But let's keep this change as is and see if solution will be found later.
Will this be released as a new major version? |
@LinusU so far I was considering only a minor version bump (-> Do you think a major version bump would be justified? |
I guess it will be a breaking change for native esm users. Currently only this way of import is possible
|
Thanks for explicitly noting this again! I agree 100%! I'm already watching the corresponding issues btw: |
Hmm, you are right:
Hmm, I now see two interpretations:
Since using this module as a native ESM with node was officially not supported so far I'm currently leaning towards 1. but I would be glad to hear your opinion on that one @TrySound @broofa @LinusU . |
Since esm is still considered experimental even in node v14 with removed warning I think minor makes sense. We just need to be ready to respond upcoming issues. |
b0e7c97
to
e822c36
Compare
The reason I asked is because another library, Here is the full post mortem from the author: https://medium.com/javascript-in-plain-english/is-promise-post-mortem-cab807f18dcc I personally think that ESM should be considered stable enough that it's not okay to break imports in a minor version, even if technically ESM is still considered "experimental". Both Node.js 13 & 14 are fully supported Node versions, and a lot of people are using them. Just the fact that I'm quite sure that My personal proposal is to remove the deep require files as well, and then release this as a new major release. |
e822c36
to
a39089a
Compare
Semver should only apply to changes in the documented, "official" API. (Saying it has to apply to private/undocumented APIs makes semver meaningless. For much the same reason it's impossible to prove a negative, it's impossible to know what undocumented aspects of an API are being used. Every change ends up having to be treated as a breaking change.) Since we've only ever documented ESM usage as
Not part of public API. Not a concern. |
I do agree with this, although I think that since doing Also, I guess I'm a bit extra careful since this is a widely used module (~30M downloads/week). Anyhow, just my two cents |
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.
LGTM
Thank you for all the valuable feedback on minor vs. major version bump. I think we could argue into both directions. Since I'm not 100% decided I'll cowardly remove support for deep imports before the next release. That way we definitely have a breaking change that requires a new major version plus we finally get to a state where we have a consistent module api across all supported platforms/module loaders/bundlers and no more legacy ways of including the module! |
BREAKING CHANGE: Native ES Modules is still an experimental API in Node.js 14.0.0 and has so far not officially been supported by the `uuid` module. Since Node.js allows importing CommonJS modules it was possible to import the `uuid` module like this: ```js import uuid from 'uuid'; console.log(uuid.v4()); // -> 'cd6c3b08-0adc-4f4b-a6ef-36087a1c9869' ``` This will no longer work with proper ES Module exports in place. You can now import the `uuid` library as described in the documentation: ```js import { v4 as uuidv4 } from 'uuid'; uuidv4(); // ⇨ '9b1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d' ``` or ```js import * as uuid from 'uuid'; console.log(uuid.v4()); // -> 'cd6c3b08-0adc-4f4b-a6ef-36087a1c9869' ``` Enabling native ES Modules for Node.js requires some special care for the v1 algorithm which needs internal state. This makes this library susceptible to the dual package hazard described in https://nodejs.org/docs/latest-v14.x/api/esm.html#esm_dual_commonjs_es_module_packages While the "isolated state" solution seems to make more sense it causes trouble with rollup which supports CommonJS files only with an additional plugin, see rollup/rollup#3514. It is worth noting that webpack could deal with the "isolated state" solution since webpack supports CommonJS sources out of the box without further plugins and also doesn't get confused by `.cjs` file extensions that would have to be used in the state isolation approach for compatibility with Node.js. The wrapper approach should however work fine. Here's what code will be used in each case: 1. Node.js `require('uuid')` -> dist/index.js (CommonJS) -> dist/v1.js (CommonJS) 2. Node.js `import { v1 as uuidv1 } from 'uuid'` -> wrapper.mjs (ESM) -> dist/v1.js (CommonJS) 3. rollup/webpack (targeting Node.js environments) -> dist/esm-node/index.js (ESM) -> dist/esm-node/v1.js (ESM) 4. rollup/webpack (targeting Browser environments) -> dist/esm-browser/index.js (ESM) -> dist/esm-browser/v1.js (ESM)
a39089a
to
5c133ff
Compare
BREAKING CHANGE: Native ES Modules is still an experimental API in Node.js 14.0.0 and has so far not officially been supported by the
uuid
module.Since Node.js allows importing CommonJS modules it was possible to import the
uuid
module like this:This will no longer work with proper ES Module exports in place. You can now import the
uuid
library as described in the documentation:or
Enabling native ES Modules for Node.js requires some special care for the v1 algorithm which needs internal state. This makes this library susceptible to the dual package hazard described in https://nodejs.org/docs/latest-v14.x/api/esm.html#esm_dual_commonjs_es_module_packages
While the "isolated state" solution seems to make more sense it causes trouble with rollup which supports CommonJS files only with an additional plugin, see rollup/rollup#3514.
It is worth noting that webpack could deal with the "isolated state" solution since webpack supports CommonJS sources out of the box without further plugins and also doesn't get confused by
.cjs
file extensions that would have to be used in the state isolation approach for compatibility with Node.js.The wrapper approach should however work fine. Here's what code will be used in each case:
require('uuid')
-> dist/index.js (CommonJS) -> dist/v1.js (CommonJS)
import { v1 as uuidv1 } from 'uuid'
-> wrapper.mjs (ESM) -> dist/v1.js (CommonJS)
-> dist/esm-node/index.js (ESM) -> dist/esm-node/v1.js (ESM)
-> dist/esm-browser/index.js (ESM) -> dist/esm-browser/v1.js (ESM)
This is the first approach from #402 which I believe is currently more appropriate.
Fixes #245
Fixes #419
Fixes #342