Skip to content

[25.x] module: fix extensionless CJS files in type:module packages #62083

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mcollina wants to merge 1 commit into
base: v25.x-staging
Choose a base branch
from
+18 −4
Open

[25.x] module: fix extensionless CJS files in type:module packages #62083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1952,12 +1952,12 @@ Module._extensions['.js'] = function(module, filename) {
format = 'typescript';
}
} else if (path.extname(filename) === '') {
// Extensionless files skip the .js suffix check above. When type is explicit,
// follow it so ESM syntax surfaces as SyntaxError for commonjs instead of
// silently delegating to ESM.
// 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
// detection handles it (allowing CJS extensionless files in ESM packages).
Comment on lines +1956 to +1957
Copy link
Member

@GeoffreyBooth GeoffreyBooth Mar 9, 2026

Choose a reason for hiding this comment

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

Does our syntax detection handle it? I think to confirm that it does, this PR also needs a test that requiring an extensionless file with ESM syntax in a type: module package scope also works.

This is also the part that worries me, though: according to our docs, syntax detection happens for

Files with a .js extension or no extension; and either no controlling package.json file or one that lacks a type field.

Assuming the comment here does what it claims to do, then this PR is creating a new exception to our docs: syntax detection applies in the above case, and for the string input, and for extensionless files in a type: module context. I thought we wanted syntax detection to never apply when a type field was set; that's what the warning we print tells users to add to avoid the performance hit of syntax detection. That's why this PR in general feels like the wrong solution to me.

Alternatively if this comment is just wrong and there's no syntax detection involved here, then we should fix this comment (and the PR description).

Copy link
Member

@joyeecheung joyeecheung Mar 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

this PR is creating a new exception to our docs: syntax detection applies in the above case, and for the string input, and for extensionless files in a type: module context.

I think this needs to factor in that it's not a new exception, it's still the status quo for v20-v24. #61600 removed the exceptions on v25, and this PR is only partially putting a small part of the exception back to prevent regressions on v25, since that part is documented (but not the others). In v26 the exception will be fully disallowed since it'll have #61600 in full.

Alternatively, if we want to forbid syntax detection entirely, to make it strictly follow the documented exception it should change to if (typeFromPjson === 'module') format = 'commonjs', but that feels even more wrong and it can only happen on v25....syntax detection is mostly just a mechanics to relax the interpretation of the type field to be less wrong, instead of completely going the opposite of what type field says (which is what got documented) and forbid extensionless ESM when there's type: module (meanwhile, without type in package.json it still allows extensionless ESM since 20).

Copy link
Member

@GeoffreyBooth GeoffreyBooth Mar 9, 2026

Choose a reason for hiding this comment

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

If we're currently doing any syntax detection for files where the controlling package.json file has a type field, that's a bug. That's not documented in the syntax detection section, nor in https://nodejs.org/docs/v25.7.0/api/modules.html#enabling. The type field is supposed to prevent syntax detection, full stop, for all files including extensionless ones. That's why we print this:

node/lib/internal/modules/esm/get_format.js

Lines 148 to 150 in 17c76c1

const warning = `Module type of ${url} is not specified and it doesn't parse as CommonJS.\n` +
'Reparsing as ES module because module syntax was detected. This incurs a performance overhead.\n' +
`To eliminate this warning, add "type": "module" to ${pjsonPath}.`;

So I'm not sure what exception you're referring to? Are the docs and this warning all wrong and we've been secretly doing syntax detection for extensionless files in type: module scopes prior to #61600?

Copy link
Member

@joyeecheung joyeecheung Mar 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Are the docs and this warning all wrong and we've been secretly doing syntax detection for extensionless files in type: module scopes prior to #61600?

Yes, or more precisely, we had been ignoring the type field for required extensionless files before 61600 - and still do on v20-v24. We didn't even read the package.json's type field at all for required extensionless files until 61600. That's why I say reverting it is going to make it more wrong.

As it turned out, if the type field is module, treating the required extensionless file as ESM caused a regression from the documented exception, so we have two choices to fix it for 25.

  1. If the type field is module, treat the required file as commonjs (this sounds even weirder, but it's what the doc says, and what yargs 17 expects). Note that this could still be breaking because it then disallows required extensionless ESM even with type: module, which has been available on v20-24.
  2. If type field is module, ignore the type field (what this PR does), so required extensionless files under this package.json can be either commonjs (what yargs 17 needs) or ESM (less weird for other cases, and what actually respecting the type field would've allowed), which is also the behavior we have for v20-v24.

Note that this is unrelated to entry points, and only for required extensionless files.

pkg = packageJsonReader.getNearestParentPackageJSON(filename);
const typeFromPjson = pkg?.data?.type;
if (typeFromPjson === 'commonjs' || typeFromPjson === 'module') {
if (typeFromPjson === 'commonjs') {
format = typeFromPjson;
}
Comment on lines +1960 to 1962
Copy link
Contributor

@guybedford guybedford Mar 4, 2026

Choose a reason for hiding this comment

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

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.

}
Expand Down
10 changes: 10 additions & 0 deletions test/es-module/test-extensionless-esm-type-commonjs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,13 @@ spawnSyncAndAssert(process.execPath, [
stdout: /script STARTED[\s\S]*v\d+\./,
trim: true,
});

// CJS extensionless file inside a type: "module" package should work
// when require()'d. Regression test for https://github.com/nodejs/node/issues/61971
spawnSyncAndAssert(process.execPath, [
'-e', `const m = require(${JSON.stringify(
fixtures.path('es-modules', 'extensionless-cjs-module', 'index')
)}); if (m.hello !== 'world') throw new Error('expected CJS exports, got: ' + JSON.stringify(m))`,
], {
status: 0,
});
1 change: 1 addition & 0 deletions test/fixtures/es-modules/extensionless-cjs-module/index
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = { hello: 'world' };
3 changes: 3 additions & 0 deletions test/fixtures/es-modules/extensionless-cjs-module/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"type": "module"
}