GeezerVison

Review requested:

• @nodejs/loaders

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

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.

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.

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

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.

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.

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)

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.

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.

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

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.

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.

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

(nb, also require.extensions)

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.

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.

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

Landed in 2602c46