feat: native Node.js ES Modules (isolated state) #402

ctavan · 2020-03-07T10:53:58Z

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 affects v1 uuid creation since we store internal state in the module:

uuid/src/v1.js

Lines 9 to 14 in ee039ee

    
           var _nodeId; 
        
           var _clockseq; 
        
           // Previous uuid creation time 
        
           var _lastMSecs = 0; 
        
           var _lastNSecs = 0;

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

    
           babel --env-name esmNode src --source-root src --out-dir "$DIR/esm-node" --copy-files --quiet

And again this whole stuff is still experimental in Node.js, so maybe it's too early to land this.

Clarify interaction with esm-node build.
Add documentation to README
Add test case
Clarify the need for rollup commonjs() plugin -> Support Dual CommonJS/ES Module Packages with isolated state rollup/rollup#3514
- 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 feat: native Node.js ES Modules (wrapper approach) #423 therefore seems to be the more robust solution.

TrySound · 2020-03-07T11:01:31Z

package.json

@@ -18,6 +18,10 @@
  },
  "sideEffects": false,
  "main": "dist/index.js",
+  "exports": {
+    "require": "dist/index.js",


require will trigger warnings on old node 13 versions. Looks like there is a solution around this
https://twitter.com/lukastaegert/status/1236188083621687298

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

Thanks for the hint. Although I’m definitely not planning to add workarounds for experimental APIs of old non-LTS node versions.

Also: how does the rollup solution prevent the dual package hazard?

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?

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.

got it. thanks.
well, its on dependent packages to provide es modules support. we can only ask to open issues on those packages instead.

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 that uuid 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 😂

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.

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 😉

ctavan · 2020-03-30T15:09:14Z

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?

TrySound · 2020-03-30T15:39:07Z

src/v1state.cjs

@@ -0,0 +1,7 @@
+module.exports = exports.default = {


module.exports.default. exports has old reference.

TrySound · 2020-03-30T17:15:38Z

examples/browser-rollup/package.json

@@ -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",


@rollup/plugin-node-resolve

@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?

After an hour of debugging I figured out the problem is symlink to uuid. The new version does not handle it properly.

@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?

I think it's a bug. I'll try to write test and fix the issue. Keep old package for now.

By the way "preserveSymlinks: true" in rollup config solves the problem with new package.

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 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

See: https://nodejs.org/docs/latest-v12.x/api/esm.html#esm_dual_commonjs_es_module_packages

frank-dspeed · 2020-04-24T14:01:39Z

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.

frank-dspeed · 2020-04-24T14:09:08Z

@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,

ctavan · 2020-04-27T15:53:43Z

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.

TrySound reviewed Mar 7, 2020

View reviewed changes

TrySound mentioned this pull request Mar 14, 2020

ES6 import support? #245

Closed

ctavan force-pushed the node-esm branch from 5a5bd73 to d6aa52a Compare March 30, 2020 15:25

TrySound reviewed Mar 30, 2020

View reviewed changes

TrySound mentioned this pull request Mar 31, 2020

node-resolve: resolve symlinked entry point properly rollup/plugins#291

Merged

9 tasks

ctavan force-pushed the node-esm branch 4 times, most recently from 4287d24 to e791432 Compare April 14, 2020 12:51

th1j5 mentioned this pull request Apr 17, 2020

Import error using Node with ES6 syntax #419

Closed

ctavan mentioned this pull request Apr 23, 2020

build: remove babel-plugin-add-module-exports #421

Merged

ctavan force-pushed the node-esm branch from e791432 to 0e755bf Compare April 24, 2020 07:42

This was referenced Apr 24, 2020

Support Dual CommonJS/ES Module Packages with isolated state rollup/rollup#3514

Closed

feat: native Node.js ES Modules (wrapper approach) #423

Merged

ctavan added 3 commits April 24, 2020 13:19

test: add test for Node.js ES Modules and CommonJS usage

d06b79c

Also remove unused npm script for browser-esmodules

feat: native Node.js ES Modules (isolated state approach)

33c8ad4

See: https://nodejs.org/docs/latest-v12.x/api/esm.html#esm_dual_commonjs_es_module_packages

ctavan force-pushed the node-esm branch from 0e755bf to 33c8ad4 Compare April 24, 2020 11:26

ctavan changed the title ~~feat: native ES Module support for Node.js~~ feat: native ES Module support for Node.js (isolated state) Apr 24, 2020

ctavan changed the title ~~feat: native ES Module support for Node.js (isolated state)~~ feat: native Node.js ES Modules (isolated state) Apr 24, 2020

ctavan closed this Apr 27, 2020

ctavan deleted the node-esm branch April 29, 2020 19:59

ctavan mentioned this pull request Apr 30, 2020

No valid exports main found for '...\node_modules\uuid' #428

Closed

ctavan mentioned this pull request Jun 8, 2020

Add support for pkg.exports.module or similar webpack/webpack#11014

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: native Node.js ES Modules (isolated state) #402

feat: native Node.js ES Modules (isolated state) #402

ctavan commented Mar 7, 2020 •

edited

Loading

TrySound Mar 7, 2020

TrySound Mar 7, 2020 •

edited

Loading

ctavan Mar 8, 2020

ctavan Mar 8, 2020

TrySound Mar 8, 2020

broofa Mar 8, 2020

TrySound Mar 8, 2020

ctavan Mar 8, 2020 •

edited

Loading

TrySound Mar 8, 2020 •

edited

Loading

ctavan Mar 8, 2020

ctavan commented Mar 30, 2020

TrySound Mar 30, 2020

TrySound Mar 30, 2020

ctavan Mar 31, 2020

TrySound Mar 31, 2020

ctavan Mar 31, 2020

TrySound Mar 31, 2020

TrySound Mar 31, 2020

TrySound Mar 31, 2020

frank-dspeed commented Apr 24, 2020

frank-dspeed commented Apr 24, 2020

ctavan commented Apr 27, 2020

	var _nodeId;
	var _clockseq;

	// Previous uuid creation time
	var _lastMSecs = 0;
	var _lastNSecs = 0;

feat: native Node.js ES Modules (isolated state) #402

feat: native Node.js ES Modules (isolated state) #402

Conversation

ctavan commented Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

TrySound Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctavan Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

TrySound Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctavan commented Mar 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frank-dspeed commented Apr 24, 2020

frank-dspeed commented Apr 24, 2020

ctavan commented Apr 27, 2020

ctavan commented Mar 7, 2020 •

edited

Loading

TrySound Mar 7, 2020 •

edited

Loading

ctavan Mar 8, 2020 •

edited

Loading

TrySound Mar 8, 2020 •

edited

Loading