Skip to content

Commit 278b15f

Browse files
GeoffreyBoothGeoffreyBooth
committed
module: eliminate performance cost of detection for cjs entry
1 parent b360532 commit 278b15f

File tree

6 files changed

+171
-10
lines changed

6 files changed

+171
-10
lines changed

‎lib/internal/modules/cjs/loader.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,18 @@ const cjsParseCache = new SafeWeakMap();
6969
*/
7070
const cjsExportsCache = new SafeWeakMap();
7171

72+
/**
73+
* Map of CJS modules where the initial attempt to parse as CommonJS failed;
74+
* we want to retry as ESM but avoid reading the module from disk again.
75+
* @type {SafeMap<string, string>} filename: contents
76+
*/
77+
const cjsRetryAsESMCache = new SafeMap();
78+
7279
// Set first due to cycle with ESM loader functions.
7380
module.exports = {
7481
cjsExportsCache,
7582
cjsParseCache,
83+
cjsRetryAsESMCache,
7684
initializeCJS,
7785
Module,
7886
wrapSafe,
@@ -1293,6 +1301,10 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) {
12931301

12941302
return result.function;
12951303
} catch (err) {
1304+
if (getOptionValue('--experimental-detect-module')) {
1305+
cjsRetryAsESMCache.set(filename, content);
1306+
}
1307+
12961308
if (process.mainModule === cjsModuleInstance) {
12971309
const { enrichCJSError } = require('internal/modules/esm/translators');
12981310
enrichCJSError(err, content, filename);

‎lib/internal/modules/helpers.js‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,37 @@ function normalizeReferrerURL(referrerName) {
320320
}
321321

322322

323+
const esmSyntaxErrorMessages = new SafeSet([
324+
'Cannot use import statement outside a module', // `import` statements
325+
"Unexpected token 'export'", // `export` statements
326+
"Cannot use 'import.meta' outside a module", // `import.meta` references
327+
]);
328+
const throwsOnlyInCommonJSErrorMessages = new SafeSet([
329+
"Identifier 'module' has already been declared",
330+
"Identifier 'exports' has already been declared",
331+
"Identifier 'require' has already been declared",
332+
"Identifier '__filename' has already been declared",
333+
"Identifier '__dirname' has already been declared",
334+
'await is only valid in async functions and the top level bodies of modules', // Top-level `await`
335+
]);
336+
/**
337+
* After an attempt to parse a module as CommonJS throws an error, should we try again as ESM?
338+
* @param {string} errorMessage The string message thrown by V8 when attempting to parse as CommonJS
339+
* @param {string} source Module contents
340+
*/
341+
function shouldRetryAsESM(errorMessage, source) {
342+
if (esmSyntaxErrorMessages.has(errorMessage)) {
343+
return true;
344+
}
345+
346+
if (throwsOnlyInCommonJSErrorMessages.has(errorMessage)) {
347+
return /** @type {boolean} */(internalBinding('contextify').shouldRetryAsESM(source));
348+
}
349+
350+
return false;
351+
}
352+
353+
323354
// Whether we have started executing any user-provided CJS code.
324355
// This is set right before we call the wrapped CJS code (not after,
325356
// in case we are half-way in the execution when internals check this).
@@ -339,6 +370,7 @@ module.exports = {
339370
loadBuiltinModule,
340371
makeRequireFunction,
341372
normalizeReferrerURL,
373+
shouldRetryAsESM,
342374
stripBOM,
343375
toRealPath,
344376
hasStartedUserCJSExecution() {

‎lib/internal/modules/run_main.js‎

Lines changed: 24 additions & 10 deletions
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

@@ -162,7 +157,30 @@ function runEntryPointWithESMLoader(callback) {
162157
function executeUserEntryPoint(main = process.argv[1]) {
163158
const resolvedMain = resolveMainPath(main);
164159
const useESMLoader = shouldUseESMLoader(resolvedMain);
165-
if (useESMLoader) {
160+
161+
// We might retry as ESM if the CommonJS loader throws because the file is ESM, so
162+
// try the CommonJS loader first unless we know it's supposed to be parsed as ESM.
163+
let retryAsESM = false;
164+
if (!useESMLoader) {
165+
// Module._load is the monkey-patchable CJS module loader.
166+
const { Module } = require('internal/modules/cjs/loader');
167+
try {
168+
Module._load(main, null, true);
169+
} catch (error) {
170+
if (getOptionValue('--experimental-detect-module')) {
171+
const { cjsRetryAsESMCache } = require('internal/modules/cjs/loader');
172+
const source = cjsRetryAsESMCache.get(resolvedMain);
173+
cjsRetryAsESMCache.delete(resolvedMain);
174+
const { shouldRetryAsESM } = require('internal/modules/helpers');
175+
retryAsESM = shouldRetryAsESM(error.message, source);
176+
}
177+
if (!retryAsESM) {
178+
throw error;
179+
}
180+
}
181+
}
182+
183+
if (useESMLoader || retryAsESM) {
166184
const mainPath = resolvedMain || main;
167185
const mainURL = pathToFileURL(mainPath).href;
168186

@@ -171,10 +189,6 @@ function executeUserEntryPoint(main = process.argv[1]) {
171189
// even after the event loop stops running.
172190
return cascadedLoader.import(mainURL, undefined, { __proto__: null }, true);
173191
});
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);
178192
}
179193
}
180194

‎src/node_contextify.cc‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,13 +345,15 @@ void ContextifyContext::CreatePerIsolateProperties(
345345
SetMethod(isolate, target, "makeContext", MakeContext);
346346
SetMethod(isolate, target, "compileFunction", CompileFunction);
347347
SetMethod(isolate, target, "containsModuleSyntax", ContainsModuleSyntax);
348+
SetMethod(isolate, target, "shouldRetryAsESM", ShouldRetryAsESM);
348349
}
349350

350351
void ContextifyContext::RegisterExternalReferences(
351352
ExternalReferenceRegistry* registry) {
352353
registry->Register(MakeContext);
353354
registry->Register(CompileFunction);
354355
registry->Register(ContainsModuleSyntax);
356+
registry->Register(ShouldRetryAsESM);
355357
registry->Register(PropertyGetterCallback);
356358
registry->Register(PropertySetterCallback);
357359
registry->Register(PropertyDescriptorCallback);
@@ -1566,6 +1568,88 @@ void ContextifyContext::ContainsModuleSyntax(
15661568
args.GetReturnValue().Set(should_retry_as_esm);
15671569
}
15681570

1571+
void ContextifyContext::ShouldRetryAsESM(
1572+
const FunctionCallbackInfo<Value>& args) {
1573+
Environment* env = Environment::GetCurrent(args);
1574+
Isolate* isolate = env->isolate();
1575+
Local<Context> context = env->context();
1576+
1577+
if (args.Length() != 1) {
1578+
return THROW_ERR_MISSING_ARGS(
1579+
env, "shouldRetryAsESM needs 1 argument");
1580+
}
1581+
// Argument 1: source code
1582+
Local<String> code;
1583+
CHECK(args[0]->IsString());
1584+
code = args[0].As<String>();
1585+
1586+
Local<String> script_id = String::NewFromUtf8(isolate, "throwaway").ToLocalChecked();
1587+
Local<Symbol> id_symbol = Symbol::New(isolate, script_id);
1588+
1589+
Local<PrimitiveArray> host_defined_options =
1590+
GetHostDefinedOptions(isolate, id_symbol);
1591+
ScriptCompiler::Source source = GetCommonJSSourceInstance(
1592+
isolate, code, script_id, 0, 0, host_defined_options, nullptr);
1593+
ScriptCompiler::CompileOptions options = GetCompileOptions(source);
1594+
1595+
TryCatchScope try_catch(env);
1596+
ShouldNotAbortOnUncaughtScope no_abort_scope(env);
1597+
1598+
// Try parsing where instead of the CommonJS wrapper we use an async function
1599+
// wrapper. If the parse succeeds, then any CommonJS parse error for this
1600+
// module was caused by either a top-level declaration of one of the CommonJS
1601+
// module variables, or a top-level `await`.
1602+
code =
1603+
String::Concat(isolate,
1604+
String::NewFromUtf8(isolate, "(async function() {")
1605+
.ToLocalChecked(),
1606+
code);
1607+
code = String::Concat(
1608+
isolate,
1609+
code,
1610+
String::NewFromUtf8(isolate, "})();").ToLocalChecked());
1611+
1612+
ScriptCompiler::Source wrapped_source = GetCommonJSSourceInstance(
1613+
isolate, code, script_id, 0, 0, host_defined_options, nullptr);
1614+
1615+
std::vector<Local<String>> params = GetCJSParameters(env->isolate_data());
1616+
std::ignore = ScriptCompiler::CompileFunction(
1617+
context,
1618+
&wrapped_source,
1619+
params.size(),
1620+
params.data(),
1621+
0,
1622+
nullptr,
1623+
options,
1624+
v8::ScriptCompiler::NoCacheReason::kNoCacheNoReason);
1625+
1626+
bool should_retry_as_esm = false;
1627+
if (!try_catch.HasTerminated()) {
1628+
if (try_catch.HasCaught()) {
1629+
// If on the second parse an error is thrown by ESM syntax, then
1630+
// what happened was that the user had top-level `await` or a
1631+
// top-level declaration of one of the CommonJS module variables
1632+
// above the first `import` or `export`.
1633+
Utf8Value message_value(
1634+
env->isolate(), try_catch.Message()->Get());
1635+
auto message_view = message_value.ToStringView();
1636+
for (const auto& error_message : esm_syntax_error_messages) {
1637+
if (message_view.find(error_message) !=
1638+
std::string_view::npos) {
1639+
should_retry_as_esm = true;
1640+
break;
1641+
}
1642+
}
1643+
} else {
1644+
// No errors thrown in the second parse, so most likely the error
1645+
// was caused by a top-level `await` or a top-level declaration of
1646+
// one of the CommonJS module variables.
1647+
should_retry_as_esm = true;
1648+
}
1649+
}
1650+
args.GetReturnValue().Set(should_retry_as_esm);
1651+
}
1652+
15691653
static void CompileFunctionForCJSLoader(
15701654
const FunctionCallbackInfo<Value>& args) {
15711655
CHECK(args[0]->IsString());

‎src/node_contextify.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,8 @@ class ContextifyContext : public BaseObject {
106106
const v8::ScriptCompiler::Source& source);
107107
static void ContainsModuleSyntax(
108108
const v8::FunctionCallbackInfo<v8::Value>& args);
109+
static void ShouldRetryAsESM(
110+
const v8::FunctionCallbackInfo<v8::Value>& args);
109111
static void WeakCallback(
110112
const v8::WeakCallbackInfo<ContextifyContext>& data);
111113
static void PropertyGetterCallback(

‎test/es-module/test-esm-detect-ambiguous.mjs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
88
describe('string input', { concurrency: true }, () => {
99
it('permits ESM syntax in --eval input without requiring --input-type=module', async () => {
1010
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
11+
'--no-warnings',
1112
'--experimental-detect-module',
1213
'--eval',
1314
'import { version } from "node:process"; console.log(version);',
@@ -23,6 +24,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
2324

2425
it('permits ESM syntax in STDIN input without requiring --input-type=module', async () => {
2526
const child = spawn(process.execPath, [
27+
'--no-warnings',
2628
'--experimental-detect-module',
2729
]);
2830
child.stdin.end('console.log(typeof import.meta.resolve)');
@@ -32,6 +34,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
3234

3335
it('should be overridden by --input-type', async () => {
3436
const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
37+
'--no-warnings',
3538
'--experimental-detect-module',
3639
'--input-type=commonjs',
3740
'--eval',
@@ -46,6 +49,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
4649

4750
it('should be overridden by --experimental-default-type', async () => {
4851
const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
52+
'--no-warnings',
4953
'--experimental-detect-module',
5054
'--experimental-default-type=commonjs',
5155
'--eval',
@@ -60,6 +64,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
6064

6165
it('does not trigger detection via source code `eval()`', async () => {
6266
const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [
67+
'--no-warnings',
6368
'--experimental-detect-module',
6469
'--eval',
6570
'eval("import \'nonexistent\';");',
@@ -101,6 +106,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
101106
]) {
102107
it(testName, async () => {
103108
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
109+
'--no-warnings',
104110
'--experimental-detect-module',
105111
entryPath,
106112
]);
@@ -142,6 +148,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
142148
]) {
143149
it(testName, async () => {
144150
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
151+
'--no-warnings',
145152
'--experimental-detect-module',
146153
entryPath,
147154
]);
@@ -156,6 +163,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
156163
it('should not hint wrong format in resolve hook', async () => {
157164
let writeSync;
158165
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
166+
'--no-warnings',
159167
'--experimental-detect-module',
160168
'--no-warnings',
161169
'--loader',
@@ -194,6 +202,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
194202
]) {
195203
it(testName, async () => {
196204
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
205+
'--no-warnings',
197206
'--experimental-detect-module',
198207
entryPath,
199208
]);
@@ -223,6 +232,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
223232
]) {
224233
it(testName, async () => {
225234
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
235+
'--no-warnings',
226236
'--experimental-detect-module',
227237
entryPath,
228238
]);
@@ -239,6 +249,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
239249
describe('syntax that errors in CommonJS but works in ESM', { concurrency: true }, () => {
240250
it('permits top-level `await`', async () => {
241251
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
252+
'--no-warnings',
242253
'--experimental-detect-module',
243254
'--eval',
244255
'await Promise.resolve(); console.log("executed");',
@@ -252,6 +263,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
252263

253264
it('permits top-level `await` above import/export syntax', async () => {
254265
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
266+
'--no-warnings',
255267
'--experimental-detect-module',
256268
'--eval',
257269
'await Promise.resolve(); import "node:os"; console.log("executed");',
@@ -265,6 +277,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
265277

266278
it('still throws on `await` in an ordinary sync function', async () => {
267279
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
280+
'--no-warnings',
268281
'--experimental-detect-module',
269282
'--eval',
270283
'function fn() { await Promise.resolve(); } fn();',
@@ -278,6 +291,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
278291

279292
it('throws on undefined `require` when top-level `await` triggers ESM parsing', async () => {
280293
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
294+
'--no-warnings',
281295
'--experimental-detect-module',
282296
'--eval',
283297
'const fs = require("node:fs"); await Promise.resolve();',
@@ -291,6 +305,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
291305

292306
it('permits declaration of CommonJS module variables', async () => {
293307
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
308+
'--no-warnings',
294309
'--experimental-detect-module',
295310
fixtures.path('es-modules/package-without-type/commonjs-wrapper-variables.js'),
296311
]);
@@ -303,6 +318,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
303318

304319
it('permits declaration of CommonJS module variables above import/export', async () => {
305320
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
321+
'--no-warnings',
306322
'--experimental-detect-module',
307323
'--eval',
308324
'const module = 3; import "node:os"; console.log("executed");',
@@ -316,6 +332,7 @@ describe('--experimental-detect-module', { concurrency: true }, () => {
316332

317333
it('still throws on double `const` declaration not at the top level', async () => {
318334
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
335+
'--no-warnings',
319336
'--experimental-detect-module',
320337
'--eval',
321338
'function fn() { const require = 1; const require = 2; } fn();',

0 commit comments

Comments
 (0)