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

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

Merged
merged 1 commit into from Apr 29, 2020
Merged

Conversation

ctavan
Copy link
Member

@ctavan ctavan commented Apr 24, 2020

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:

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:

import { v4 as uuidv4 } from 'uuid';
uuidv4(); // ⇨ '9b1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d'

or

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)

This is the first approach from #402 which I believe is currently more appropriate.

Fixes #245
Fixes #419
Fixes #342

@ctavan ctavan requested review from broofa and LinusU April 24, 2020 09:24
@ctavan
Copy link
Member Author

ctavan commented Apr 24, 2020

@TrySound maybe you could also have a look at this one?

@ctavan
Copy link
Member Author

ctavan commented Apr 27, 2020

@broofa @LinusU would be nice to get your reviews on this one since it should solve a bunch of open issues.

Also given that Node.js 14.x is out now and supports ES Modules without commandline flags, I expect more "bug" reports if we don't support native ESM for Node.js

Copy link
Member

@TrySound TrySound left a 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.

@LinusU
Copy link
Member

LinusU commented Apr 27, 2020

Will this be released as a new major version?

@ctavan
Copy link
Member Author

ctavan commented Apr 27, 2020

Will this be released as a new major version?

@LinusU so far I was considering only a minor version bump (-> 7.1.0) since this only adds a new feature but doesn't introduce any breaking changes.

Do you think a major version bump would be justified?

@TrySound
Copy link
Member

I guess it will be a breaking change for native esm users. Currently only this way of import is possible

import uuid from 'uuid';
uuid.v1

@ctavan
Copy link
Member Author

ctavan commented Apr 27, 2020

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.

Thanks for explicitly noting this again! I agree 100%!

I'm already watching the corresponding issues btw:

@ctavan
Copy link
Member Author

ctavan commented Apr 27, 2020

I guess it will be a breaking change for native esm users. Currently only this way of import is possible

import uuid from 'uuid';
uuid.v1

Hmm, you are right:

uuid@7.0.3:

import uuid from 'uuid';
console.log(uuid.v4()); // -> 'cd6c3b08-0adc-4f4b-a6ef-36087a1c9869'

uuid@next:

import uuid from 'uuid';
       ^^^^
SyntaxError: The requested module 'uuid' does not provide an export named 'default'
import * as uuid from 'uuid';
console.log(uuid.v1()); // -> 'cd6c3b08-0adc-4f4b-a6ef-36087a1c9869'

Hmm, I now see two interpretations:

  1. We consider the current node esm behavior a bug / nonexistent feature that will be added -> 7.1.0
  2. We consider this a breaking change -> 8.0.0

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 .

@TrySound
Copy link
Member

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.

@ctavan ctavan force-pushed the node-esm-wrapper branch 2 times, most recently from b0e7c97 to e822c36 Compare April 28, 2020 06:53
@ctavan
Copy link
Member Author

ctavan commented Apr 28, 2020

@LinusU @broofa fine with minor version bump?

@LinusU
Copy link
Member

LinusU commented Apr 28, 2020

The reason I asked is because another library, is-promise, recently made the same change in a patch version, and it broke a lot of packages, including the widely used create-react-app.

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 import uuid from 'uuid' will stop working is, personally to me, enough of a thing to make this a breaking change. Also, require('uuid/package.json') will stop working, which is probably something that most people doesn't use, but it will break if they use it.

I'm quite sure that require('uuid/v1') will stop working on Node.js 13 & 14 as well, which also pushes me to want to do this in a major version.

My personal proposal is to remove the deep require files as well, and then release this as a new major release. ☺️

@ctavan
Copy link
Member Author

ctavan commented Apr 28, 2020

Thanks for the insights @LinusU. Let's hear @broofa before we come to a conclusion 😉

@broofa
Copy link
Member

broofa commented Apr 28, 2020

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 import {v4 as uuidv4} from 'uuid' (right?) I think a minor version is justifiable. But if you want to major-bump this for the sake of caution, I'd support that in this case.

Also, require('uuid/package.json') will stop working

Not part of public API. Not a concern.

@LinusU
Copy link
Member

LinusU commented Apr 29, 2020

Semver should only apply to changes in the documented, "official" API.

I do agree with this, although I think that since doing import uuid from 'uuid' was the only way to import uuid via Node.js ESM before this change, the odds of someone doing that are quite high.

Also, require('uuid/v4') is documented, although as deprecated, and I still think that that will stop working in Node.js 13 & 14 after this change.

I guess I'm a bit extra careful since this is a widely used module (~30M downloads/week).

Anyhow, just my two cents ☺️

wrapper.mjs Show resolved Hide resolved
Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

LGTM

@ctavan
Copy link
Member Author

ctavan commented Apr 29, 2020

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

Import error using Node with ES6 syntax ECMAScript Module for Node.js ES6 import support?
4 participants