Skip to content

Commit 1e7d8be

Browse files
GeoffreyBoothjoyeecheung
authored andcommitted
module: eliminate performance cost of detection for cjs entry
PR-URL: nodejs#52093 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Richard Lau <[email protected]>
1 parent 97291c2 commit 1e7d8be

File tree

6 files changed

+193
-96
lines changed

6 files changed

+193
-96
lines changed

benchmark/misc/startup-core.js

+1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const bench = common.createBenchmark(main, {
99
script: [
1010
'benchmark/fixtures/require-builtins',
1111
'test/fixtures/semicolon',
12+
'test/fixtures/snapshot/typescript',
1213
],
1314
mode: ['process', 'worker'],
1415
n: [30],

lib/internal/modules/cjs/loader.js

+10-2
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ module.exports = {
106106
kModuleExportNames,
107107
kModuleCircularVisited,
108108
initializeCJS,
109+
entryPointSource: undefined, // Set below.
109110
Module,
110111
wrapSafe,
111112
kIsMainSymbol,
@@ -1392,8 +1393,15 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) {
13921393
return result;
13931394
} catch (err) {
13941395
if (process.mainModule === cjsModuleInstance) {
1395-
const { enrichCJSError } = require('internal/modules/esm/translators');
1396-
enrichCJSError(err, content, filename);
1396+
if (getOptionValue('--experimental-detect-module')) {
1397+
// For the main entry point, cache the source to potentially retry as ESM.
1398+
module.exports.entryPointSource = content;
1399+
} else {
1400+
// We only enrich the error (print a warning) if we're sure we're going to for-sure throw it; so if we're
1401+
// retrying as ESM, wait until we know whether we're going to retry before calling `enrichCJSError`.
1402+
const { enrichCJSError } = require('internal/modules/esm/translators');
1403+
enrichCJSError(err, content, filename);
1404+
}
13971405
}
13981406
throw err;
13991407
}

lib/internal/modules/helpers.js

+35
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,15 @@ const {
1919
} = require('internal/errors').codes;
2020
const { BuiltinModule } = require('internal/bootstrap/realm');
2121

22+
const {
23+
shouldRetryAsESM: contextifyShouldRetryAsESM,
24+
constants: {
25+
syntaxDetectionErrors: {
26+
esmSyntaxErrorMessages,
27+
throwsOnlyInCommonJSErrorMessages,
28+
},
29+
},
30+
} = internalBinding('contextify');
2231
const { validateString } = require('internal/validators');
2332
const fs = require('fs'); // Import all of `fs` so that it can be monkey-patched.
2433
const internalFS = require('internal/fs/utils');
@@ -329,6 +338,31 @@ function urlToFilename(url) {
329338
return url;
330339
}
331340

341+
let esmSyntaxErrorMessagesSet; // Declared lazily in shouldRetryAsESM
342+
let throwsOnlyInCommonJSErrorMessagesSet; // Declared lazily in shouldRetryAsESM
343+
/**
344+
* After an attempt to parse a module as CommonJS throws an error, should we try again as ESM?
345+
* We only want to try again as ESM if the error is due to syntax that is only valid in ESM; and if the CommonJS parse
346+
* throws on an error that would not have been a syntax error in ESM (like via top-level `await` or a lexical
347+
* redeclaration of one of the CommonJS variables) then we need to parse again to see if it would have thrown in ESM.
348+
* @param {string} errorMessage The string message thrown by V8 when attempting to parse as CommonJS
349+
* @param {string} source Module contents
350+
*/
351+
function shouldRetryAsESM(errorMessage, source) {
352+
esmSyntaxErrorMessagesSet ??= new SafeSet(esmSyntaxErrorMessages);
353+
if (esmSyntaxErrorMessagesSet.has(errorMessage)) {
354+
return true;
355+
}
356+
357+
throwsOnlyInCommonJSErrorMessagesSet ??= new SafeSet(throwsOnlyInCommonJSErrorMessages);
358+
if (throwsOnlyInCommonJSErrorMessagesSet.has(errorMessage)) {
359+
return /** @type {boolean} */(contextifyShouldRetryAsESM(source));
360+
}
361+
362+
return false;
363+
}
364+
365+
332366
// Whether we have started executing any user-provided CJS code.
333367
// This is set right before we call the wrapped CJS code (not after,
334368
// in case we are half-way in the execution when internals check this).
@@ -362,6 +396,7 @@ module.exports = {
362396
loadBuiltinModule,
363397
makeRequireFunction,
364398
normalizeReferrerURL,
399+
shouldRetryAsESM,
365400
stripBOM,
366401
toRealPath,
367402
hasStartedUserCJSExecution() {

lib/internal/modules/run_main.js

+33-9
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ function resolveMainPath(main) {
4444
} catch (err) {
4545
if (defaultType === 'module' && err?.code === 'ENOENT') {
4646
const { decorateErrorWithCommonJSHints } = require('internal/modules/esm/resolve');
47+
const { getCWDURL } = require('internal/util');
4748
decorateErrorWithCommonJSHints(err, mainPath, getCWDURL());
4849
}
4950
throw err;
@@ -85,10 +86,6 @@ function shouldUseESMLoader(mainPath) {
8586
case 'commonjs':
8687
return false;
8788
default: { // No package.json or no `type` field.
88-
if (getOptionValue('--experimental-detect-module')) {
89-
// If the first argument of `containsModuleSyntax` is undefined, it will read `mainPath` from the file system.
90-
return containsModuleSyntax(undefined, mainPath);
91-
}
9289
return false;
9390
}
9491
}
@@ -153,12 +150,43 @@ function runEntryPointWithESMLoader(callback) {
153150
* by `require('module')`) even when the entry point is ESM.
154151
* This monkey-patchable code is bypassed under `--experimental-default-type=module`.
155152
* Because of backwards compatibility, this function is exposed publicly via `import { runMain } from 'node:module'`.
153+
* When `--experimental-detect-module` is passed, this function will attempt to run ambiguous (no explicit extension, no
154+
* `package.json` type field) entry points as CommonJS first; under certain conditions, it will retry running as ESM.
156155
* @param {string} main - First positional CLI argument, such as `'entry.js'` from `node entry.js`
157156
*/
158157
function executeUserEntryPoint(main = process.argv[1]) {
159158
const resolvedMain = resolveMainPath(main);
160159
const useESMLoader = shouldUseESMLoader(resolvedMain);
161-
if (useESMLoader) {
160+
161+
// Unless we know we should use the ESM loader to handle the entry point per the checks in `shouldUseESMLoader`, first
162+
// try to run the entry point via the CommonJS loader; and if that fails under certain conditions, retry as ESM.
163+
let retryAsESM = false;
164+
if (!useESMLoader) {
165+
const cjsLoader = require('internal/modules/cjs/loader');
166+
const { Module } = cjsLoader;
167+
if (getOptionValue('--experimental-detect-module')) {
168+
try {
169+
// Module._load is the monkey-patchable CJS module loader.
170+
Module._load(main, null, true);
171+
} catch (error) {
172+
const source = cjsLoader.entryPointSource;
173+
const { shouldRetryAsESM } = require('internal/modules/helpers');
174+
retryAsESM = shouldRetryAsESM(error.message, source);
175+
// In case the entry point is a large file, such as a bundle,
176+
// ensure no further references can prevent it being garbage-collected.
177+
cjsLoader.entryPointSource = undefined;
178+
if (!retryAsESM) {
179+
const { enrichCJSError } = require('internal/modules/esm/translators');
180+
enrichCJSError(error, source, resolvedMain);
181+
throw error;
182+
}
183+
}
184+
} else { // `--experimental-detect-module` is not passed
185+
Module._load(main, null, true);
186+
}
187+
}
188+
189+
if (useESMLoader || retryAsESM) {
162190
const mainPath = resolvedMain || main;
163191
const mainURL = pathToFileURL(mainPath).href;
164192

@@ -167,10 +195,6 @@ function executeUserEntryPoint(main = process.argv[1]) {
167195
// even after the event loop stops running.
168196
return cascadedLoader.import(mainURL, undefined, { __proto__: null }, true);
169197
});
170-
} else {
171-
// Module._load is the monkey-patchable CJS module loader.
172-
const { Module } = require('internal/modules/cjs/loader');
173-
Module._load(main, null, true);
174198
}
175199
}
176200

0 commit comments

Comments
 (0)