module: fix extensionless CJS files in type:module packages

[mcollina]

Summary

• PR module: fix extensionless entry with explicit type=commonjs #61600 made the CJS loader respect package.json type field for extensionless files, but also enforced type: "module" which breaks CJS extensionless files inside ESM packages (e.g. yargs v17)
• Only enforce type: "commonjs" for extensionless files; for type: "module", leave format undefined so V8's syntax detection determines the correct format
• Adds regression test that require()s a CJS extensionless file from a type: "module" package and verifies exports are correct

Test plan

• Existing test test/es-module/test-extensionless-esm-type-commonjs.js still passes (ESM syntax in type: "commonjs" still throws SyntaxError)
• New test case verifies CJS extensionless file in type: "module" package returns correct exports via require()
• Manual verification: require() of fixture returns { hello: 'world' } (was returning empty ESM namespace before fix)

Fixes: #61971
Refs: yargs/yargs#2509
Refs: #61600

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.65%. Comparing base (e78bf55) to head (519235b).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #62083   +/-   ##
=======================================
  Coverage   89.64%   89.65%           
=======================================
  Files         676      676           
  Lines      206249   206326   +77     
  Branches    39518    39527    +9     
=======================================
+ Hits       184892   184980   +88     
+ Misses      13479    13471    -8     
+ Partials     7878     7875    -3     

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	98.14% <100.00%> (-0.22%)	⬇️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[joyeecheung]

Code LGTM with a correction on comment

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[GeoffreyBooth]

• PR module: fix extensionless entry with explicit type=commonjs #61600 made the CJS loader respect package.json type field for extensionless files, but also enforced type: "module" which breaks CJS extensionless files inside ESM packages (e.g. yargs v17)
• Only enforce type: "commonjs" for extensionless files; for type: "module", leave format undefined so V8's syntax detection determines the correct format

I think this proposed behavior is incorrect; according to our spec, extensionless files within a package scope with an explicit type field follow the format of the type field—that is, they behave however .js files would behave. We never do syntax detection when there's an explicit type field.

I read yargs/yargs#2509 and it's unfortunate that they seemed to rely on what was buggy behavior. But this PR seems to make several breaking changes, going against our own documented behaviors for how the type field works and how syntax detection works. I'm not sure what to do instead; perhaps just revert #61600 and re-land it as semver-major?

Marking as request changes so that we discuss this before landing. If others disagree with my analysis here I'm happy to be persuaded I'm wrong; I understand there's probably an eagerness to unbreak a popular library but let's not be so hasty that we cause other bugs.

[aduh95]

this PR seems to make several breaking changes

Can you list them? I don't see what would be breaking about it

[ljharb]

fwiw this seems right to me; i remain surprised that extensionless ESM was ever possible, and in all the module group discussions, i thought we concluded it basically wouldn't ever be (unless the "type" default changed)

[joyeecheung]

This is not recent, it has been possible since v20, so it would be too late to do anything about it.

[mcollina]

@GeoffreyBooth I didn't put much thought into the spec for this. My main concern was to fix this without having to revert that commit. I'm happy either way.

The alternative would be to revert that commit from 25.x and land it after the problematic behavior is fixed in the ecosystem.

We should be doing something soon about this.

[joyeecheung]

according to our spec, extensionless files within a package scope with an explicit type field follow the format of the type field—that is, they behave however .js files would behave. We never do syntax detection when there's an explicit type field.

This is a case where we have contradicting documentations: while in the esm part of the documentation it says the module format always respect the type field for extension files, in the cjs part of the documentation it has been including this exception that's not present in the spec, as far back as v16 from #41383 - so yargs is technically following a documented exception, the problem is more on the Node.js side that it has conflicting documentation.

when the nearest parent package.json file contains a top-level field "type" with a value of "module", those files will be recognized as CommonJS modules only if they are being included via require(), not when used as the command-line entry point of the program

#61600 made the extensionless behavior match what's in the ESM spec (so reverting it is widening this exception to ESM, not eliminating it) without counting this exception in the CJS documentation, and this PR only adds that CJS exception back. I agree the saner behavior would be to respect type field always (this is the status quo after #61600 fixed the mismatch), and remove this exception from the CJS documentation, but that should be a semver-major. For existing releases, the best we can do IMO is to combine #61600 and this PR to reduce the exception surface without introducing a regression.

[guybedford]

Why would type: commonjs "silently delegate to ESM"? Surely that should be a loud syntax detection case with a warning?

[joyeecheung]

Surely that should be a loud syntax detection case with a warning?

I think that might be missed in the syntax detection implementation to add a warning in this path, so it never emitted a warning in this case, and the comment was just describing the previous behavior - it should probably be removed because we generally don't talk about "what a previous bug looked like" in comments, only in commit logs.

Suggested change

	// Extensionless files skip the .js suffix check above. When type is commonjs,
	// follow it so ESM syntax surfaces as SyntaxError instead of silently
	// delegating to ESM. For type: module, leave format undefined so our syntax
	// Extensionless files skip the .js suffix check above. When type is commonjs, follow it so ESM
	// syntax surfaces as SyntaxError. For type: module, leave format undefined so our syntax

[guybedford]

Would it be possible to have extensionless files entirely ignore the package.json "type"? That seems like a more pragmatic approach given that they are designed to exist outside of package boundaries in most "binary execution" use cases.