modules: throw an error for invalid package.json main entries

[BridgeAR]

We currently ignore invalid main entries in package.json files.
This does not seem to be very user friendly as it's certainly an
error if the main entry is not a valid file name. So instead of
trying to resolve the file otherwise, throw an error immediately to
improve the user experience.

Fixes: #26588

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2990/pipeline

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/21691/
CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1770/

[guybedford]

I think we need to remain consistent with the existing not found error code.

[guybedford]

Thanks for changing the error.

The only other concern with this PR would be the chance of breaking scenarios - say if users are relying on the main not found causing a fallback in the in the main _findPath algorithm.

For example, consider the following folder structure

app/node_modules/pkg/package.json = { "main": "./does-not-exist" }
node_modules/pkg/package.json = { "main": "./does-exist.js" }

currently I think the lookup algorithm for require('pkg') in app/ will actually load node_modules/pkg/does-exist.js just fine (and this is just from scanning the code, do correct me if I'm wrong). So that is, there could well be working code that relies on this fallback.

errors were aligned

[BridgeAR]

@guybedford that observation is correct. The code would theoretically run but it does seem like the expectation is that the other module would be loaded instead. So it does seem to be a clear user mistake. If someone provides an invalid main entry, we should notify the user about the mistake in what ever code that happens in.

CITGM actually revealed one failure: https://github.com/dominictarr/assertions seems to have an invalid main entry but it has an index.js in the directory which is resolved instead (I am going to open a PR to fix that).

I could add a runtime deprecation for cases that resolve with the index.js file being in the same directory (that's why I marked it as WIP, I wanted to give it some more thought) or should we throw the error right away? I guess having a transition time would be good. I do not fear files being able to resolve in other node_modules directories and more about this case.

@nodejs/modules PTAL.

[guybedford]

@BridgeAR I didn't realise this code path applied separately to the index.js directory check for packages. If that is the case then I don't think this can land, as there will likely be many more user packages with invalid package.json mains, that have valid index.js files that are being loaded.

[BridgeAR]

@guybedford I'll change it to a deprecation warning in case the main entry is invalid but there is a index.js file in the same directory as well.

[guybedford]

That seems sensible to me.

[guybedford]

One thing that could be quite frustrating is, as per usual, if the warning is in a dependency the user doesn't necessarily have control over.

Perhaps we should docs-only deprecate first as per policy?

Which makes me wish there could be a more gradual way to introduce warnings... say a node --latest-warnings or similar, where only those users interested in being ahead can do the checks, then get the fixes published to the third-party packages for everyone else before they become annoyance.

[targos]

Which makes me wish there could be a more gradual way to introduce warnings...

We have the --pending-deprecation flag.

[BridgeAR]

I updated the PR to keep on working in case the index.js file is in the same directory as the package.json.

Now, only completely broken modules throw the error (as in: no file could be found what so ever). On top of that I added a pending deprecation in case the main entry is invalid.

[guybedford]

Seems like a great approach.

[richardlau]

This changes the high-level algorithm described in https://github.com/nodejs/node/blob/master/doc/api/modules.md#all-together. Please could you update that to match?

[BridgeAR]

@richardlau we did not document potential parsing errors so far either. Shall I include that as well?

[BridgeAR]

PTAL. I updated the documentation accordingly. It now includes the steps that we took but that were never documented so far.

[nodejs-github-bot]

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

[BridgeAR]

Resumed CI for a yellow build: https://ci.nodejs.org/job/node-test-pull-request/21870/ ✅ (yellow build with ARM failing)
Rebuild ARM https://ci.nodejs.org/job/node-test-commit-arm-fanned/7560/ ✔️

CITGM looks good to me.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/21926/ (yellow build with ARM failing)

[mcollina]

LGTM

[BridgeAR]

Thanks a lot for the reviews.

Landed in 115f0f5 🎉