-
-
Notifications
You must be signed in to change notification settings - Fork 900
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 (isolated state) #402
Conversation
package.json
Outdated
@@ -18,6 +18,10 @@ | |||
}, | |||
"sideEffects": false, | |||
"main": "dist/index.js", | |||
"exports": { | |||
"require": "dist/index.js", |
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.
require will trigger warnings on old node 13 versions. Looks like there is a solution around this
https://twitter.com/lukastaegert/status/1236188083621687298
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.
Ah, here's what rollup end up with. Probably no need in mjs wrapper which would not work when exports support will come to bundlers (I mean in rollup for example).
https://github.com/rollup/rollup/pull/3430/files
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.
Thanks for the hint. Although I’m definitely not planning to add workarounds for experimental APIs of old non-LTS node versions.
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.
Also: how does the rollup solution prevent the dual package hazard?
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.
I'm not sure. Looks like some fields are just ignored in early node 13.
https://twitter.com/guybedford/status/1235329049779634177
Or what do you mean by dual package hazard?
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.
Well... it doesn't expose state externally, but v1 has those private state vars it maintains. For functional programming purists and anyone looking for idempotent behavior, v1 isn't gonna cut it.
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.
got it. thanks.
well, its on dependent packages to provide es modules support. we can only ask to open issues on those packages instead.
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.
I think there are 2 aspects w.r.t. uuid and the dual package hazard:
- I agree that most likely in practice it wouldn't be a problem to just not care about the
v1()
internal state… - On the other hand,
uuid
is so heavily depended upon that it is probably one of the very few modules on npm where the dual package hazard will actually manifest in the real world! I just checked a couple of normal web projects and found thatuuid
often appears up to 10-20 times as a transitive dependency: So unless the entire dependency chain was either 100% ESModule or 100% CommonJS (and that will likely not be the case for a very long time) the dual package hazard will manifest for real!
Again, while my pragmatic self thinks it's unlikely that this will cause bugs in the real world, it would still somewhat hurt the pedantic and principled regions of my brains to simply ignore this issue 😂
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.
Funny thing is that uuid in transitive dependencies may be uuid@3
for ages which is not much different from dual packages hazard and definitely not our responsibility. It's also possible the most used api is stateless uuid/v4.
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.
That’s a very valid point and in practice probably much more important than my musings about the dual package hazard, I totally agree with you.
But that still doesn’t convince me that we shouldn’t care about it and at least be exactly aware of what we are doing 😉
FYI I have built reproduction examples for the dual package hazard: https://github.com/ctavan/uuid-dual-package-hazard The two solutions described in https://nodejs.org/api/esm.html#esm_dual_commonjs_es_module_packages (wrapper & state isolation) work well. I tend towards using state isolation since this would allow us to ship one ESM build for node that can both directly be consumed by modern node versions as well as by module bundlers when targeting node. If we use the wrapper technique then I think we'll have to ship a separate ESM node build for bundlers. Otherwise bundlers would lose the ability to tree-shake Node.js code (which might not be a big deal, but also unnecessary). Opinions? |
src/v1state.cjs
Outdated
@@ -0,0 +1,7 @@ | |||
module.exports = exports.default = { |
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.
module.exports.default. exports has old reference.
examples/browser-rollup/package.json
Outdated
@@ -10,6 +10,7 @@ | |||
"uuid": "file:../../.local" | |||
}, | |||
"devDependencies": { | |||
"@rollup/plugin-commonjs": "^11.0.2", | |||
"rollup": "^1.32.0", | |||
"rollup-plugin-node-resolve": "^5.2.0", |
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.
@rollup/plugin-node-resolve
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.
@TrySound when I upgrade to @rollup/plugin-node-resolve
the {browser: true}
setting seems to be ignored. Here's the output of an npm run build
inside examples/browser-rollup
:
./example-all.js → dist/all.js...
(!) Plugin node-resolve: preferring built-in module 'crypto' over local alternative at 'crypto', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
crypto (imported by ../../dist/esm-node/sha1.js, ../../dist/esm-node/rng.js, ../../dist/esm-node/md5.js)
(!) Missing global variable name
Use output.globals to specify browser global variable names corresponding to external modules
crypto (guessing 'crypto')
created dist/all.js in 417ms
Any idea why?
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.
After an hour of debugging I figured out the problem is symlink to uuid. The new version does not handle it properly.
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.
@TrySound thanks for looking into this, you are of great help for this library!
Have you been able to figure out if that is an intended behavior of the new rollup plugin or a bug? If so, would you mind reporting it upstream with the information you discovered?
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.
I think it's a bug. I'll try to write test and fix the issue. Keep old package for now.
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.
By the way "preserveSymlinks: true" in rollup config solves the problem with new package.
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.
Fixed here rollup/plugins#291
The regression appeared here uuidjs/uuid#402 (comment) after upgrading from `rollup-plugin-node-resolve` to `@rollup/plugin-node-solve` Dependencies entry points are resolved into symlinked path when browser field map contains paths resolved into real paths.
The regression appeared here uuidjs/uuid#402 (comment) after upgrading from `rollup-plugin-node-resolve` to `@rollup/plugin-node-solve` Dependencies entry points are resolved into symlinked path when browser field map contains paths resolved into real paths.
4287d24
to
e791432
Compare
The v1 algorithm need internal state which 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 } 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)
Also remove unused npm script for browser-esmodules
Hello I am Frank Lemanschik a German Senior ECMAScript Consultant from the rollup Team i will investigate and clone https://github.com/uuidjs/uuid/tree/node-esm I will do all the needed and review it with the newest nodejs and older nodejs versions I have researched the best packaging patterns. |
@ctavan msged you on gitter maybe we can solve that more stable and fast when we talk before as I think there are some other changes needed maybe also I am not sure, |
Rollup considers itself as a pure ESM bundler, so supporting CommonJS modules out of the box seems out of scope for them. This means that we cannot ship the "isolated state" code for bundling with rollup. The wrapper approach proposed in #423 therefore seems to be the more robust solution. |
The regression appeared here uuidjs/uuid#402 (comment) after upgrading from `rollup-plugin-node-resolve` to `@rollup/plugin-node-solve` Dependencies entry points are resolved into symlinked path when browser field map contains paths resolved into real paths.
The regression appeared here uuidjs/uuid#402 (comment) after upgrading from `rollup-plugin-node-resolve` to `@rollup/plugin-node-solve` Dependencies entry points are resolved into symlinked path when browser field map contains paths resolved into real paths.
See: https://nodejs.org/docs/latest-v12.x/api/esm.html#esm_dual_commonjs_es_module_packages
Potentially closes #342
The native esmodule for node.js now uses an
.mjs
wrapper around the transpiled CommonJS code in order to avoid the dual package hazard which potentially affectsv1
uuid creation since we store internal state in the module:uuid/src/v1.js
Lines 9 to 14 in ee039ee
I'm not yet sure how this wrapper approach will interact with the node-specific esm build that we currently generate for module bundlers like webpack/rollup (for use in serverless environments):
uuid/scripts/build.sh
Line 21 in ee039ee
And again this whole stuff is still experimental in Node.js, so maybe it's too early to land this.
esm-node
build.commonjs()
plugin -> Support Dual CommonJS/ES Module Packages with isolated state rollup/rollup#3514