test: update ESM loader hooks to allow ambiguous format

[avivkeller]

Pull request #50314 added support for a null format value in addition to the existing commonjs or module. However, the test for ESM hooks relied on the default value of commonjs, causing it to fail when run with the --experimental-detect-module flag. This PR enables the test to detect null formats, ensuring it runs successfully with experimental detection enabled.

Sidenote: While @GeoffreyBooth and I were debugging this behavior, the single-character function arguments made it very difficult to understand, so this PR also expands them into the full names.

[GeoffreyBooth]

But is there a purpose behind Node’s internal resolve hook sometimes returning format: null as opposed to the key being missing? If not, maybe our internal hooks should always either set format if it has a truthy value, or don’t define it at all (or set format: undefined) and leave null out of it.

[GeoffreyBooth]

@nodejs/loaders

[avivkeller]

But is there a purpose behind Node’s internal resolve hook sometimes returning format: null as opposed to the key being missing? If not, maybe our internal hooks should always either set format if it has a truthy value, or don’t define it at all (or set format: undefined) and leave null out of it.

I don't think there is a purpose of setting it to null rather than undefined, but IIRC @aduh95 added the change, so maybe he has a reason?

That being said, in this case, the null vs undefined could be seen asa "was the format even checked?":
Yes, the resolver tried (null)
No, the resolver didn't even check (undefined)

[GeoffreyBooth]

Unless there’s a good reason we need null, I think we should first land this and then reopen #52988 and update it so that format is either a string or undefined (at least as set by Node internals).

That being said, in this case, the null vs undefined could be seen asa “was the format even checked?”:

I’m not sure that this is a good reason. If this is data that we want to preserve, it could be its own property on context, like formatWasChecked: true, and therefore it would be explicit and not mysterious.

[JakobJingleheimer]

IIR, the historical reason (advocated by Antoine) was that null vs undefined was immaterial/unimportant and the historical precedent was nil.

Splitting hairs: undefined means "idk" and null means "empty" (where empty is "i explicitly acknowledged this exists and there is nothing here" whereas undefined means 🤷‍♂️ or "ignore me")

[GeoffreyBooth]

In this case, the different is very material—some tests fail with null but not with undefined. This is a footgun for hooks authors, to need to handle an unexpected extra potential return value. Unless we need null for some reason, I think we should toss it and stick to just strings and undefined.

[avivkeller]

Just for sake of discussion, what value would be put in place for an unknown format module?

(But maybe this is a discussion for another time)

[GeoffreyBooth]

Just for sake of discussion, what value would be put in place for an unknown format module?

Without custom hooks registered, Node throws on unknown formats.

[aduh95]

Changing the test should happen in the same commit that changes the code

[aduh95]

You can use this PR to land the variable renaming if you want

[avivkeller]

Changing the test should happen in the same commit that changes the code

Unlike the other test changes for --experimental-detect-module, I think this one deserves its own PR, as IMO it should have been updated in #50314 to account for the new return value (but there was really no way to tell that this would happen).

WDYT @GeoffreyBooth

[GeoffreyBooth]

@redyetidev I think reopen #52988 and have it undo the part of #50314 that returned null instead of undefined; and update the tests accordingly. Unless @aduh95 can point to a reason that sometimes returning null is desirable, it seems to me that the returning of null was an unintended change in #50314 that had no documentation or explanation, and we should just undo that part.

[avivkeller]

@redyetidev I think reopen #52988 and have it undo the part of #50314 that returned null instead of undefined; and update the tests accordingly. Unless @aduh95 can point to a reason that sometimes returning null is desirable, it seems to me that the returning of null was an unintended change in #50314 that had no documentation or explanation, and we should just undo that part.

Sure, it'll take some work because it seems that certain infrastructure rely on that behavior, but I'll work on it