-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
build,module: add node-esm
support
#49407
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
I think the subsystem should be I quite like the opt-in strategy, but it doesn't seem very reasonable to expect a lot of people to use it, but maybe that's a feature and not a bug, that gives us time to assess wether we actually want/need it. Things that would need to happen before this can land:
|
Doesn't need to happen in this PR, but if we are really making a new binary (I expect this to be very controversial, and I would be surprised if no one was against it), I would like to see us removing some quirks of
Another idea:
|
FWIW, my understanding is that this is not actually a new binary, but rather just a symlink to the existing On the other hand, having an entirely new binary because we failed to figure out first-class ESM support in the regular |
It would double the size of the release tarballs too. Not very appealing. |
2b60379
to
abd97f3
Compare
node-esm
node-esm
support
See #49295 (comment). I think step 1 is to create a flag that flips Node to default to ESM. Then step 2 would be to somehow create a binary to act as Node with that flag set, without doubling the size of our download. Ideally we’d do so in a way that we could have a third binary that does the reverse, so like |
Added basic test as a part of proof-of-concept, will add more tests and docs if there are any explicit approvals for adding this and no blocking concerns... Changed title to make it less ambiguous that it's an optional mechanism that is not shipped as independent binary. Not sure what's the problem with tarballs, AFAICT they support symlinks already?
I don't think hooks can be a reasonable replacement for shebang usecase. As developer, I want to prepend my scripts with Although this would be a cool task for code golf: make esm loader as
I'm very conflicted about this... As CLI user, I also want it to be omnivorous and automatically guess if it's URL or path; but as developer I can't see mixing URLs and paths as good decision at least for security: IMHO it's either only paths for There's no path requirements or adjustments from shebang for arguments: it only requires path to
That's true, symlinks usually come with complications. But good thing is that users can do it differently: rename, or make a copy, or symlink, or hardlink. For production-ready usage with package managers, I think, hardlink can be preferable way to install it, similar to how all |
You could have |
Asking users to install a npm package globally just to run ESM scripts doesn't sound much better. This should be supported by Node.js in straightforward way without extra steps. Is it possible run release build on this to see if it affects the size in any way? |
I think you should be able to build locally via I just noticed another approach in https://gist.github.com/dmohs/13e2ea044707b77ff5d2af1a1c8585f4 per #34049 (comment). Perhaps this code could be used to create a second binary that just calls the first. This second binary doesn’t seem to contain the |
This PR adds
node-esm
(ornode-esm.exe
) supportnode-esm
is ESM-first version ofnode
It resolves unscoped
.js
files, extensionless files and unknown-extension files as ES modulesSupports shebang OOTB
To use this feature, user can rename
node
tonode-esm
Or just create symlink:
ln -s "$(which node)" /usr/local/bin/node-esm
Or it can be done by packaging system maintainer so users will have the symlink in
/usr/bin/
For convenience, build process creates
node-esm
symlink inout/Release/
Any feedback is welcome.
Fixes: #32316
Fixes: #33223
Fixes:: #34049
Fixes: #37512
Fixes: #42469
Refs: #49295 (comment)