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

Add --import <module> flag for pre-loading ESM modules #40110

Closed
Qix- opened this issue Sep 14, 2021 · 23 comments · Fixed by #43942
Closed

Add --import <module> flag for pre-loading ESM modules #40110

Qix- opened this issue Sep 14, 2021 · 23 comments · Fixed by #43942
Labels
cli Issues and PRs related to the Node.js command line interface. esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale

Comments

@Qix-
Copy link

Qix- commented Sep 14, 2021

Is your feature request related to a problem? Please describe.
-r can be used only on CommonJS modules, as documented here. Pre-loading an .mjs or an ESM module package is currently not possible.

Describe the solution you'd like
Either expansion of -r to support ESM modules (might break some people, not sure the scope of those changes) or the inclusion of a --module, -m <module> flag. Neither the long nor short arguments appear to exist in either Node or v8 according to the above docs (correct me if I'm wrong).

Describe alternatives you've considered
None, I perceive this to be a hole in the CLI option set.

@targos
Copy link
Member

targos commented Sep 14, 2021

@nodejs/modules

@bmeck
Copy link
Member

bmeck commented Sep 14, 2021

Would love and endorse this (likely under --import instead of --module). It has been tried a few times but async hook tests almost all blow up when you make the bootstrap async instead of the current sync bootstrap. I do not have the time for the fairly hefty testing refactor to make this work but the actual implementation effort is small.

@Qix-
Copy link
Author

Qix- commented Sep 14, 2021

Agreed, --import is much better - good catch :)

@targos targos added cli Issues and PRs related to the Node.js command line interface. esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. labels Sep 14, 2021
@GeoffreyBooth GeoffreyBooth changed the title Add --module <module> flag for pre-loading ESM modules Add --import <module> flag for pre-loading ESM modules Sep 14, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Apr 4, 2022
@ljharb
Copy link
Member

ljharb commented Apr 4, 2022

bump

@targos targos added never-stale Mark issue so that it is never considered stale and removed stale labels Apr 4, 2022
@Qix-
Copy link
Author

Qix- commented Jul 11, 2022

@benjamingr cc #43382 (comment)

@benjamingr
Copy link
Member

@nodejs/loaders if I was to find someone to work on this does this feature have reasonable consensus?

@aduh95
Copy link
Contributor

aduh95 commented Jul 12, 2022

The issue has been open for almost a year with no objections in response, and even some support for it; I'd say it's reasonable to assume it has consensus, it only need someone to do the work. Note that now that Node.js supports chaining loaders, using --loader already gives you more or less that functionality, maybe we don't need a new flag after all, just to adjust the --loader to account for this use-case.

@benjamingr
Copy link
Member

@MoLow is this something you'd be interested in working on?

@MoLow
Copy link
Member

MoLow commented Jul 12, 2022

@MoLow is this something you'd be interested in working on?

Id like to yes :)

@GeoffreyBooth
Copy link
Member

I second what @aduh95 wrote. I don't think --loader is a good substitute for --import; there are reasons to load a package absolutely first, like polyfills or environment variable stuff, that loaders could do but are a poor fit for. As we're trying for parity with CommonJS, having a flag equivalent to --require makes sense.

@Flarna
Copy link
Member

Flarna commented Jul 12, 2022

Not sure if this was discussed already somewhere but I haven't found it:

  • --import will be processed before or after --loaders?
  • Which priority/sequence is intended for --require vs --import?
  • I guess --import will be allowed multiple times like --require. Correct?

@targos
Copy link
Member

targos commented Jul 12, 2022

I don't think it was discussed already. Maybe it's worth a loaders-agenda label?

I guess --import will be allowed multiple times like --require. Correct?

That's what I'd expect.

@GeoffreyBooth
Copy link
Member

Not sure if this was discussed already somewhere but I haven't found it:

  • --import will be processed before or after --loaders?

After. Loaders should always be first, before --require too.

  • Which priority/sequence is intended for --require vs --import?

If both are used together, you mean? I don’t know. I would think that all packages defined by either are loaded in the order defined, like --import one –require two –import three –require four would be interspersed; but I can see how that might be an issue depending on which loader is handling the entry point. I think ideally try to match the user’s input, the same as if all had been --require, and if that's not possible we'll have to investigate the best alternative (and document it well).

  • I guess --import will be allowed multiple times like --require. Correct?

Yes.

@aduh95
Copy link
Contributor

aduh95 commented Jul 12, 2022

Why do we need two flags exactly? It seems to me that it would be much simpler to re-use the --loader flag here. Adding another flag raises more questions than it answers..

@GeoffreyBooth
Copy link
Member

Why do we need two flags exactly? It seems to me that it would be much simpler to re-use the --loader flag here. Adding another flag raises more questions than it answers..

I'm not sure I follow. The loader flag expects a file with particular exports, like an exported resolve function. I think the use case for this feature is to load arbitrary packages that aren't loaders, like what --require is used for.

@aduh95
Copy link
Contributor

aduh95 commented Jul 12, 2022

The loader flag expects a file with particular exports, like an exported resolve function.

It would gladly accept a file with no exports though. The only use case for having a separate flag would be if one wanted to run a flag that exports whose name coincide with a loader, but not as a loader. That seems very unlikely, and quite easy to workaround anyway. --require is used to inject CJS loaders as well, so why not keep it to one flag in ESM world as well?

@GeoffreyBooth
Copy link
Member

Because I don't think code in --loader is in the same scope as application code? Especially not after we move it off thread.

@ljharb
Copy link
Member

ljharb commented Jul 12, 2022

That --require is also used for CJS loaders is because CJS has no official loader mechanism; a common use case for it is side-effecting polyfills, which if authored in ESM, would need --import.

@link89
Copy link

link89 commented Jul 13, 2022

How about to start with implementing --experimental-import to make it work first. Currently there is no workaround to preload ESM.

@MoLow
Copy link
Member

MoLow commented Jul 13, 2022

How about to start with implementing --experimental-import to make it work first. Currently there is no workaround to preload ESM.

👍🏻 I am working on this and hope to ship a initial PR this/next week

@MoLow MoLow mentioned this issue Jul 22, 2022
1 task
nodejs-github-bot pushed a commit that referenced this issue Jul 31, 2022
PR-URL: #43942
Fixes: #40110
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
PR-URL: nodejs#43942
Fixes: nodejs#40110
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
lachrist added a commit to getappmap/appmap-agent-js that referenced this issue Sep 23, 2022
The responsibility of the preloaded modules are confusing. Now the esm 
module loader has its own file. When node will support esm module 
preloading with `--import` flag it will be even better because it does 
not change the module loader for the main file -- cf: 
nodejs/node#40110
aduh95 pushed a commit to aduh95/node that referenced this issue Sep 7, 2023
PR-URL: nodejs#43942
Fixes: nodejs#40110
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this issue Sep 8, 2023
PR-URL: #43942
Backport-PR-URL: #49539
Fixes: #40110
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this issue Sep 13, 2023
PR-URL: #43942
Backport-PR-URL: #49539
Fixes: #40110
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@bpstrngr
Copy link

bpstrngr commented Nov 13, 2023

I'm trying to upgrade from the old --loader to the new --import flag for module customization hooks now, but i'm encountering an issue with process.argv going missing when the --import flag is used (both in the module registration worker, and the main process), unlike without the flag being used:
image
image
( for full picture, this is where the hooks get called in each case: )
image
I have seen similar occur with the --watch flag not being present in execArgv when used, but that is less critical and i can imagine a reason for it.
Is there a reason for process.argv disappearing with --import modules?
Tested with Node 20.9 and 21.1, happens in both versions.

@GeoffreyBooth
Copy link
Member

@bpstrngr please open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Issues and PRs related to the Node.js command line interface. esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale
Projects
Development

Successfully merging a pull request may close this issue.