module: prevent format from being set to null internaly

[avivkeller]

With all the changes and backtraces my PR's have caused regarding changes similar to this, @GeoffreyBooth and I agreed that preventing format from being set to null internaly would at least be a step in the right direction.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[GeoffreyBooth]

The reason for this is to simplify tests. The format value might be null or it might be undefined based on arbitrary things like which code path is followed. Our internal code is careful to do loose tests like format == null to account for format being either null or undefined, but not all the tests are so loose. Eliminating null from the potential possibilities means that our tests can remain precise while not breaking as we refactor code paths. The internal conditions will remain loose so that user hooks can still set format: null.

[GeoffreyBooth]

https://github.com/nodejs/node/blob/main/doc/api/module.md needs updating too, the various lines like this:

  * `format` {string|null|undefined} The format optionally supplied by the
    `resolve` hook chain

To remove null from the list.

[aduh95]

IIUC, before this change, defaultResolve would set format to null when it can't find a better value (and load would still accept undefined nonetheless in case that's what a user hook returned). With this change, it will now be undefined (and load would still accept null nonetheless in case that's what a user hook returned). How does that make tests more precise? It seems to me it's just switching a value for another, that seems rather pointless.

[dygabo]

I accidentally found this PR and looked a bit into the details (might still miss something). But iiuc the goal of the module detection is to allow .js files to be either CJS or ESM as opposed to current behaviour without the --experimental-detect-module that is .js are assumed to be CJS if not otherwise specified with --experimental-default-type command line option. Is this correct?

I can have a look and open a PR if I find a solution. I agree that null or undefined have equivalent explanatory value.

cc: @aduh95, @GeoffreyBooth

[avivkeller]

I accidentally found this PR and looked a bit into the details (might still miss something). But iiuc the goal of the module detection is to allow .js files to be either CJS or ESM as opposed to current behaviour without the --experimental-detect-module that is .js are assumed to be CJS if not otherwise specified with --experimental-default-type command line option. Is this correct?

#53016 has some more information, but essentially, in the end, we want it to default to CJS, but it defaults to null / undefined

[nodejs-github-bot]

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

[aduh95]

I disagree this simplifies anything, let's not land this – it'd be a small breaking change, but still a breaking change, and we should have some justification for breaking things

[avivkeller]

I was actually planning on closing this favor of another PR (I need to find it then I'll link it)