Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adds es modules support and tests #126
Adds es modules support and tests #126
Changes from 8 commits
46729a9
fec2197
0ff4aef
ad71efd
3160d50
d819465
cf03cbb
c7980ae
965b93e
1007d3e
964103a
2ee4347
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why not add scripts to copy
tslib.es6.js
tomodules/index.js
?import cjs from mjs will cause warning in Angular CLI.
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.
Just wanna add that I was getting an error around this line in a react application after this update:
Let me know if I shall create an issue for this.
Some specs:
React 16.13.0
Bundled using: Webpack 5
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.
@udayrajMT if you could make a sample that'd be great, I have some webpack tests using modules in here but must have missed something 👍🏻
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.
Tried running it on a react-start-kit with the upgraded version, but that seems to run fine 🤔
Will check my configuration again and let you know if I find a way to reproduce the issue in a standalone app
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.
This pattern definitely works if the
.js
extension is important for both. I take it that is the constraint you're working too, even if.mjs
would be slightly simpler.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.
Curious, what was the point of adding this almost empty package.json with no package name, no package version?
Sincerely,
ES Module Bundlers
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.
https://nodejs.org/api/packages.html#type
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.
Hmm, is it best to comprehensively interpret this spec like this though? To just drop a nested, nearly empty
package.json
in a distribution directory, when thinking about all the nuances that go into an NPM package and bundling/transpiling packages?And how is an NPM package like pkg-dir expected to behave now, in it's claim to:
The above
pkg-dir
behavior will fail to resolve the root of the NPM package fortslib
after an ESM bundler follows to path./imports/index.js
declared inpackage.json#exports['.'].imports
and then executespkg-dir
with a cwd of:node_modules/tslib/imports/index.js
It seems the case that NOT using the
.mjs
extension was a shortcut and may be more the problem hereThere 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.
The spec is quite intentionally written like this as far as I know. It intentionally doesn't use something like "package.json at the root of the package directory" multiple times
Even before node has supported
.mjs
,.cjs
,package.json#type
and more, it was already possible to define nestedpackage.json
files. It was possible to putpackage.json#main
there to "redirect" the request to another location. I've been using this for years already.I'm not sure if this package is implemented correctly but a lot of similar resolvers were implemented correctly in the past. All modern module bundlers etc already work just OK with those nested
package.json
files. The rule, in fact, is quite simple if you want to just grab the root of a pkg, arguably you wouldn't even have to look forpackage.json
files but rather just find a directory nested directly withinnode_modules
directoryThere 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.
Please provide
"module"
condition as well. It's going to be supported by the upcoming webpack 5 and will allow it to keep only a single copy oftslib
in the bundle. Point of reference: https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62#reference-syntaxThere 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'm open to covering this in a new PR, that gist isn't really enough docs for me to go with yet
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.
Sure, can be added afterwards.
There are no real docs for this yet on their page but this gist has been updated multiple times when they have been iterating over semantics. This is already implemented and shipped in webpack 5 RC.
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.
@Andarist do you have any examples of packages using the
"module"
condition successfully for reference here?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.
@guybedford I did not (mainly because conditional exports themselves are rather rare to find so far), but I've decided to showcase this using one of my smaller modules. Here are exports that I have defined:
https://github.com/Andarist/callbag-last-element/blob/07099fd55b4dfff80a7697065c71ce9b5fc07ade/package.json#L7-L14
and here is the webpack output:
https://github.com/Andarist/webpack-module-condition-test/tree/7bca3a5ae4f07b5429c7b36db3d43b2072b39cf2/src
Notice how
last
function is only there a single time in this bundle even when mixing ESM and CJS files.The input for this is as simple as:
and can be found here (same repo)
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 required, but I'd be tempted to say you should move the
"dependencies"
from https://github.com/microsoft/tslib/pull/126/files#diff-f1e4194bc14f4a4ca169da388d9135de to each individual test so they run in isolation.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.
Aye, normally this is something I'd just throw into a yarn workspace and forget, but I'll make them all conform to a similar scripts structure then just loop through them