Skip to content
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.

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

esm: handle extension-less "bin" files #41552

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/get_format.js
Expand Up @@ -74,7 +74,7 @@ function getLegacyExtensionFormat(ext) {

function getFileProtocolModuleFormat(url, ignoreErrors) {
const ext = extname(url.pathname);
if (ext === '.js') {
if (ext === '.js' || !ext) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has a broader impact than just "bin" files.

I could artificially limit it with something like the parentDir check I put together as a quick-fix to work around this issue for my own projects, so that it affects only "bin" files. I'm not sure whether the extra complexity would be worthwhile.

Is there a particular reason other extension-less files should specifically be excluded?

Copy link
Member

Choose a reason for hiding this comment

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

What about if extensionless files are wasm in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exclusively or additionally?

Copy link
Member

Choose a reason for hiding this comment

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

Exclusive isn't possible; it'd have to be additionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When more than 2 options are possible, I would probably just convert the logic to some kind of Map or Set.

return getPackageType(url) === 'module' ? 'module' : 'commonjs';
}

Expand Down
26 changes: 16 additions & 10 deletions lib/internal/modules/run_main.js
Expand Up @@ -28,20 +28,26 @@ function resolveMainPath(main) {
}

function shouldUseESMLoader(mainPath) {
// General indicators of user intention

const userLoader = getOptionValue('--experimental-loader');
if (userLoader)
return true;
// This flag is a user's only way to opt into ESMLoader (which can handle both
// CJS & ESM), so assume that's what is happening here.
if (userLoader) return true;

const esModuleSpecifierResolution =
getOptionValue('--experimental-specifier-resolution');
if (esModuleSpecifierResolution === 'node')
return true;
// Determine the module format of the main
if (mainPath && StringPrototypeEndsWith(mainPath, '.mjs'))
return true;
if (!mainPath || StringPrototypeEndsWith(mainPath, '.cjs'))
return false;
// This flag is only applicable to ESM, so assume it signals opting in.
if (esModuleSpecifierResolution === 'node') return true;

// Specific indicators of user intention

// Check trump-cards
if (mainPath && StringPrototypeEndsWith(mainPath, '.mjs')) return true;
if (!mainPath || StringPrototypeEndsWith(mainPath, '.cjs')) return false;

const pkg = readPackageScope(mainPath);
return pkg && pkg.data.type === 'module';
return pkg?.data?.type === 'module';
}

function runMainESM(mainPath) {
Expand Down
21 changes: 18 additions & 3 deletions test/es-module/test-esm-resolve-type.js
Expand Up @@ -38,13 +38,28 @@ try {
[
[ '/es-modules/package-type-module/index.js', 'module' ],
[ '/es-modules/package-type-commonjs/index.js', 'commonjs' ],
// "commonjs" is the default type, so it is assumed when there is no "type"
// or specific file extension (`.cjs` or `.mjs`)
// It happens to be "correct" here:
[ '/es-modules/packages-with-bin/no-type-but-commonjs/bin/foo', 'commonjs'],
// It happens to be "wrong" here:
[ '/es-modules/packages-with-bin/no-type-but-module/bin/foo', 'commonjs' ],
[ '/es-modules/packages-with-bin/type-commonjs/bin/foo', 'commonjs' ],
[ '/es-modules/packages-with-bin/type-module/bin/foo', 'module' ],
[ '/es-modules/package-without-type/index.js', 'commonjs' ],
[ '/es-modules/package-without-pjson/index.js', 'commonjs' ],
].forEach((testVariant) => {
const [ testScript, expectedType ] = testVariant;
].forEach(([ testScript, expectedType ]) => {
const resolvedPath = path.resolve(fixtures.path(testScript));
const resolveResult = resolve(url.pathToFileURL(resolvedPath));
assert.strictEqual(resolveResult.format, expectedType);
assert.strictEqual(
resolveResult.format,
expectedType,
new assert.AssertionError({
actual: resolveResult?.format,
expected: expectedType,
message: `Expectation failed for "${testScript}"`
})
);
});

/**
Expand Down
8 changes: 3 additions & 5 deletions test/es-module/test-esm-unknown-or-no-extension.js
Expand Up @@ -5,13 +5,11 @@ const fixtures = require('../common/fixtures');
const { spawn } = require('child_process');
const assert = require('assert');

// In a "type": "module" package scope, files with unknown extensions or no
// extensions should throw; both when used as a main entry point and also when
// referenced via `import`.
// In a "type": "module" package scope, files with unknown extensions should
// throw; both when used as a main entry point and also when referenced via
// `import`.

[
'/es-modules/package-type-module/noext-esm',
'/es-modules/package-type-module/imports-noext.mjs',
'/es-modules/package-type-module/extension.unknown',
'/es-modules/package-type-module/imports-unknownext.mjs',
].forEach((fixturePath) => {
Expand Down
@@ -0,0 +1 @@
require('assert');
@@ -0,0 +1,4 @@
{
"bin": "./bin/foo",
"name": "bin-without-type"
}
@@ -0,0 +1 @@
import 'assert';
@@ -0,0 +1,4 @@
{
"bin": "./bin/foo",
"name": "bin-without-type"
}
@@ -0,0 +1 @@
require('assert');
@@ -0,0 +1,5 @@
{
"bin": "./bin/foo",
"name": "bin-without-type",
"type": "commonjs"
}
@@ -0,0 +1 @@
import 'assert';
@@ -0,0 +1,5 @@
{
"bin": "./bin/foo",
"name": "bin-without-type",
"type": "module"
}