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

build,module: add node-esm support #49407

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

Conversation

LiviaMedeiros
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros commented Aug 30, 2023

This PR adds node-esm (or node-esm.exe) support

node-esm is ESM-first version of node
It resolves unscoped .js files, extensionless files and unknown-extension files as ES modules

Supports shebang OOTB

$ cat blep
#!/usr/bin/env node-esm
console.log(import.meta.url);
$ ./blep
file:///home/livia/blep

To use this feature, user can rename node to node-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 in out/Release/

Any feedback is welcome.

Fixes: #32316
Fixes: #33223
Fixes:: #34049
Fixes: #37512
Fixes: #42469
Refs: #49295 (comment)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 30, 2023
@aduh95
Copy link
Contributor

aduh95 commented Aug 30, 2023

I think the subsystem should be build: rather than module:, or maybe build,module:.

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:

  • add tests
  • answer the question "does this need to be in core when it can already be achieved in userland using a loader / module customization hooks?"
  • bike shedding the binary name
  • add docs?
  • avoid making any python2/python3 reference

@aduh95
Copy link
Contributor

aduh95 commented Aug 30, 2023

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 node regarding argv[1]:

  • no magic extension guessing: node index loads index.js, node-esm index should error out
  • absolute URL support: node file:///dev/null loads ./file:/dev/null, node-esm file:///dev/null should load /dev/null
  • keep absolute path support (required for shebang I believe): node /root/index.js?ext=.js and node-esm /root/index.js?ext=.js should both load index.js?ext=.js, node index.js; similarly node C:\index.js and node-esm C:\index.js should both load C:\index.js.
  • not sure what to do regarding relative path vs relative URLs

Another idea:

  • defaultLoad should return a source for CJS modules (with node, this is deferred to the monkey-patchable CJS loader for backward compat).

@tniessen
Copy link
Member

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)

FWIW, my understanding is that this is not actually a new binary, but rather just a symlink to the existing node binary. That comes with all kinds of complications, for example, forking using process.execPath will suddenly revert back to the original node binary.

On the other hand, having an entirely new binary because we failed to figure out first-class ESM support in the regular node binary sounds discouraging to me.

@bnoordhuis
Copy link
Member

It would double the size of the release tarballs too. Not very appealing.

@LiviaMedeiros LiviaMedeiros changed the title module: add node-esm build,module: add node-esm support Aug 30, 2023
@GeoffreyBooth
Copy link
Member

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 nodem and nodec for Node in ESM-default mode and Node in CommonJS-default mode, so that in a semver-major change we could make the regular node binary ESM by default.

@LiviaMedeiros
Copy link
Contributor Author

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?


does this need to be in core when it can already be achieved in userland using a loader / module customization hooks?

I don't think hooks can be a reasonable replacement for shebang usecase. As developer, I want to prepend my scripts with #!/usr/bin/env node-esm instead of telling the user "hey, if you wanna copypaste it and run, take this esmloader.mjs file and place it somewhere and don't forget to run it as node --experimental-loader path/to/esmloader.mjs cowsay.txt".

Although this would be a cool task for code golf: make esm loader as data: to fit in 128byte shebang limit. 😄

  • absolute URL support: node file:///dev/null loads ./file:/dev/null, node-esm file:///dev/null should load /dev/null

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: https: or %2e%2e%2f%50%57%4e%45%44%00 directories are not obvious to filter out.

IMHO it's either only paths for argv[1] and only URLs for applicable options, or subject for dedicated discussion.

There's no path requirements or adjustments from shebang for arguments: it only requires path to env to be absolute.

That comes with all kinds of complications, for example, forking using process.execPath will suddenly revert back to the original node binary.

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 git binaries work.

@aduh95
Copy link
Contributor

aduh95 commented Aug 30, 2023

I don't think hooks can be a reasonable replacement for shebang usecase. As developer, I want to prepend my scripts with #!/usr/bin/env node-esm instead of telling the user "hey, if you wanna copypaste it and run, take this esmloader.mjs file and place it somewhere and don't forget to run it as node --experimental-loader path/to/esmloader.mjs cowsay.txt".

You could have node-esm be a binary provided by an npm package you require your users to install before using your extensionless ESM script. node-esm would itself be registering the hooks, it doesn't need to be provided by us.

@LiviaMedeiros
Copy link
Contributor Author

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?

@GeoffreyBooth
Copy link
Member

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 make?

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 node one, so it shouldn’t be as large. I’m not a C++ developer so I’m not a good judge of how big this one would be; I assume it would be trivial if those #include lines are references to shared libraries rather than getting embedded into the built binary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
6 participants