ESM loading loads duplicate, starting with node 16.14.0 when using --experimental-specifier-resolution=node

[SanderElias]

Version

v16.14.0

Platform

Linux xxXxx 5.13.0-30-generic #33-Ubuntu SMP Fri Feb 4 17:03:31 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

I'm the main author of Scully.
In our @next version we switched completely to ESM modules. I have an internal register of plugins.
Up to version 16.4.0, our code works as expected.
After that, I'm erroring out on plugins that are not 'found'. This seems to happen because I get a new copy of the plugin registry.
That would only be possible as somehow the module loading imports the same thing twice.

It does not happen when I don't use the --experimental-specifier-resolution=node option.

However, as I have to load a piece of code dynamically, I do need that option. I have no control over the code I need to load, and it is importing without specifying the extension.
So, if I leave off the option, and I try to load the dynamic code I'm getting a load of Error [ERR_MODULE_NOT_FOUND]: Cannot find module errors.

A workaround could be if I can somehow tell NodeJs that the dynamic code is ESM, and is available with the .js extension.
There is already a package.json in the dynamic folder that has {type: "module"}

So, I'm unsure what exactly triggered this change in behavior between node 16.13.2 and 16.14.0(and later, the latest 17 has the same issue), But I would love a way so I can work around this.

Its going to be time-consuming to create an simple replication out of this, so I hope I can get some guidance here.

How often does it reproduce? Is there a required condition?

every time I run the program with >16.14.0

What is the expected behavior?

For my programs to work as before.

What do you see instead?

The error my program shows is there because we handle error conditions, and doesn't add anything to this issue.
Before 16.14:

After:

Additional information

No response

[aduh95]

In your Before 16.14 screenshot, there's no --experimental-specifier-resolution, is this expected?

A workaround could be if I can somehow tell NodeJs that the dynamic code is ESM, and is available with the .js extension.
There is already a package.json in the dynamic folder that has {type: "module"}

.js files inside a directory with a package.json containing "type":"module" are parsed as ESM, --experimental-specifier-resolution has no impact on that.
A minimal repro would be helpful to understand what the bug is here.

[SanderElias]

@DerekNonGeneric I replaced the hostname of the machine with xx's as I saw no reason to advertise that ;)
@aduh95 Well, I should have added a comment about the missing --experimental-specifier-resolution. It works with it on everything <16.14, but not after that. It saved some time by not switching versions for the screenshot. The output is 100% the same.

I'll try to create a minimal reproduction today.

[SanderElias]

I created a reproduction for the Error [ERR_MODULE_NOT_FOUND]: Cannot find module part of my problem.
you can find it here

The duplicated loading is way harder to reproduce, as it involves loading a 3rth party.

[aduh95]

OK so the steps are:

1. The entry point dynamically import a third-party module
2. The third party module statically import a submodule without specifying the extension (import "./module" instead of import "./module.js")

I'm afraid this is the expected result, see https://nodejs.org/api/esm.html#mandatory-file-extensions. When using ESM, you must use file extension in imports, and/or use --experimental-specifier-resolution=node CLI flag.

I think the duplicated loading is the most interesting problem, that's too bad it's also the most difficult to reproduce. Could you try with Node.js 17.0.1 and Node.js 17.1.0 please? We've made some changes between those two versions that were backported only recently to v16.14.0, I'm curious if that could be related.

[SanderElias]

@aduh95 I know it is expected behavior, that is why I'm using the CLI flag, to begin with.
However, I would love to have a way to programmatically tell the node runtime that this import (and all triggered by it) is ESM.
That would allow me to load the modules anyway. I did put a package.json in the third-party folded that states everything in that folder is ESM, but that seems to be ignored. Also, it would allow me to move away from the CLI flag.
Users are going to have questions about that anyway

@DerekNonGeneric

Could you try with Node.js 17.0.1 and Node.js 17.1.0 please?

I just did. 17.0.1 seems to work, whereas 17.1.0 doesn't. (I'm getting other issues, that are mine to fix, but the issue is the same as between 16.13.2 and 16.14, delivering me the same error.

[SanderElias]

@aduh95

Could you try with Node.js 17.0.1 and Node.js 17.1.0 please? We've made some changes between those two versions that were backported only recently to v16.14.0, I'm curious if that could be related.

I did this, and it shows exactly the same behavior. works in 0.1 and breaks in 1.0. may I ask about those changes? It might well be something I'm doing. as I'm walking the edge on what can be done. Knowing what those changes are, might lead me to a workaround, or even to a fix.

[aduh95]

I just did. 17.0.1 seems to work, whereas 17.1.0 doesn't. (I'm getting other issues, that are mine to fix, but the issue is the same as between 16.13.2 and 16.14, delivering me the same error.

Thanks, that's very helpful :)

I would love to have a way to programmatically tell the node runtime that this import (and all triggered by it) is ESM

So, you can try the road of loader hooks (https://nodejs.org/api/esm.html#hooks) – while it's still experimental, since you are using --experimental-specifier-resolution anyway, I thought I mention them. Loader hooks API allows you to customize how Node.js resolves a package specifier, so you could tell Node.js that to parse all modules as ESM. However:

• If you still want to support CJS, it will be tricky to implement.
• It still requires to use a CLI flag.

Also, it would allow me to move away from the CLI flag.

I think you lost me there, why would that change? The CLI flag you are using only change how Node.js interprets the string you pass to the import statement, AFAIK it doesn't (or at least shouldn't) change how Node.js parses the files (that's only based on the file extension when you don't use loader hooks).

may I ask about those changes?

There was a resolver spec hardening, whose changes are described in #40510 (comment) which could be related.
We also added support for import assertions, which made us rework the ESM module cache implementation. The commit is 95e4d29eb4 for reference.

[SanderElias]

Hmm, I'm not familiar enough with the node source code to have a verdict, but the PR you linked might have a clue.
I'm seeing this in the old code:

 {
      [internalFS.realpathCacheKey]: realpathCache
    }

Where I don't see how this is cached in the updated code.
However, this should make a difference when the URL is parsed slightly differently.
I'll check my code to see if that is happening.

added:
Aditional thought. @aduh95, Might it be that If I use a deep import from a library, vs a relative import in the library itself, I end up with 2 versions of the same thing?

[aduh95]

Might it be that If I use a deep import from a library, vs a relative import in the library itself, I end up with 2 versions of the same thing?

I've tried to test this hypothesis, but that doesn't seem to be the case. Here's the script I used to reproduce in case you want to iterate on it:

mkdir repro
cd repro
mkdir -p node_modules/third_party
echo 'import {a} from "./index-symbolic-link";a.val="mutated value";console.log("should be logged only once");' > node_modules/third_party/submodule.mjs
echo 'import("./submodule.mjs"); export const a = {val:"original value"};' > node_modules/third_party/index.mjs
ln -s ./index.mjs node_modules/third_party/index-symbolic-link.mjs
echo '{"exports":"./index.mjs"}' > node_modules/third_party/package.json
echo 'import {a} from "third_party";import {a as b} from "./node_modules/third_party";setTimeout(() => console.log(a,b), 300);' > index.mjs
node --experimental-specifier-resolution=node index.mjs
cd ..
rm -r repro

[SanderElias]

Small update. I can reproduce the issue without a hitch in our mono-repo. However, I have been working on an isolated reproduction, and have not yet been able to do that. The errors I'm getting are only possible when I get the same ESM module twice.
It really seems related to the way node determines the 'unique' identifier for a module, as pointed out by the commits @aduh95 pointed me to.
As soon as I will have more detailed info, I will post it back here.

[aduh95]

@SanderElias do you think your problem may be related to #42195? Any chance you would be able to compile node from #42197 to check if that solves your problem?

[GeoffreyBooth]

cc @nodejs/modules @nodejs/loaders

FYI --experimental-specifier-resolution=node is unmaintained, so I wouldn’t rely on any solutions that depend on that. We’re focusing our efforts on the Loaders API.

[SanderElias]

@aduh95

do you think your problem may be related to #42195?

That really looks like the same problem. I'm using symlinks too. However, I couldn't reproduce with only a symlink, so there must be something more.

Any chance you would be able to compile node from #42197 to check if that solves your problem?

If I had the time available I would love to, but my current schedule doesn't allow me to. (I have to take in account that I'm pretty rusty in this area)

[SanderElias]

@GeoffreyBooth When I decided to go with --experimental-specifier-resolution=node it was for 2 reasons alone.

1. this warning in the docs:

Note: The loaders API is being redesigned. This hook may disappear or its signature may change. Do not rely on the API described below.

2. the fact that I needed an even "uglier" cmd-line option, that includes a custom script.
   Let me elaborate on this, people looking for a solution might distrust a program where they are forced to start it in some "bizarre" way. It would take a whole lot of persuasion to sway them to that this is in fact a good Idea™️ On top of that, I would have to distribute the loader, and make it available to every user too. That's another hurdle to tackle.

If/When I would be able to add a loader without having to resort to cmd-line options, this would be much easier to adapt.

[aduh95]

@SanderElias You can also use one of the nightly builds (the most recent one at the time of writing can be found at https://nodejs.org/download/nightly/v18.0.0-nightly202203074158d62dec/ and does contain the fix), so you don't have to set up a whole dev env :) If you were able to download that version and report if if the issue is still there, that'd be awesome (and if the issue is gone, feel free to close this one).

[SanderElias]

@aduh95 Sory for the (involuntary cv19) delay.
I did just test with 18.0.0-nightly202203096b004f1bb1, and all works as expected. I didn't do any benchmarks, but it did feel faster with the nightly also.

[aduh95]

it did feel faster with the nightly

The nightly is built from the master branch, which uses a more modern version of V8 than v16.x, which would explain the speed improvement :)
Anyway, good to hear that fixed it for you, closing the issue now, thanks for the report. Have in mind that the --experimental-specifier-resolution flag is not here to stay, and will disappear when loaders can achieve the same thing as it's doing, it's fine to use it until then as long as you're understand the risks.

[SanderElias]

@aduh95 Thanks for the support, and I'm glad this will be fixed in the next releases!

Also, thanks for the head-up, and I'm well aware of this "risk". Personally, it really saddens me, that the use of ESM is still considered 'a risk' or experimental in 2022.

[aduh95]

Personally, it really saddens me, that the use of ESM is still considered 'a risk' or experimental in 2022.

Yes, me too! You'd think that there would be companies interested in making Node.js ESM implementation extensible via a stable API, but it looks like there are no takers but the few volunteers who are doing the actual work on their free time for the most part...