-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
module: eliminate performance cost of ESM syntax detection for CommonJS entry points #52093
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
Changes from all commits
5aa3528
29f5a6d
75dba32
f06b12e
c82a15a
4b6a52a
65d690b
9ad9ace
4ad9958
43eb1b5
51268ec
1c6496c
1685245
8d7e077
d69398a
fd2e330
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ const { | |
globalThis, | ||
} = primordials; | ||
|
||
const { containsModuleSyntax } = internalBinding('contextify'); | ||
const { getNearestParentPackageJSONType } = internalBinding('modules'); | ||
const { getOptionValue } = require('internal/options'); | ||
const { checkPackageJSONIntegrity } = require('internal/modules/package_json_reader'); | ||
|
@@ -87,10 +86,6 @@ function shouldUseESMLoader(mainPath) { | |
|
||
// No package.json or no `type` field. | ||
if (response === undefined || response[0] === 'none') { | ||
if (getOptionValue('--experimental-detect-module')) { | ||
// If the first argument of `containsModuleSyntax` is undefined, it will read `mainPath` from the file system. | ||
return containsModuleSyntax(undefined, mainPath); | ||
} | ||
return false; | ||
} | ||
|
||
|
@@ -157,12 +152,43 @@ function runEntryPointWithESMLoader(callback) { | |
* by `require('module')`) even when the entry point is ESM. | ||
* This monkey-patchable code is bypassed under `--experimental-default-type=module`. | ||
* Because of backwards compatibility, this function is exposed publicly via `import { runMain } from 'node:module'`. | ||
* When `--experimental-detect-module` is passed, this function will attempt to run ambiguous (no explicit extension, no | ||
* `package.json` type field) entry points as CommonJS first; under certain conditions, it will retry running as ESM. | ||
* @param {string} main - First positional CLI argument, such as `'entry.js'` from `node entry.js` | ||
*/ | ||
function executeUserEntryPoint(main = process.argv[1]) { | ||
const resolvedMain = resolveMainPath(main); | ||
const useESMLoader = shouldUseESMLoader(resolvedMain); | ||
if (useESMLoader) { | ||
|
||
GeoffreyBooth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Unless we know we should use the ESM loader to handle the entry point per the checks in `shouldUseESMLoader`, first | ||
// try to run the entry point via the CommonJS loader; and if that fails under certain conditions, retry as ESM. | ||
let retryAsESM = false; | ||
if (!useESMLoader) { | ||
const cjsLoader = require('internal/modules/cjs/loader'); | ||
const { Module } = cjsLoader; | ||
if (getOptionValue('--experimental-detect-module')) { | ||
try { | ||
// Module._load is the monkey-patchable CJS module loader. | ||
Module._load(main, null, true); | ||
GeoffreyBooth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} catch (error) { | ||
const source = cjsLoader.entryPointSource; | ||
const { shouldRetryAsESM } = require('internal/modules/helpers'); | ||
retryAsESM = shouldRetryAsESM(error.message, source); | ||
// In case the entry point is a large file, such as a bundle, | ||
// ensure no further references can prevent it being garbage-collected. | ||
cjsLoader.entryPointSource = undefined; | ||
if (!retryAsESM) { | ||
const { enrichCJSError } = require('internal/modules/esm/translators'); | ||
enrichCJSError(error, source, resolvedMain); | ||
throw error; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you solve the minor issue you were asking about where this re-throw alters the stack-trace? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I solved it for the purposes of this PR by putting the try/catch within the check for the detect-module flag. #52093 (comment) should solve it generally, for when we unflag this. |
||
} | ||
} | ||
} else { // `--experimental-detect-module` is not passed | ||
Module._load(main, null, true); | ||
GeoffreyBooth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
if (useESMLoader || retryAsESM) { | ||
const mainPath = resolvedMain || main; | ||
const mainURL = pathToFileURL(mainPath).href; | ||
|
||
|
@@ -171,10 +197,6 @@ function executeUserEntryPoint(main = process.argv[1]) { | |
// even after the event loop stops running. | ||
return cascadedLoader.import(mainURL, undefined, { __proto__: null }, true); | ||
}); | ||
} else { | ||
// Module._load is the monkey-patchable CJS module loader. | ||
const { Module } = require('internal/modules/cjs/loader'); | ||
Module._load(main, null, true); | ||
} | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.