From e2583038230f64e3ee5fdc7aebeaf1217385176c Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Sat, 13 Apr 2024 21:41:20 +0200 Subject: [PATCH] module: tidy code and comments PR-URL: https://github.com/nodejs/node/pull/52437 Reviewed-By: Geoffrey Booth Reviewed-By: Yagiz Nizipli Reviewed-By: Joyee Cheung Reviewed-By: Luigi Pinca Reviewed-By: Moshe Atlow Reviewed-By: Antoine du Hamel Reviewed-By: Feng Yu --- doc/api/errors.md | 2 +- lib/internal/modules/cjs/loader.js | 2 +- lib/internal/modules/esm/loader.js | 28 ++++++++++++++++++------- lib/internal/modules/esm/translators.js | 5 +++-- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 7a1d6d12b981c9..eaefe13560ccba 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2533,7 +2533,7 @@ This is not allowed because ES Modules cannot be evaluated while they are already being evaluated. To avoid the cycle, the `require()` call involved in a cycle should not happen -at the top-level of either a ES Module (via `createRequire()`) or a CommonJS +at the top-level of either an ES Module (via `createRequire()`) or a CommonJS module, and should be done lazily in an inner function. diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 042f4d061c2c01..c284b39b1ac13e 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1063,7 +1063,7 @@ Module._load = function(request, parent, isMain) { } // If it's cached by the ESM loader as a way to indirectly pass // the module in to avoid creating it twice, the loading request - // come from imported CJS. In that case use the kModuleCircularVisited + // came from imported CJS. In that case use the kModuleCircularVisited // to determine if it's loading or not. if (cachedModule[kModuleCircularVisited]) { return getExportsForCircularRequire(cachedModule); diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 4c0b351aa8163e..418bbbfe7bdf39 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -42,6 +42,10 @@ const { } = require('internal/modules/helpers'); let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer; +/** + * @typedef {import('url').URL} URL + */ + /** * Lazy loads the module_map module and returns a new instance of ResolveCache. * @returns {import('./module_map.js').ResolveCache} @@ -77,6 +81,10 @@ function getTranslators() { */ let hooksProxy; +/** + * @typedef {import('../cjs/loader.js').Module} CJSModule + */ + /** * @typedef {Record} ModuleExports */ @@ -257,11 +265,11 @@ class ModuleLoader { /** * This constructs (creates, instantiates and evaluates) a module graph that * is require()'d. - * @param {import('../cjs/loader.js').Module} mod CJS module wrapper of the ESM. + * @param {CJSModule} mod CJS module wrapper of the ESM. * @param {string} filename Resolved filename of the module being require()'d * @param {string} source Source code. TODO(joyeecheung): pass the raw buffer. * @param {string} isMain Whether this module is a main module. - * @param {import('../cjs/loader.js').Module|undefined} parent Parent module, if any. + * @param {CJSModule|undefined} parent Parent module, if any. * @returns {{ModuleWrap}} */ importSyncForRequire(mod, filename, source, isMain, parent) { @@ -343,7 +351,7 @@ class ModuleLoader { } throw new ERR_REQUIRE_CYCLE_MODULE(message); } - // Othersie the module could be imported before but the evaluation may be already + // Otherwise the module could be imported before but the evaluation may be already // completed (e.g. the require call is lazy) so it's okay. We will return the // module now and check asynchronicity of the entire graph later, after the // graph is instantiated. @@ -352,8 +360,12 @@ class ModuleLoader { defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; const loadResult = defaultLoadSync(url, { format, importAttributes }); - const { responseURL, source } = loadResult; - const { format: finalFormat } = loadResult; + const { + format: finalFormat, + responseURL, + source, + } = loadResult; + this.validateLoadResult(url, finalFormat); if (finalFormat === 'wasm') { assert.fail('WASM is currently unsupported by require(esm)'); @@ -725,11 +737,11 @@ function getOrInitializeCascadedLoader() { /** * Register a single loader programmatically. - * @param {string|import('url').URL} specifier - * @param {string|import('url').URL} [parentURL] Base to use when resolving `specifier`; optional if + * @param {string|URL} specifier + * @param {string|URL} [parentURL] Base to use when resolving `specifier`; optional if * `specifier` is absolute. Same as `options.parentUrl`, just inline * @param {object} [options] Additional options to apply, described below. - * @param {string|import('url').URL} [options.parentURL] Base to use when resolving `specifier` + * @param {string|URL} [options.parentURL] Base to use when resolving `specifier` * @param {any} [options.data] Arbitrary data passed to the loader's `initialize` hook * @param {any[]} [options.transferList] Objects in `data` that are changing ownership * @returns {void} We want to reserve the return value for potential future extension of the API. diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 4bdee1b843fbd5..8f4b6b25d88896 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -41,6 +41,7 @@ const { dirname, extname, isAbsolute } = require('path'); const { loadBuiltinModule, stripBOM, + urlToFilename, } = require('internal/modules/helpers'); const { kIsCachedByESMLoader, @@ -243,7 +244,7 @@ function loadCJSModule(module, source, url, filename) { } } const { url: resolvedURL } = cascadedLoader.resolveSync(specifier, url, kEmptyObject); - return StringPrototypeStartsWith(resolvedURL, 'file://') ? fileURLToPath(resolvedURL) : resolvedURL; + return urlToFilename(resolvedURL); }); setOwnProperty(requireFn, 'main', process.mainModule); @@ -265,7 +266,7 @@ const cjsCache = new SafeMap(); function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) { debug(`Translating CJSModule ${url}`); - const filename = StringPrototypeStartsWith(url, 'file://') ? fileURLToPath(url) : url; + const filename = urlToFilename(url); // In case the source was not provided by the `load` step, we need fetch it now. source = stringify(source ?? getSource(new URL(url)).source);