Repository navigation
--import order is wrong when one of the modules imports a module #50427
Description
Activity
@nodejs/loaders
It's probably because these imports are executed in order, but not in a forced sequence.
I guess currently we have the equivalent ofPromise.all(specifiers.map(s => import(s))), but I haven't looked at the code that handles this CLI option.The relevant code is here:
node/lib/internal/process/esm_loader.js
Lines 24 to 30 in 0a0b8df
So it’s essentially as if you had written a single import file with:
import './a.mjs' import './b.mjs'
Instead of:
await import('./a.mjs') await import('./b.mjs')
We could change the current behavior from the first option to the second, but I think it would be a breaking change.
Reacted by Vladimir Grenaderov and Jithil P PonnanI did not find this by accident. I wrestled with figuring out why my loaders were loading in a different order than I specified with --import.
This will people hard. Definitely goes against the principle of least surprise.
I think this must be changed.
It also differs from how
--requireworks, because by the nature of CommonJS everything in the first--requireis loaded before the second one starts (though async activity kicked off by the first is not necessarily resolved before the second begins). I guess we could consider this a bug and therefore not a breaking change? @targos what do you think?In the static import case, b isn’t evaluated until all of a is finished, so it would be in serial i think?
We could change the current behavior from the first option to the second, but I think it would be a breaking change.
But we need it to emulate loader chaining without extra hacks & wrappers. @arcanis, what do you think about Yarn PnP and possible
--loader->--importtransition?Definitely sounds to me like a bug, not a major change. It would be entirely reasonable for a user to expect that the imports happen in serial as they are serial to the app entrypoint, just not serial amongst each other.
Reacted by Vladimir Grenaderov, Jordan Harband, Gil Tayar, Fabian Meyer, Jacob Smith and Jaakko SirénBut we need it to emulate loader chaining without extra hacks & wrappers. @arcanis, what do you think about Yarn PnP and possible
--loader->--importtransition?I didn't check yet but I'd also expect the
--importflags to be executed sequentially - I'm also not sure why it'd prevent loader chaining if it was the case, do you have an example in mind?I’m also not sure why it’d prevent loader chaining if it was the case, do you have an example in mind?
Because
importstatements are async, even if that async-ness is obscured. Consider:// a.js import { register } from 'node:module' import typescript from 'typescript' register('./hooks-a.js', import.meta.url) typescript.doSomethingWithThis() // b.js import { register } from 'node:module' register('./hooks-b.js', import.meta.url)
When run via
--import ./a.js --import ./b.js, Node evaluates this as if you had written a root file of:import './a.js' import './b.js'
The way ESM is specified, modules are evaluated from the leaves of the tree back up to the root. So the first code to get evaluated here is whatever is in
typescript, then back up toa.jsandb.js(not necessarily in that order). Others understand this better than I do; see https://stackoverflow.com/questions/35551366/what-is-the-defined-execution-order-of-es6-importsAnyway I agree that this is surprising and that Node should evaluate
--imports sequentially rather than in parallel. My only concern is whether changing this would break anyone.Reacted by Vladimir Grenaderov and Gil Tayarwhy it'd prevent loader chaining if it was the case, do you have an example in mind?
Sure. Let's assume that yarn use
--importinstead of--experimental-loaderfor .pnp.loader.mjs (withregistercall inside) and setNODE_OPTIONS='--import /project-path/.pnp.loader.mjs'. The following scenarios will fail:-
yarn node --import any-package app.js- can't resolveany-package, there is no node_modules on disk -
The same as 1 - can't resolve
cool-hooks:
// ts-transpiler.mjs import { register } from 'node:module'; register('cool-hooks'); // failed
yarn node --import ./ts-transpiler.mjs app.ts- Depends on whether
registerin .pnp.loader.mjs was executed or not:
// ts-transpiler.mjs import { register } from 'node:module'; register('./local-hook.mjs'); // local-hook.mjs const esbuild = await import('esbuild'); // random crashes
yarn node --import ./ts-transpiler.mjs app.ts-
yarn node --import any-package app.js- can't resolve any-package, there is nonode_moduleson diskIf it's made sequential, I'd expect the
--importcalls from the NODE_OPTIONS to be evaluated prior to the ones from the command line (same as for--requure), so it should work 🤔One thing to be careful about is that the resolution for
any-packagewon't be able to start until after the precedent imports have finished executing, but that's already the case for--loader, so as long as we keep that it sounds ok.Reacted by Gil Tayar and Vladimir GrenaderovWhen run via
--import ./a.js --import ./b.js, Node evaluates this as if you had written a root file of:import './a.js' import './b.js'
I'm pretty sure it's not the case.
The implementation you linked in #50427 (comment) literally does a (primordial-based)Promise.all. That's not what successiveimportstatements do.I agree that we should consider this a bug and change it to evaluate in a sequence.
Reacted by Antoine du Hamel, Jordan Harband and Jithil P PonnanThere are three way of supporting several
--import:- Concurrently (the current way):
await Promise.all(imports.map(moduleReferrer => import(moduleReferrer)));
- Using static imports (the linking is done concurrently, but execution order is guaranteed):
await import(`data:text/javascript,${encodeURIComponent(imports.map( moduleReferrer => `import ${JSON.stringify(moduleReferrer)}` ).join(';'))}`);
- Sequentially:
for (const moduleReferrer of imports) { await import(moduleReferrer); }
It seems that only the latter would allow chaining of loaders, the tradeoff being that if one of the module has a long-running TLA task, work will start later on other
--imported modules, but it's probably the only way for--importto compete with--require.Reacted by Michaël Zasso, Benjamin Gruenbaum, Vladimir Grenaderov, Gil Tayar and Jacob Smith- Concurrently (the current way):
It seems that only the latter would allow chaining of loaders, the tradeoff being that if one of the module has a long-running TLA task, work will start later on other
--imported modules, but it’s probably the only way for--importto compete with--require.Number 3 was what I was envisioning as well. It would also allow hooks registered in the first
--importto affect subsequent imports, which is a feature of--loaderthat should be present in--importas well. The following should work:node --import ts-node/register --import my-script.ts app.ts
@aduh95 are you already working on this?
Reacted by Jordan Harband and Benjamin GruenbaumTo clarify - (2) very much does permit chaining here since evaluation of each successive module will only happen after the previous has fully completed including with TLA support, with that benefit over (3) of continuing to parallelize the loading pipeline.
To clarify - (2) very much does permit chaining here since evaluation of each successive module will only happen after the previous has fully completed including with TLA support, with that benefit over (3) of continuing to parallelize the loading pipeline.
It doesn't, because the modules will still get resolved in parallel, meaning that a loader registered in the first
--importwouldn't be able to drive the resolution of subsequent--importcalls (ie something like--import import-map --import other-loader, whereother-loaderis a package registered in the import map).It doesn't, because the modules will still get resolved in parallel, meaning that a loader registered in the first --import wouldn't be able to drive the resolution of subsequent --import calls (ie something like --import import-map --import other-loader, where other-loader is a package registered in the import map).
That is not what the reported bug is in this issue, the bug reported and replication provided only refer to the import ordering bug.
I appreciate the loader chaining use case though in widening the fix to also support that case, in which case that seems a worthy justification for (3).
I appreciate the loader chaining use case though in widening the fix to also support that case, in which case that seems a worthy justification for (3).
Yes, I think the example I posted, where the second
--importis written in TypeScript, is only possible in 3?Reacted by Guy BedfordCan I take it?
- added a commit that references this issue
on Nov 1, 2023 - added 2 commits that reference this issue
on Nov 11, 2023 - added a commit that references this issue
on Dec 11, 2023
Version
v20.9.0
Platform
Linux XXXXXXX 5.15.90.1-microsoft-standard-WSL2 #1 SMP Fri Jan 27 02:56:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Subsystem
module
What steps will reproduce the bug?
git clone https://github.com/giltayar/import-order-bug.gitThe output here makes sense, as the order of execution of
a.jsandb.jsis firstathenba.jsand uncomment the first line (withimport './sub.mjs). Note thatsub.mjsis an empty file.Now run the same command again:
This doesn't make sense: the order of execution of the imports shouldn't change if one of the modules has an
importand the other doesn't.How often does it reproduce? Is there a required condition?
Every time.
What is the expected behavior? Why is that the expected behavior?
The order of the execution of the modules in
--importshould not change based on whether they are importing another module or not.What do you see instead?
The order of the execution of the modules in
--importchanges based on whether they are importing another module or not.Additional information
This stumped me for 3 hours when modifying my ESM loaders talk for NodeConf EU and the new
registerdidn't work with loader chaining. It took me 3 hours to figure out that the chaining didn't change, but the execution order in--importdoes, and that it has nothing to do with loaders.