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

module: warn on detection in typeless package #52168

Merged
merged 1 commit into from
Mar 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 19 additions & 3 deletions lib/internal/modules/esm/get_format.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const {
ObjectPrototypeHasOwnProperty,
PromisePrototypeThen,
PromiseResolve,
SafeSet,
StringPrototypeIncludes,
StringPrototypeCharCodeAt,
StringPrototypeSlice,
Expand All @@ -19,7 +20,7 @@ const {
const experimentalNetworkImports =
getOptionValue('--experimental-network-imports');
const { containsModuleSyntax } = internalBinding('contextify');
const { getPackageType } = require('internal/modules/package_json_reader');
const { getPackageScopeConfig, getPackageType } = require('internal/modules/package_json_reader');
const { fileURLToPath } = require('internal/url');
const { ERR_UNKNOWN_FILE_EXTENSION } = require('internal/errors').codes;

Expand Down Expand Up @@ -81,6 +82,7 @@ function underNodeModules(url) {
return StringPrototypeIncludes(url.pathname, '/node_modules/');
}

let typelessPackageJsonFilesWarnedAbout;
/**
* @param {URL} url
* @param {{parentURL: string; source?: Buffer}} context
Expand All @@ -92,7 +94,7 @@ function getFileProtocolModuleFormat(url, context = { __proto__: null }, ignoreE
const ext = extname(url);

if (ext === '.js') {
const packageType = getPackageType(url);
const { type: packageType, pjsonPath } = getPackageScopeConfig(url);
if (packageType !== 'none') {
return packageType;
}
Expand All @@ -111,9 +113,23 @@ function getFileProtocolModuleFormat(url, context = { __proto__: null }, ignoreE
// `source` is undefined when this is called from `defaultResolve`;
// but this gets called again from `defaultLoad`/`defaultLoadSync`.
if (getOptionValue('--experimental-detect-module')) {
return source ?
const format = source ?
(containsModuleSyntax(`${source}`, fileURLToPath(url)) ? 'module' : 'commonjs') :
null;
if (format === 'module') {
// This module has a .js extension, a package.json with no `type` field, and ESM syntax.
// Warn about the missing `type` field so that the user can avoid the performance penalty of detection.
typelessPackageJsonFilesWarnedAbout ??= new SafeSet();
if (!typelessPackageJsonFilesWarnedAbout.has(pjsonPath)) {
const warning = `${url} parsed as an ES module because module syntax was detected;` +
` to avoid the performance penalty of syntax detection, add "type": "module" to ${pjsonPath}`;
process.emitWarning(warning, {
code: 'MODULE_TYPELESS_PACKAGE_JSON',
});
typelessPackageJsonFilesWarnedAbout.add(pjsonPath);
}
}
return format;
}
return 'commonjs';
}
Expand Down
45 changes: 45 additions & 0 deletions test/es-module/test-esm-detect-ambiguous.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
]) {
it(testName, async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--no-warnings',
'--experimental-detect-module',
entryPath,
]);
Expand Down Expand Up @@ -142,6 +143,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
]) {
it(testName, async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--no-warnings',
'--experimental-detect-module',
entryPath,
]);
Expand Down Expand Up @@ -291,6 +293,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {

it('permits declaration of CommonJS module variables', async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--no-warnings',
'--experimental-detect-module',
fixtures.path('es-modules/package-without-type/commonjs-wrapper-variables.js'),
]);
Expand Down Expand Up @@ -327,6 +330,48 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
strictEqual(signal, null);
});
});

describe('warn about typeless packages for .js files with ESM syntax', { concurrency: true }, () => {
for (const { testName, entryPath } of [
{
testName: 'warns for ESM syntax in a .js entry point in a typeless package',
entryPath: fixtures.path('es-modules/package-without-type/module.js'),
},
{
testName: 'warns for ESM syntax in a .js file imported by a CommonJS entry point in a typeless package',
entryPath: fixtures.path('es-modules/package-without-type/imports-esm.js'),
},
{
testName: 'warns for ESM syntax in a .js file imported by an ESM entry point in a typeless package',
entryPath: fixtures.path('es-modules/package-without-type/imports-esm.mjs'),
},
]) {
it(testName, async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--experimental-detect-module',
entryPath,
]);

match(stderr, /MODULE_TYPELESS_PACKAGE_JSON/);
strictEqual(stdout, 'executed\n');
strictEqual(code, 0);
strictEqual(signal, null);
});
}

it('warns only once for a package.json that affects multiple files', async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--experimental-detect-module',
fixtures.path('es-modules/package-without-type/detected-as-esm.js'),
]);

match(stderr, /MODULE_TYPELESS_PACKAGE_JSON/);
strictEqual(stderr.match(/MODULE_TYPELESS_PACKAGE_JSON/g).length, 1);
strictEqual(stdout, 'executed\nexecuted\n');
strictEqual(code, 0);
strictEqual(signal, null);
});
});
});

// Validate temporarily disabling `--abort-on-uncaught-exception`
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import './module.js';
console.log('executed');
Loading