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. |
lukastaegert
left a comment
lukastaegert
left a comment
There was a problem hiding this comment.
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/[email protected]/dist/es/
TrySound
force-pushed
the
node-resolve-mjs
branch
from
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.
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.
For modules there is package.json with {type:module} in dist/es
There was a problem hiding this comment.
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.
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.
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.
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/[email protected]/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
* 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
6 participants
Rollup Plugin Name:
node-resolveThis 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.