Skip to content

Commit

Permalink
module: warn on detection in typeless package
Browse files Browse the repository at this point in the history
PR-URL: #52168
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
GeoffreyBooth authored and targos committed Sep 21, 2024
1 parent d53e536 commit 90b632e
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 3 deletions.
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/esm/resolve');
const { getPackageScopeConfig, getPackageType } = require('internal/modules/esm/resolve');
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');

0 comments on commit 90b632e

Please sign in to comment.