module: prevent `format` from being set to `null` internaly by avivkeller · Pull Request #53015

module: prevent `format` from being set to `null` internaly by avivkeller · Pull Request #53015 · nodejs/node

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.

Review requested:

@nodejs/loaders

avivkeller changed the title ~~module: prevent format from being null~~ module: prevent format from being set to null internaly

May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

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

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

H4ad added request-ci

Add this label to start a Jenkins CI on a PR.

commit-queue-squash

Add this label to instruct the Commit Queue to squash all the PR commits into the first one.

labels

Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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