-
-
Notifications
You must be signed in to change notification settings - Fork 593
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(node-resolve): add native node es modules support #413
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Not in this PR. Need to figure out if this is the way we want to go. |
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.
node-resolve already has a rollup build step that produces ESM output. While your suggestion is valid, there is no really a hard reason to avoid duplication here as the plugin is essentially stateless. What I do not like here is that the file is not in dist
, but more importantly that it needs to be maintained manually.
I would rather either reference the ESM build here via export * from...
if it should be non-breaking, or rename the ESM build to .mjs, which might be breaking for direct importers.
To place the file in dist
, we could just emit it inline in the rollup.config file.
But this is just my opinion. Also the recommendation for greater compatibility is to use
"exports": {
"import": "./index.mjs"
"default": "./dist/index.js"
}
as require
was added later and default
is a catch-all for all non-import-cases.
From my experience node will select 'default' slot to often and ignore 'import' |
Should we support old versions of "unstable" node? |
Order matters here, |
May not be too important, admittedly. |
I vote no. |
* feat(pluginutils): add native node es modules support Ref #412 #413 Enable native node es modules via "exports" field in package.json. Added custom plugin to generate nested package.json with `{"type": "module"}` to prevent mjs extension usage. * Export emitModulePackageFile from shared config * Bundle estree-walker * Swap options
Ref #412 Enable native node es modules via "exports" field in package.json. Added custom plugin to generate nested package.json with {"type": "module"} as an alternative to mjs extension usage. This works similar to rollup distribution. https://unpkg.com/browse/rollup@2.10.9/dist/es/
41da746
to
48d3fba
Compare
@lukastaegert please take another look at this one |
"import": "./dist/es/index.js" | ||
}, | ||
"module": "./dist/es/index.js", | ||
"type": "commonjs", |
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.
Should this say commonjs
instead of module
? When I was reading https://nodejs.org/api/esm.html#esm_conditional_exports I saw that they use type
module
together with exports
require
and import
.
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.
For modules there is package.json with {type:module} in dist/es
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.
Interesting. Is there a reason a nested package.json
approach is preferred to declaring everything in the top-level package.json
?
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.
Not all tools are ready to use native es modules. Some may just not work. So we go with commonjs default and enable module mode where necessary.
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 type: commonjs
in a package.json is not really necessary as this is the default anyway: https://nodejs.org/api/esm.html#esm_package_json_type_field
But we can still keep it for clarity's sake.
"import": "./dist/es/index.js" | ||
}, | ||
"module": "./dist/es/index.js", | ||
"type": "commonjs", |
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 type: commonjs
in a package.json is not really necessary as this is the default anyway: https://nodejs.org/api/esm.html#esm_package_json_type_field
But we can still keep it for clarity's sake.
Yes, I added type to be explicit. |
* feat(pluginutils): add native node es modules support Ref rollup#412 rollup#413 Enable native node es modules via "exports" field in package.json. Added custom plugin to generate nested package.json with `{"type": "module"}` to prevent mjs extension usage. * Export emitModulePackageFile from shared config * Bundle estree-walker * Swap options
Ref rollup#412 Enable native node es modules via "exports" field in package.json. Added custom plugin to generate nested package.json with {"type": "module"} as an alternative to mjs extension usage. This works similar to rollup distribution. https://unpkg.com/browse/rollup@2.10.9/dist/es/
* feat(pluginutils): add native node es modules support Ref rollup/plugins#412 rollup/plugins#413 Enable native node es modules via "exports" field in package.json. Added custom plugin to generate nested package.json with `{"type": "module"}` to prevent mjs extension usage. * Export emitModulePackageFile from shared config * Bundle estree-walker * Swap options
Rollup Plugin Name:
node-resolve
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
Wrapper module is the simplest solution. I used it in terser plugin
which cannot resolve worker without commonjs for now.