test_runner: mock_loader resolve the cjs and esm formats respectively.

[syi0808]

This issue was caused by recovering specifier in cjs resolve without format(esm,cjs) check in mock_loader.
So I did resolve logic differently for each format. And also added test.

Fixes: #53807

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[RedYetiDev]

Could you amend your commits to be test_runner rather than mock_loader? Thanks!

[syi0808]

Could you amend your commits to be test_runner rather than mock_loader? Thanks!

I will do that. Thank you!
And I think I need to get the format of the parent URL, but now I'm getting the format of the import specifier. I'll change it.
The parentURL has not been handed over to the context parameter, so i should include the modification of the parentURL along with the call of the rescue function.

[syi0808]

I changed to use specifier only when there is no parentURL.

[GeoffreyBooth]

This just got changed a few days ago in order to unflag detect-module: #53619. Please let that land first, then rebase and ensure that your changes still work when detection is enabled by default.

[GeoffreyBooth]

It feels wrong to me that the mock loader would be using module internals directly rather than the public API. This interdependence will make it harder to refactor modules stuff since non-public changes might break the test runner.

[syi0808]

As far as I understand, mock_loader is an internal module resolver when using module mock. So this has to decide the format internally, is there an public API that can solve it? I'm sorry if it's something I haven't found. Thanks!

[GeoffreyBooth]

You should be able to use the next hook in the chain, I'd think.

[aduh95]

As long as we're comfortable depending on internal things, we should skip the createRequire call (which is quite heavy) and call directly the CJS resolver.

[syi0808]

I wanted to figure out how not to call createRequire, but this would be difficult because the existing cjs filenameResolve relies on a thoroughly generated require.

#53846 (comment)
If we think of a direction that does not depend on the internal module, I think this problem can also be solved by creating a new filename resolve for cjs and esm.

[GeoffreyBooth]

@syi0808 If you can verify that this won't break with detect-module unflagged, I'll lift my block. I thought that could land this week but it looks like it'll be pushed to this weekend or next week and I don't want to keep you waiting.

@nodejs/test_runner Is there a way to refactor the mock loader to not depend on modules internals? I'd prefer not to keep having issues with modules updates breaking the mock loader or test runner updates necessitating changes in modules code. The current implementation has one small reference to modules internals that I bet could be refactored out, and I feel like this PR could solve the referenced issue without needing internals. If it can't currently be done without internals, I think that's perhaps a sign that we should create new modules public APIs, like import { defaultResolve } from 'node:module'. Then any test runner can benefit from such access, not just Node's internal one, and if our test runner limits itself to public APIs we avoid the refactoring issue.

[cjihrig]

If you think it can be refactored to not use the module internals, that would be great. My feeling from working on the mock loader is that we may need some other public API. I know during development I found it frustrating trying to normalize all of the different possible inputs (path, file URL, file URL string, core, CJS, ESM, relative, absolute, etc.) into something consistent.

[GeoffreyBooth]

. I know during development I found it frustrating trying to normalize all of the different possible inputs

Can we just have the user resolve before passing it in? Something like mock(new URL(import.meta.resolve('module-to-be-mocked'))).

[cjihrig]

That seems like a worse DX IMO since it punts the problem to users. It also seems like functionality that at least some customization hooks authors will run into. IIRC, defaultResolve() was close to enough but broke in the CI on Windows. If someone with a Windows setup can debug that, maybe it could be made to work (but is that a modules internal that you wouldn't want being used?).

[GeoffreyBooth]

is that a modules internal that you wouldn't want being used?).

It is currently, but we could expose it. I think it's very low risk because it's essentially already public via import.meta.resolve. That's better than any modules internals potentially being used in the mock loader.

[syi0808]

@syi0808 If you can verify that this won't break with detect-module unflagged, I'll lift my block. I thought that could land this week but it looks like it'll be pushed to this weekend or next week and I don't want to keep you waiting.

@nodejs/test_runner Is there a way to refactor the mock loader to not depend on modules internals? I'd prefer not to keep having issues with modules updates breaking the mock loader or test runner updates necessitating changes in modules code. The current implementation has one small reference to modules internals that I bet could be refactored out, and I feel like this PR could solve the referenced issue without needing internals. If it can't currently be done without internals, I think that's perhaps a sign that we should create new modules public APIs, like import { defaultResolve } from 'node:module'. Then any test runner can benefit from such access, not just Node's internal one, and if our test runner limits itself to public APIs we avoid the refactoring issue.

If there's no disagreement with the new API coming out for resolve, I think I can try to refactor this.

I will test the detect-module unflagged you mentioned after refactoring.

[GeoffreyBooth]

If there's no disagreement with the new API coming out for resolve, I think I can try to refactor this.

I think if we make the new API it should be its own PR. But before that, are we sure this PR needs it? You have access to defaultResolve (as next) within the resolve hook; can that do what you need?

[syi0808]

If there's no disagreement with the new API coming out for resolve, I think I can try to refactor this.

I think if we make the new API it should be its own PR. But before that, are we sure this PR needs it? You have access to defaultResolve (as next) within the resolve hook; can that do what you need?

I think this PR is necessary.

Basically, the purpose of fixing is as follows. Suppose you call a mock.module inside an ESM file. This will destroy the existing require behavior, regardless of which file format or package within the mock.module. This is because mock.module adds a hook, and inside the mock_loader, even though parentURL is in ESM format, it resolves like CJS.

The import_loader breaks this despite the incorrect import format because the mock_loader hands over the resolved url to the nextResolve.
However, the existing implementation does not follow the parentURL format, so it is difficult for CJS resolver to determine whether it has been properly resolved. It is also problematic to hand over the received specifier to the nextResolve.

// ESM
mock.module("./test.js")
import("./test")

In the currently implementation, we cannot prevent this behavior.

In order to resolve this, we have to rely on nextResolve, as you think inside the mock_loader, but the mock.module doesn't know what hooks will come before and after depending on the timing of the call.

The purpose of resolve the specifier in mock_loader is to match the URL that was locked in mock.js. If there is no custom hook after mock_loader in the ESM file, you can get the same effect as defaultResolve by calling nextResolve. If there is a custom hook after mock_loader, this may be unintended behavior, there is no guarantee that the saved filename will be the same as the mockSpecifier because the internal implementation of the custom resolve cannot be predicted.
Therefore, I think it is desirable to normalize the mock_loader internally according to the format of parentURL and call nextResolve if the locked module cannot be found based on it.

Additionally, assuming that the user doesn't know that the mock_loader writes registers internally, it may not be understood that the mock_loader can behave differently depending on the timing of the call. I think it would be nice to be able to pull up the hook of the mock_loader to be the first to run.

[GeoffreyBooth]

Is the mock loader trying to define a mock for when a specifier is required, imported, or both? Because of exports conditions, the same specifier might resolve as different things when it's imported or when it's required. I think it's hazard prone to try to guess which module system the user wants the mock for; I know it's less convenient, but requiring the user to pass in a URL instance would eliminate the ambiguity.

[syi0808]

I also don't want the user to guess the module system.

I just didn't want the existing import behavior to be broken just by mocking the module.

To be more specific, call CJS filenameResolve to normalize the specifier, find the mocking module, and if can't find it, if you pass the specifier taken over by parameter to nextResolve, the case that corresponds to the issue will be covered.

// ESM
mock.module("./test.js")
import("./test")

But this case is still not covered.

In conclusion, this pr is just exception handling. As you said, it would be nice to hand over this exception to the user, but if they use mock.module, they should pay attention to all import syntax.

[GeoffreyBooth]

Can I open a follow up PR about creating a new API and refactoring mock_loader? Or is this the core team's job?

You're volunteering PRs so that makes you part of the core team 😄

If it's possible to achieve the fix you're aiming for in this PR without using modules internals, I would prefer such a solution. If not, it's better to fix a bug than leave it broken, and we can follow up with how to address the issues related to the mock loader.

[syi0808]

If it's possible to achieve the fix you're aiming for in this PR without using modules internals, I would prefer such a solution. If not, it's better to fix a bug than leave it broken, and we can follow up with how to address the issues related to the mock loader.

Sorry, I can't think of a way for the mock_loader to not dependent on the cjs and module format resolve method.

Or shall we wait for this to find a better idea?

[syi0808]

I think the time has come to propose a new API that is merge before this is left unattended.

I'm going to create a resolve function that integrates CJS and ESM. What do you think? @GeoffreyBooth

[aduh95]

I have another approach that would simplify the loader code greatly IMO, but let's land this first, as it's easier to iterate on a follow-up PR than for me to make a very large suggestion in this PR

[nodejs-github-bot]

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

[syi0808]

I don't know if the failed CI is related to this PR.

[nodejs-github-bot]

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

[aduh95]

Landed in 5bfebfe

[syi0808]

I have another approach that would simplify the loader code greatly IMO, but let's land this first, as it's easier to iterate on a follow-up PR than for me to make a very large suggestion in this PR

Are you going to open a follow-up PR for this?