From fdf2ca1a04f4caab3214b8f0e154e24bd6187aaf Mon Sep 17 00:00:00 2001 From: Sergey Chernyshev Date: Thu, 25 Apr 2024 04:11:57 +0200 Subject: [PATCH] esm: bypass CommonJS loader under --default-type PR-URL: https://github.com/nodejs/node/pull/49986 Backport-PR-URL: https://github.com/nodejs/node/pull/50669 Reviewed-By: Jacob Smith Reviewed-By: Antoine du Hamel --- graal-nodejs/doc/api/cli.md | 14 ++- .../lib/internal/main/run_main_module.js | 22 +++-- .../lib/internal/modules/esm/resolve.js | 33 ++++--- graal-nodejs/lib/internal/modules/run_main.js | 39 +++++--- .../lib/internal/process/pre_execution.js | 15 ++- .../test-esm-type-flag-cli-entry.mjs | 92 +++++++++++++++++++ .../fixtures/errors/force_colors.snapshot | 4 +- .../es-modules/package-without-type/file#1.js | 1 + 8 files changed, 179 insertions(+), 41 deletions(-) create mode 100644 graal-nodejs/test/es-module/test-esm-type-flag-cli-entry.mjs create mode 100644 graal-nodejs/test/fixtures/es-modules/package-without-type/file#1.js diff --git a/graal-nodejs/doc/api/cli.md b/graal-nodejs/doc/api/cli.md index 4fecc46df4d..83d9c5182e1 100644 --- a/graal-nodejs/doc/api/cli.md +++ b/graal-nodejs/doc/api/cli.md @@ -25,14 +25,16 @@ For more info about `node inspect`, see the [debugger][] documentation. The program entry point is a specifier-like string. If the string is not an absolute path, it's resolved as a relative path from the current working -directory. That path is then resolved by [CommonJS][] module loader. If no -corresponding file is found, an error is thrown. +directory. That path is then resolved by [CommonJS][] module loader, or by the +[ES module loader][Modules loaders] if [`--experimental-default-type=module`][] +is passed. If no corresponding file is found, an error is thrown. If a file is found, its path will be passed to the [ES module loader][Modules loaders] under any of the following conditions: * The program was started with a command-line flag that forces the entry - point to be loaded with ECMAScript module loader. + point to be loaded with ECMAScript module loader, such as `--import` or + [`--experimental-default-type=module`][]. * The file has an `.mjs` extension. * The file does not have a `.cjs` extension, and the nearest parent `package.json` file contains a top-level [`"type"`][] field with a value of @@ -45,8 +47,9 @@ Otherwise, the file is loaded using the CommonJS module loader. See When loading, the [ES module loader][Modules loaders] loads the program entry point, the `node` command will accept as input only files with `.js`, -`.mjs`, or `.cjs` extensions; and with `.wasm` extensions when -[`--experimental-wasm-modules`][] is enabled. +`.mjs`, or `.cjs` extensions; with `.wasm` extensions when +[`--experimental-wasm-modules`][] is enabled; and with no extension when +[`--experimental-default-type=module`][] is passed. ## Options @@ -2420,6 +2423,7 @@ done [`"type"`]: packages.md#type [`--cpu-prof-dir`]: #--cpu-prof-dir [`--diagnostic-dir`]: #--diagnostic-dirdirectory +[`--experimental-default-type=module`]: #--experimental-default-typetype [`--experimental-wasm-modules`]: #--experimental-wasm-modules [`--heap-prof-dir`]: #--heap-prof-dir [`--import`]: #--importmodule diff --git a/graal-nodejs/lib/internal/main/run_main_module.js b/graal-nodejs/lib/internal/main/run_main_module.js index 51331270a21..5d09203b8c2 100644 --- a/graal-nodejs/lib/internal/main/run_main_module.js +++ b/graal-nodejs/lib/internal/main/run_main_module.js @@ -6,18 +6,24 @@ const { prepareMainThreadExecution, markBootstrapComplete, } = require('internal/process/pre_execution'); +const { getOptionValue } = require('internal/options'); -prepareMainThreadExecution(true); +const mainEntry = prepareMainThreadExecution(true); markBootstrapComplete(); // Necessary to reset RegExp statics before user code runs. RegExpPrototypeExec(/^/, ''); -// Note: this loads the module through the ESM loader if the module is -// determined to be an ES module. This hangs from the CJS module loader -// because we currently allow monkey-patching of the module loaders -// in the preloaded scripts through require('module'). -// runMain here might be monkey-patched by users in --require. -// XXX: the monkey-patchability here should probably be deprecated. -require('internal/modules/cjs/loader').Module.runMain(process.argv[1]); +if (getOptionValue('--experimental-default-type') === 'module') { + require('internal/modules/run_main').executeUserEntryPoint(mainEntry); +} else { + /** + * To support legacy monkey-patching of `Module.runMain`, we call `runMain` here to have the CommonJS loader begin + * the execution of the main entry point, even if the ESM loader immediately takes over because the main entry is an + * ES module or one of the other opt-in conditions (such as the use of `--import`) are met. Users can monkey-patch + * before the main entry point is loaded by doing so via scripts loaded through `--require`. This monkey-patchability + * is undesirable and is removed in `--experimental-default-type=module` mode. + */ + require('internal/modules/cjs/loader').Module.runMain(mainEntry); +} diff --git a/graal-nodejs/lib/internal/modules/esm/resolve.js b/graal-nodejs/lib/internal/modules/esm/resolve.js index 529110f92ee..96dd20a86b0 100644 --- a/graal-nodejs/lib/internal/modules/esm/resolve.js +++ b/graal-nodejs/lib/internal/modules/esm/resolve.js @@ -1204,17 +1204,7 @@ function defaultResolve(specifier, context = {}) { if (StringPrototypeStartsWith(specifier, 'file://')) { specifier = fileURLToPath(specifier); } - const found = resolveAsCommonJS(specifier, parentURL); - if (found) { - // Modify the stack and message string to include the hint - const lines = StringPrototypeSplit(error.stack, '\n'); - const hint = `Did you mean to import ${found}?`; - error.stack = - ArrayPrototypeShift(lines) + '\n' + - hint + '\n' + - ArrayPrototypeJoin(lines, '\n'); - error.message += `\n${hint}`; - } + decorateErrorWithCommonJSHints(error, specifier, parentURL); } throw error; } @@ -1227,7 +1217,28 @@ function defaultResolve(specifier, context = {}) { }; } +/** + * Decorates the given error with a hint for CommonJS modules. + * @param {Error} error - The error to decorate. + * @param {string} specifier - The specifier that was attempted to be imported. + * @param {string} parentURL - The URL of the parent module. + */ +function decorateErrorWithCommonJSHints(error, specifier, parentURL) { + const found = resolveAsCommonJS(specifier, parentURL); + if (found) { + // Modify the stack and message string to include the hint + const lines = StringPrototypeSplit(error.stack, '\n'); + const hint = `Did you mean to import ${found}?`; + error.stack = + ArrayPrototypeShift(lines) + '\n' + + hint + '\n' + + ArrayPrototypeJoin(lines, '\n'); + error.message += `\n${hint}`; + } +} + module.exports = { + decorateErrorWithCommonJSHints, defaultResolve, encodedSepRegEx, getPackageScopeConfig, diff --git a/graal-nodejs/lib/internal/modules/run_main.js b/graal-nodejs/lib/internal/modules/run_main.js index 2f9254beed2..287f3ed91b1 100644 --- a/graal-nodejs/lib/internal/modules/run_main.js +++ b/graal-nodejs/lib/internal/modules/run_main.js @@ -1,7 +1,6 @@ 'use strict'; const { - ObjectCreate, StringPrototypeEndsWith, } = primordials; @@ -13,17 +12,33 @@ const path = require('path'); * @param {string} main - Entry point path */ function resolveMainPath(main) { - // Note extension resolution for the main entry point can be deprecated in a - // future major. - // Module._findPath is monkey-patchable here. - const { Module } = require('internal/modules/cjs/loader'); - let mainPath = Module._findPath(path.resolve(main), null, true); + const defaultType = getOptionValue('--experimental-default-type'); + /** @type {string} */ + let mainPath; + if (defaultType === 'module') { + if (getOptionValue('--preserve-symlinks-main')) { return; } + mainPath = path.resolve(main); + } else { + // Extension searching for the main entry point is supported only in legacy mode. + // Module._findPath is monkey-patchable here. + const { Module } = require('internal/modules/cjs/loader'); + mainPath = Module._findPath(path.resolve(main), null, true); + } if (!mainPath) { return; } const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); if (!preserveSymlinksMain) { const { toRealPath } = require('internal/modules/helpers'); - mainPath = toRealPath(mainPath); + try { + mainPath = toRealPath(mainPath); + } catch (err) { + if (defaultType === 'module' && err?.code === 'ENOENT') { + const { decorateErrorWithCommonJSHints } = require('internal/modules/esm/resolve'); + const { getCWDURL } = require('internal/util'); + decorateErrorWithCommonJSHints(err, mainPath, getCWDURL()); + } + throw err; + } } return mainPath; @@ -34,6 +49,8 @@ function resolveMainPath(main) { * @param {string} mainPath - Absolute path to the main entry point */ function shouldUseESMLoader(mainPath) { + if (getOptionValue('--experimental-default-type') === 'module') { return true; } + /** * @type {string[]} userLoaders A list of custom loaders registered by the user * (or an empty list when none have been registered). @@ -69,11 +86,10 @@ function shouldUseESMLoader(mainPath) { function runMainESM(mainPath) { const { loadESM } = require('internal/process/esm_loader'); const { pathToFileURL } = require('internal/url'); + const main = pathToFileURL(mainPath).href; handleMainPromise(loadESM((esmLoader) => { - const main = path.isAbsolute(mainPath) ? - pathToFileURL(mainPath).href : mainPath; - return esmLoader.import(main, undefined, ObjectCreate(null)); + return esmLoader.import(main, undefined, { __proto__: null }); })); } @@ -97,8 +113,9 @@ async function handleMainPromise(promise) { * Parse the CLI main entry point string and run it. * For backwards compatibility, we have to run a bunch of monkey-patchable code that belongs to the CJS loader (exposed * 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'`. - * @param {string} main - Resolved absolute path for the main entry point, if found + * @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); diff --git a/graal-nodejs/lib/internal/process/pre_execution.js b/graal-nodejs/lib/internal/process/pre_execution.js index 6b365c6e1c7..9347c8b1e4a 100644 --- a/graal-nodejs/lib/internal/process/pre_execution.js +++ b/graal-nodejs/lib/internal/process/pre_execution.js @@ -37,7 +37,7 @@ const { } = require('internal/v8/startup_snapshot'); function prepareMainThreadExecution(expandArgv1 = false, initializeModules = true) { - prepareExecution({ + return prepareExecution({ expandArgv1, initializeModules, isMainThread: true, @@ -58,8 +58,8 @@ function prepareExecution(options) { refreshRuntimeOptions(); reconnectZeroFillToggle(); - // Patch the process object with legacy properties and normalizations - patchProcessObject(expandArgv1); + // Patch the process object and get the resolved main entry point. + const mainEntry = patchProcessObject(expandArgv1); setupTraceCategoryState(); setupPerfHooks(); setupInspectorHooks(); @@ -113,6 +113,8 @@ function prepareExecution(options) { if (initializeModules) { setupUserModules(); } + + return mainEntry; } function setupSymbolDisposePolyfill() { @@ -167,6 +169,8 @@ function patchProcessObject(expandArgv1) { process._exiting = false; process.argv[0] = process.execPath; + /** @type {string} */ + let mainEntry; // If requested, update process.argv[1] to replace whatever the user provided with the resolved absolute file path of // the entry point. if (expandArgv1 && process.argv[1] && @@ -174,7 +178,8 @@ function patchProcessObject(expandArgv1) { // Expand process.argv[1] into a full path. const path = require('path'); try { - process.argv[1] = path.resolve(process.argv[1]); + mainEntry = path.resolve(process.argv[1]); + process.argv[1] = mainEntry; } catch { // Continue regardless of error. } @@ -208,6 +213,8 @@ function patchProcessObject(expandArgv1) { return fn.apply(self, args); } } + + return mainEntry; } function addReadOnlyProcessAlias(name, option, enumerable = true) { diff --git a/graal-nodejs/test/es-module/test-esm-type-flag-cli-entry.mjs b/graal-nodejs/test/es-module/test-esm-type-flag-cli-entry.mjs new file mode 100644 index 00000000000..00284075153 --- /dev/null +++ b/graal-nodejs/test/es-module/test-esm-type-flag-cli-entry.mjs @@ -0,0 +1,92 @@ +import { spawnPromisified } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import { describe, it } from 'node:test'; +import { match, strictEqual } from 'node:assert'; + +describe('--experimental-default-type=module should not support extension searching', { concurrency: true }, () => { + it('should support extension searching under --experimental-default-type=commonjs', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=commonjs', + 'index', + ], { + cwd: fixtures.path('es-modules/package-without-type'), + }); + + strictEqual(stdout, 'package-without-type\n'); + strictEqual(stderr, ''); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should error with implicit extension under --experimental-default-type=module', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + 'index', + ], { + cwd: fixtures.path('es-modules/package-without-type'), + }); + + match(stderr, /ENOENT.*Did you mean to import .*index\.js\?/s); + strictEqual(stdout, ''); + strictEqual(code, 1); + strictEqual(signal, null); + }); +}); + +describe('--experimental-default-type=module should not parse paths as URLs', { concurrency: true }, () => { + it('should not parse a `?` in a filename as starting a query string', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + 'file#1.js', + ], { + cwd: fixtures.path('es-modules/package-without-type'), + }); + + strictEqual(stderr, ''); + strictEqual(stdout, 'file#1\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should resolve `..`', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '../package-without-type/file#1.js', + ], { + cwd: fixtures.path('es-modules/package-without-type'), + }); + + strictEqual(stderr, ''); + strictEqual(stdout, 'file#1\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should allow a leading `./`', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + './file#1.js', + ], { + cwd: fixtures.path('es-modules/package-without-type'), + }); + + strictEqual(stderr, ''); + strictEqual(stdout, 'file#1\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should not require a leading `./`', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + 'file#1.js', + ], { + cwd: fixtures.path('es-modules/package-without-type'), + }); + + strictEqual(stderr, ''); + strictEqual(stdout, 'file#1\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); +}); diff --git a/graal-nodejs/test/fixtures/errors/force_colors.snapshot b/graal-nodejs/test/fixtures/errors/force_colors.snapshot index 5eee5742e2d..389bc964793 100644 --- a/graal-nodejs/test/fixtures/errors/force_colors.snapshot +++ b/graal-nodejs/test/fixtures/errors/force_colors.snapshot @@ -8,7 +8,7 @@ Error: Should include grayed stack trace  at Module._extensions..js (node:internal*modules*cjs*loader:1414:10)  at Module.load (node:internal*modules*cjs*loader:1197:32)  at Module._load (node:internal*modules*cjs*loader:1013:12) - at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:109:12) - at node:internal*main*run_main_module:23:47 + at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:128:12) + at node:internal*main*run_main_module:28:49 Node.js * diff --git a/graal-nodejs/test/fixtures/es-modules/package-without-type/file#1.js b/graal-nodejs/test/fixtures/es-modules/package-without-type/file#1.js new file mode 100644 index 00000000000..6ab97dbf4b5 --- /dev/null +++ b/graal-nodejs/test/fixtures/es-modules/package-without-type/file#1.js @@ -0,0 +1 @@ +console.log('file#1');