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

Migrate to named exports and provide node esm support #241

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TrySound
Copy link

In this diff I added new entry point to compile es modules with named
exports to solve this problem (#240).

It's a breaking change so we can drop legacy node support (newer
versions of rollup and many other tools requires this) and add native
node es modules support.

In this diff I added new entry point to compile es modules with named
exports to solve this problem (json5#240).

It's a breaking change so we can drop legacy node support (newer
versions of rollup and many other tools requires this) and add native
node es modules support.
@jordanbtucker
Copy link
Member

Thank you for this PR, but I think it adds too much complexity compared to #243. Instead of one entry point (lib/index.js) there are now two (lib/index.js and lib/esm.js) as well as dist/index.mjs which is redundant. Removing dist/index.mjs is a breaking change, so you end up with two ESM compatible entry points that do the same thing, one used by Node.js and the other used by webpack.

There is also a module field and an exports field that each point to different entry points, which confuses things further.

You've also removed the browser field from package.json, which ensures that certain polyfills are included. And lib/esm.js cannot be used by browsers because both parse.js and stringify.js require util.js.

@TrySound
Copy link
Author

TrySound commented Oct 13, 2020

esm.js is used only for building proper exports in dist/index.mjs because generating exports from commonjs is not reliable and may change with rollup upgrade. I do not suggest to remove dist/index.mjs.

"module" is bundler specific field. "exports" is node specific way to support es modules natively. Probably only webpack 5 support both them yet.
https://nodejs.org/api/packages.html#packages_conditional_exports

Here's example
https://unpkg.com/browse/nanoid@3.1.12/package.json

"browser" field should not just point to umd. Bundlers prefer "browser" field over over "module" when specified as a string. This makes "module" field useless in this case. See the correct use case for "browser" field https://github.com/visgl/react-map-gl/blob/master/package.json#L17-L25

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.

None yet

2 participants