--import order is wrong when one of the modules imports a module

[giltayar]

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?

1. Run git clone https://github.com/giltayar/import-order-bug.git
2. Run the following command:

$ node --import ./a.mjs --import ./b.mjs main.mjs
a...
b...

The output here makes sense, as the order of execution of a.js and b.js is first a then b

3. Go to a.js and uncomment the first line (with import './sub.mjs). Note that sub.mjs is an empty file.

Now run the same command again:

$ node --import ./a.mjs --import ./b.mjs main.mjs
b...
a...

This doesn't make sense: the order of execution of the imports shouldn't change if one of the modules has an import and 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 --import should 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 --import changes 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 register didn't work with loader chaining. It took me 3 hours to figure out that the chaining didn't change, but the execution order in --import does, and that it has nothing to do with loaders.

[benjamingr]

@nodejs/loaders

[targos]

It's probably because these imports are executed in order, but not in a forced sequence.
I guess currently we have the equivalent of Promise.all(specifiers.map(s => import(s))), but I haven't looked at the code that handles this CLI option.

[GeoffreyBooth]

The relevant code is here:

node/lib/internal/process/esm_loader.js

Lines 24 to 30 in 0a0b8df

	if (userImports.length > 0) {
	const parentURL = getCWDURL().href;
	await SafePromiseAllReturnVoid(userImports, (specifier) => esmLoader.import(
	specifier,
	parentURL,
	kEmptyObject,
	));

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.

[giltayar]

I 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.

[GeoffreyBooth]

It also differs from how --require works, because by the nature of CommonJS everything in the first --require is 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?

[ljharb]

In the static import case, b isn’t evaluated until all of a is finished, so it would be in serial i think?

[koshic]

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-> --import transition?

[Qard]

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.

[arcanis]

But we need it to emulate loader chaining without extra hacks & wrappers. @arcanis, what do you think about Yarn PnP and possible --loader-> --import transition?

I didn't check yet but I'd also expect the --import flags 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?

[GeoffreyBooth]

I’m also not sure why it’d prevent loader chaining if it was the case, do you have an example in mind?

Because import statements 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 to a.js and b.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-imports

Anyway 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.

[koshic]

why it'd prevent loader chaining if it was the case, do you have an example in mind?

Sure. Let's assume that yarn use --import instead of --experimental-loader for .pnp.loader.mjs (with register call inside) and set NODE_OPTIONS='--import /project-path/.pnp.loader.mjs'. The following scenarios will fail:

1. yarn node --import any-package app.js - can't resolve any-package, there is no node_modules on disk

2. 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

3. Depends on whether register in .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

[arcanis]

yarn node --import any-package app.js - can't resolve any-package, there is no node_modules on disk

If it's made sequential, I'd expect the --import calls 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-package won'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.

[targos]

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'

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 successive import statements do.

I agree that we should consider this a bug and change it to evaluate in a sequence.

[aduh95]

There are three way of supporting several --import:

1. Concurrently (the current way):

   await Promise.all(imports.map(moduleReferrer => import(moduleReferrer)));

2. 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(';'))}`);

3. 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 --import to compete with --require.

[GeoffreyBooth]

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 --import to compete with --require.

Number 3 was what I was envisioning as well. It would also allow hooks registered in the first --import to affect subsequent imports, which is a feature of --loader that should be present in --import as well. The following should work:

node --import ts-node/register --import my-script.ts app.ts

@aduh95 are you already working on this?

[guybedford]

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.

[arcanis]

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 --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).

[guybedford]

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).

[GeoffreyBooth]

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 --import is written in TypeScript, is only possible in 3?

[MrJithil]

Can I take it?