esm: do not call getSource when format is commonjs

[fasttime]

This PR ensures that defaultLoad does not access the file system to read the source of modules that are known in advance to be in CommonJS format. This is not necessary, since the CommonJS loader can't use the source property and must instead read the file once again.

For context, this was noticed while investigating a test failure in ESLint on Node.js 21.1.0. Our logic (now fixed) was incorrectly assuming that some asynchronous operations would complete in a specific order, and this order has changed from Node.js 21.0.0 to 21.1.0.
The simplest repro I've found is this one:

// index.mjs
await import('./file1.cjs');
process.nextTick(() => console.log('Tick'));
await import('./file2.cjs');
console.log('Done');

with file1.cjs and file2.cjs both empty modules.

This is the output of running index.mjs in Node.js 21.0.0:

Done
Tick

And this is the output in Node.js 21.1.0:

Tick
Done

Here is another similar repro in a single file:

import { mkdtemp, writeFile }   from 'node:fs/promises';
import { join }                 from 'node:path';
import { pathToFileURL }        from 'node:url';

const dir = await mkdtemp('test');
const file1 = join(dir, 'file1.cjs');
const file2 = join(dir, 'file2.cjs');
await Promise.all([writeFile(file1, ''), await writeFile(file2, '')]);

await import(pathToFileURL(file1));
process.nextTick(() => console.log('Tick'));
await import(pathToFileURL(file2));
console.log('Done');

The reason for the inverted order is this commit and a later change, which is causing getSource to be called even when the format of a module is already known to be commonjs, for example because of the .cjs extension.
getSource accesses the file system and that consistently requires one or more I/O cycles to complete. This allows callbacks scheduled with process.nextTick to run in the meantime.

Clearly, importing a CommonJS module is an asynchronous operation which souldn't be assumed to resolve in a specific order or number of phases, but calling getSource for known CommonJS modules isn't needed and should be avoided to not occupy the event loop unnecessarily.

Refs: eslint/eslint#17683

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[ljharb]

If the observable output is changing then that’s a breaking change, so it’s pretty important to fix.

[aduh95]

Thanks! A few comments regarding the tests:

[GeoffreyBooth]

I feel like the bug will just resurface whenever custom hooks are present and returning a source for a CommonJS file. At the very least this PR should add a test to prove that my worry is unfounded; but perhaps a better solution is to fetch the source within the ESM loader like we do for all other module types and the CJS loader somehow uses the already-retrieved source rather than attempt to load it again.

[fasttime]

perhaps a better solution is to fetch the source within the ESM loader like we do for all other module types and the CJS loader somehow uses the already-retrieved source rather than attempt to load it again.

Thanks @GeoffreyBooth, I've created a new branch with the change you suggested. I had to use a new internal property realSource to store the source fetched by the ESM loader in the result. If this property is set by a custom loader, it will be discarded along with other unknown properties, meaning that the public API isn't altered.

I like this change because it replaces a synchronous read operation with an asynchronous one. The downside is that there would be a discrepancy between defaultLoad and (existing) custom loader implementations. There seems to be also a concern that the observable behavior [in versions prior to 21.1.0] shouldn't change, so I can't decide which solution is better.

[aduh95]

We cannot use the source from the ESM loader, it would short cut all the monkey patching of the CJS loader.

[fasttime]

We cannot use the source from the ESM loader, it would short cut all the monkey patching of the CJS loader.

It doesn't if source stays null, that's why I had to use a new property.

[aduh95]

I think the actual fix would be to ask the main thread CJS loader for the source rather than calling getSource from the loader thread.

[aduh95]

That's a bummer because we are looking for ways to remove dependency on the monkey patchable CJS loader, but oh well. Or we accept the breaking change, and ask folks who rely on the monkey patchability of the CJS loader to switch to the new API (which mean we can't unflag until the ecosystem is ready)

[GeoffreyBooth]

ask folks who rely on the monkey patchability of the CJS loader

Because monkey patching is unsupported, there's no defined API surface. We don't really know what would or wouldn't break people.

In this case, I think the only impact would be people who monkey patch the CJS loader in order to affect import of CJS? Which is no one? I think we should just refactor as is best for our codebase and not worry about theoretical breaks.

[aduh95]

I think the only impact would be people who monkey patch the CJS loader in order to affect import of CJS? Which is no one?

Which is everyone you mean? I think Yarn PnP relies on that for example, or any project that uses pirates hooks I think.

[GeoffreyBooth]

No, that's relying on monkey patching to affect require, not import.

[aduh95]

No, that's relying on monkey patching to affect require, not import.

I don’t know what makes you think that, I think that’s not correct.

[GeoffreyBooth]

I don’t know what makes you think that, I think that’s not correct.

Can you provide an example where someone is monkey-patching the CommonJS loader to affect import of CommonJS?

Regardless, I don’t think it’s a supported use case. I don’t think monkey-patching at all is a supported use case, and monkey-patching the CommonJS loader to affect only CommonJS files in a mixed CommonJS-ESM codebase is even less supported: we provide the module hooks for that.

[aduh95]

Can you provide an example where someone is monkey-patching the CommonJS loader to affect import of CommonJS?

Do you mean examples other than the one I gave in #50465 (comment)? Could you clarify why you think those are not good examples?

we provide the module hooks for that

not really, if you provide a source with module hooks for commonjs, you have to use faux CJS, which not everyone can switch to overnight (e.g. a large portion of the ecosystem relies on require.cache to be a thing).

[ljharb]

(nb, also require.extensions)

[fasttime]

Since this PR is all about avoiding to read the same file twice with the ESM loader, maybe it would be better to continue the discussion about the CommonJS monkey-patching legacy in a separate issue to give it more visibility and solicit other perspectives. It looks like there is more need to speak about that.

Meanwhile, I'm happy to update and extend the unit tests as suggested so we can move on with this issue.

[fasttime]

I feel like the bug will just resurface whenever custom hooks are present and returning a source for a CommonJS file. At the very least this PR should add a test to prove that my worry is unfounded

@GeoffreyBooth I've added a unit test to ensure that the file specified in the URL is not used when a source is already present. I'm saying used because the actual read access is wrapped in a try-catch statement, so even if a read attempt occurred, it would not be possible to detect that situation by simulating a read error. Still, the ??= operator guarantees that the when fs.readFile is called, the result is stored in source, so checking the source should be enough to exclude a read operation. Let me know if you have a better idea. Here is the new test: c93377f. Please, have a look again.

[aduh95]

nit: since this does not in fact reference a fixtures, let's simplify.

Suggested change

	const { default: noSuchFileSource } = await import(fixtures.fileURL('no-such-file.cjs'));
	const { default: noSuchFileSource } = await import('./no-such-file.cjs');

[fasttime]

Thanks! Unfortunately, './no-such-file.cjs' wouldn't work because it's not a valid URL, and the resolve hook doesn't actually resolve relative paths because of the short-circuit. Anyway, a file URL like 'file:///no-such-file.cjs' works fine and is not relative (at least on Unix), so I switched to use that.

[aduh95]

I think it's incorrect to say it's not a valid URL, it is a valid (relative) URL.

Anyway, a file URL like 'file:///no-such-file.cjs' works fine

IMO it would make more sense to use a relative URL in the JS, and have the hook return a specific URL. That doesn't really matter anyway, for the purpose of this test the current version works, so good enough for me.

[fasttime]

Never mind, I had to change the test again because Windows doesn't like the absolute file URL without a drive letter: 4dcbec5. Let's see how the CI build goes this time.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/55501/

[nodejs-github-bot]

Landed in 2602c46