Skip to content

Commit 8bc7459

Browse files
module: eliminate performance cost of detection for cjs entry
PR-URL: #52093 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Richard Lau <rlau@redhat.com>
1 parent f1949ac commit 8bc7459

File tree

6 files changed

+192
-97
lines changed

6 files changed

+192
-97
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
count: [30],

lib/internal/modules/cjs/loader.js

+10-2
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ module.exports = {
7575
cjsExportsCache,
7676
cjsSourceCache,
7777
initializeCJS,
78+
entryPointSource: undefined, // Set below.
7879
Module,
7980
wrapSafe,
8081
};
@@ -1337,8 +1338,15 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) {
13371338
return result;
13381339
} catch (err) {
13391340
if (process.mainModule === cjsModuleInstance) {
1340-
const { enrichCJSError } = require('internal/modules/esm/translators');
1341-
enrichCJSError(err, content, filename);
1341+
if (getOptionValue('--experimental-detect-module')) {
1342+
// For the main entry point, cache the source to potentially retry as ESM.
1343+
module.exports.entryPointSource = content;
1344+
} else {
1345+
// We only enrich the error (print a warning) if we're sure we're going to for-sure throw it; so if we're
1346+
// retrying as ESM, wait until we know whether we're going to retry before calling `enrichCJSError`.
1347+
const { enrichCJSError } = require('internal/modules/esm/translators');
1348+
enrichCJSError(err, content, filename);
1349+
}
13421350
}
13431351
throw err;
13441352
}

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');
@@ -320,6 +329,31 @@ function normalizeReferrerURL(referrerName) {
320329
}
321330

322331

332+
let esmSyntaxErrorMessagesSet; // Declared lazily in shouldRetryAsESM
333+
let throwsOnlyInCommonJSErrorMessagesSet; // Declared lazily in shouldRetryAsESM
334+
/**
335+
* After an attempt to parse a module as CommonJS throws an error, should we try again as ESM?
336+
* 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
337+
* throws on an error that would not have been a syntax error in ESM (like via top-level `await` or a lexical
338+
* redeclaration of one of the CommonJS variables) then we need to parse again to see if it would have thrown in ESM.
339+
* @param {string} errorMessage The string message thrown by V8 when attempting to parse as CommonJS
340+
* @param {string} source Module contents
341+
*/
342+
function shouldRetryAsESM(errorMessage, source) {
343+
esmSyntaxErrorMessagesSet ??= new SafeSet(esmSyntaxErrorMessages);
344+
if (esmSyntaxErrorMessagesSet.has(errorMessage)) {
345+
return true;
346+
}
347+
348+
throwsOnlyInCommonJSErrorMessagesSet ??= new SafeSet(throwsOnlyInCommonJSErrorMessages);
349+
if (throwsOnlyInCommonJSErrorMessagesSet.has(errorMessage)) {
350+
return /** @type {boolean} */(contextifyShouldRetryAsESM(source));
351+
}
352+
353+
return false;
354+
}
355+
356+
323357
// Whether we have started executing any user-provided CJS code.
324358
// This is set right before we call the wrapped CJS code (not after,
325359
// in case we are half-way in the execution when internals check this).
@@ -339,6 +373,7 @@ module.exports = {
339373
loadBuiltinModule,
340374
makeRequireFunction,
341375
normalizeReferrerURL,
376+
shouldRetryAsESM,
342377
stripBOM,
343378
toRealPath,
344379
hasStartedUserCJSExecution() {

lib/internal/modules/run_main.js

+32-10
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ const {
55
globalThis,
66
} = primordials;
77

8-
const { containsModuleSyntax } = internalBinding('contextify');
98
const { getNearestParentPackageJSONType } = internalBinding('modules');
109
const { getOptionValue } = require('internal/options');
1110
const { checkPackageJSONIntegrity } = require('internal/modules/package_json_reader');
@@ -87,10 +86,6 @@ function shouldUseESMLoader(mainPath) {
8786

8887
// No package.json or no `type` field.
8988
if (response === undefined || response[0] === 'none') {
90-
if (getOptionValue('--experimental-detect-module')) {
91-
// If the first argument of `containsModuleSyntax` is undefined, it will read `mainPath` from the file system.
92-
return containsModuleSyntax(undefined, mainPath);
93-
}
9489
return false;
9590
}
9691

@@ -157,12 +152,43 @@ function runEntryPointWithESMLoader(callback) {
157152
* by `require('module')`) even when the entry point is ESM.
158153
* This monkey-patchable code is bypassed under `--experimental-default-type=module`.
159154
* Because of backwards compatibility, this function is exposed publicly via `import { runMain } from 'node:module'`.
155+
* When `--experimental-detect-module` is passed, this function will attempt to run ambiguous (no explicit extension, no
156+
* `package.json` type field) entry points as CommonJS first; under certain conditions, it will retry running as ESM.
160157
* @param {string} main - First positional CLI argument, such as `'entry.js'` from `node entry.js`
161158
*/
162159
function executeUserEntryPoint(main = process.argv[1]) {
163160
const resolvedMain = resolveMainPath(main);
164161
const useESMLoader = shouldUseESMLoader(resolvedMain);
165-
if (useESMLoader) {
162+
163+
// Unless we know we should use the ESM loader to handle the entry point per the checks in `shouldUseESMLoader`, first
164+
// try to run the entry point via the CommonJS loader; and if that fails under certain conditions, retry as ESM.
165+
let retryAsESM = false;
166+
if (!useESMLoader) {
167+
const cjsLoader = require('internal/modules/cjs/loader');
168+
const { Module } = cjsLoader;
169+
if (getOptionValue('--experimental-detect-module')) {
170+
try {
171+
// Module._load is the monkey-patchable CJS module loader.
172+
Module._load(main, null, true);
173+
} catch (error) {
174+
const source = cjsLoader.entryPointSource;
175+
const { shouldRetryAsESM } = require('internal/modules/helpers');
176+
retryAsESM = shouldRetryAsESM(error.message, source);
177+
// In case the entry point is a large file, such as a bundle,
178+
// ensure no further references can prevent it being garbage-collected.
179+
cjsLoader.entryPointSource = undefined;
180+
if (!retryAsESM) {
181+
const { enrichCJSError } = require('internal/modules/esm/translators');
182+
enrichCJSError(error, source, resolvedMain);
183+
throw error;
184+
}
185+
}
186+
} else { // `--experimental-detect-module` is not passed
187+
Module._load(main, null, true);
188+
}
189+
}
190+
191+
if (useESMLoader || retryAsESM) {
166192
const mainPath = resolvedMain || main;
167193
const mainURL = pathToFileURL(mainPath).href;
168194

@@ -171,10 +197,6 @@ function executeUserEntryPoint(main = process.argv[1]) {
171197
// even after the event loop stops running.
172198
return cascadedLoader.import(mainURL, undefined, { __proto__: null }, true);
173199
});
174-
} else {
175-
// Module._load is the monkey-patchable CJS module loader.
176-
const { Module } = require('internal/modules/cjs/loader');
177-
Module._load(main, null, true);
178200
}
179201
}
180202

0 commit comments

Comments
 (0)