Review requested:

• @nodejs/loaders
• @nodejs/modules

Node's module map should be a stricter version of the one specified in ecma262, specifically because we resolve the canonical path for a module, which means it isn't just stable within each file, but across the entire graph of files.

1. Is there […] a spec violation that can be demonstrated in a repro?

Yes, see the test I've added

2. We already have a module cache: ModuleMap. It feels wrong to create a second one. If the first one behaves incorrectly, it should be fixed or replaced

Having two seems necessary, there are simply two different things, the first one is behaving correctly AFAICT.

According to https://tc39.es/ecma262/#sec-HostLoadImportedModule, if import() is called multiple times with the same (referrer, specifier) pair, then it must fulfil return the same object (or reject).

Isn't the same thing true for import('x') and import 'x' from the same referrer? Why do we need a secondary cache for dynamic import() specifically unless that invariant is broken right now?

Isn't the same thing true for import('x') and import 'x' from the same referrer?

The same thing is true yes, this PR ensures that both static and dynamic imports return the same thing.

Migrating #46826 (comment) over here:

const importVal1 = await import('module-name');

setTimeout(() => {
  const importVal2 = await import('module-name');

  console.log(importVal1 === importVal2);
}, 999);

The ECMAScript spec enforces that the above code always logs true

A simplified version of the above is this:

import('data:text/javascript,export default 333').then(async first => {
  const second = await import('data:text/javascript,export default 333')
  console.log(first === second)
})

In Node 20 for me, this logs true. If you replace the data URIs with ./file.mjs and create a file.mjs containing export default 333, it still returns true. So it would seem that we’re already spec compliant, at least for the common use case where there are no I/O issues.

I looked at the test in this PR, and it involves filesystem mutations. Please correct me if I’m misreading this, but it seems like what you’re testing is an initial import() succeeding, then deleting the imported file, and the second import currently fails because the file is missing but it should have succeeded because the initial success should have been cached? Is that the intent of the test, and the change that this PR is intending to make?

Please correct me if I’m misreading this, but it seems like what you’re testing is an initial import() succeeding, then deleting the imported file, and the second import currently fails because the file is missing but it should have succeeded because the initial success should have been cached? Is that the intent of the test, and the change that this PR is intending to make?

That's correct.

The same thing is true yes, this PR ensures that both static and dynamic imports returns the same thing.

Why isn't the ModuleMap cache ensuring that? If it doesn't, we should delete it so we have one cache that works instead of one that works and one that doesn't. Is the problem that the ModuleMap doesn't contain entries for failed loads? If so, can we add that to the module map cache?

Is the problem that the ModuleMap doesn’t contain entries for failed loads? If so, can we add that to the module map cache?

Per the spec we’re not supposed to caching failures, only successes. So if you tried to import a nonexistent file, then created the file and tried again to import it, the second attempt should succeed. https://tc39.es/ecma262/#sec-HostLoadImportedModule (emphasis added):

If this operation is called multiple times with the same (referrer, specifier) pair and it performs FinishLoadingImportedModule(referrer, specifier, payload, result) where result is a normal completion, then it must perform FinishLoadingImportedModule(referrer, specifier, payload, result) with the same result each time.

But I also feel like this should be achievable with our current ModuleMap cache without needing to create a new cache. Is the issue that we’re checking the filesystem for every import before looking to see if a module is already in our cache?

But I also feel like this should be achievable with our current ModuleMap cache without needing to create a new cache. Is the issue that we’re checking the filesystem for every import before looking to see if a module is already in our cache?

Yes that's the issue, defaultResolve is making fs operation even if a previous resolution has succeeded in the same module. I don't think we have another option than caching all the successful resolve operations.

Yes that’s the issue, defaultResolve is making fs operation even if a previous resolution has succeeded in the same module.

Where is defaultResolve making filesystem calls? I read through it and I’m not seeing them.

Perhaps it was doing so before #47824?

Of course it's doing fs operations, it needs to in order to resolve symlinks and node_modules deps.

Here's a few examples, defaultResolve is calling moduleResolve here:

Which itself is calling packageResolve and finalizeResolution:

And here where they make the FS call:

Within the error path of one of these failed filesystem calls, could we look in ModuleMap to see if the requested module was cached? That way we would avoid both creating a new cache and slowing down resolution in the default case where the filesystem hasn’t changed under our feet.

I’m also not convinced that this is a real problem. What is the use case where import calls need to continue working correctly after files were deleted from disk? Even if that’s technically the correct behavior per spec, it’s such an extreme edge case that it doesn’t feel like something we should add a performance regression to achieve.

Beware. Read up on why the web does cache errors.

I think what we’re discussing is the opposite though, caching the success so that subsequent imports continue to succeed even if the referenced files have been deleted.

I’m also not convinced that this is a real problem. What is the use case where import calls need to continue working correctly after files were deleted from disk?

I think that's off topic: if you don't like the spec, you should take it to TC39, but until then it seems pointless to discuss this in this PR.

Within the error path of one of these failed filesystem calls, could we look in ModuleMap to see if the requested module was cached?

I think it's still a problem if the resolution ends up on a different symlink (so we never end up on the error path yet we are breaking the spec).

Read up on why the web does cache errors

Caching the errors would simplify the implementation, it's certainly something we can think about.

I haven't read the code of this PR yet but I'm generally in favor of the behavior the spec requires here. We spent a great deal of time coming to the exact semantics we wanted to preserve and anyone who feels differently may want to review our prior discussions on this (you can check the dynamic import proposal repo and tc39 notes from that time) before pushing too hard.

Landed in 053511f