From 37abe52997f29f77de5a80e64de8feddeda37a72 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Mon, 6 Jun 2022 12:17:53 +0200 Subject: [PATCH 01/15] esm: convert `resolve` hook to synchronous --- .../modules/esm/initialize_import_meta.js | 12 +---- lib/internal/modules/esm/loader.js | 45 ++++++++++++++----- lib/internal/modules/esm/resolve.js | 6 +-- .../test-esm-import-meta-resolve.mjs | 16 +++---- test/es-module/test-esm-loader-chaining.mjs | 19 ++++++++ test/es-module/test-esm-loader-search.js | 2 +- test/es-module/test-esm-resolve-type.js | 16 +++---- .../builtin-named-exports-loader.mjs | 4 +- .../es-module-loaders/hook-resolve-type.mjs | 4 +- .../loader-invalid-format.mjs | 2 +- .../es-module-loaders/loader-invalid-url.mjs | 4 +- .../es-module-loaders/loader-resolve-42.mjs | 2 +- .../loader-resolve-async-fn.mjs | 3 ++ .../loader-resolve-bad-next-context.mjs | 2 +- .../loader-resolve-bad-next-specifier.mjs | 2 +- .../es-module-loaders/loader-resolve-foo.mjs | 2 +- .../loader-resolve-incomplete.mjs | 2 +- .../loader-resolve-multiple-next-calls.mjs | 6 +-- .../loader-resolve-next-modified.mjs | 4 +- ...oader-resolve-passing-modified-context.mjs | 2 +- .../loader-resolve-passthru.mjs | 2 +- ...der-resolve-receiving-modified-context.mjs | 2 +- .../loader-resolve-shortcircuit.mjs | 2 +- .../es-module-loaders/loader-shared-dep.mjs | 4 +- .../loader-with-custom-condition.mjs | 4 +- .../es-module-loaders/loader-with-dep.mjs | 4 +- .../missing-dynamic-instantiate-hook.mjs | 4 +- .../es-module-loaders/mock-loader.mjs | 4 +- .../not-found-assert-loader.mjs | 6 +-- 29 files changed, 111 insertions(+), 76 deletions(-) create mode 100644 test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs diff --git a/lib/internal/modules/esm/initialize_import_meta.js b/lib/internal/modules/esm/initialize_import_meta.js index d6be06f23e1493..3242d6028f12d4 100644 --- a/lib/internal/modules/esm/initialize_import_meta.js +++ b/lib/internal/modules/esm/initialize_import_meta.js @@ -3,21 +3,11 @@ const { getOptionValue } = require('internal/options'); const experimentalImportMetaResolve = getOptionValue('--experimental-import-meta-resolve'); -const { - PromisePrototypeThen, - PromiseReject, -} = primordials; const asyncESM = require('internal/process/esm_loader'); function createImportMetaResolve(defaultParentUrl) { return async function resolve(specifier, parentUrl = defaultParentUrl) { - return PromisePrototypeThen( - asyncESM.esmLoader.resolve(specifier, parentUrl), - ({ url }) => url, - (error) => ( - error.code === 'ERR_UNSUPPORTED_DIR_IMPORT' ? - error.url : PromiseReject(error)) - ); + return asyncESM.esmLoader.resolve(specifier, parentUrl); }; } diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index f81ffa8e31cf22..3e8d60a46f7fab 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -148,7 +148,7 @@ function nextHookFactory(chain, meta, validate) { } return ObjectDefineProperty( - async (...args) => { + (...args) => { // Update only when hook is invoked to avoid fingering the wrong filePath meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`; @@ -158,11 +158,19 @@ function nextHookFactory(chain, meta, validate) { if (generatedHookIndex === 0) { meta.chainFinished = true; } ArrayPrototypePush(args, nextNextHook); - const output = await ReflectApply(hook, undefined, args); + const output = ReflectApply(hook, undefined, args); - if (output?.shortCircuit === true) { meta.shortCircuited = true; } - return output; + function checkShortCircuited(output) { + if (output?.shortCircuit === true) { meta.shortCircuited = true; } + } + + if (output instanceof Promise) { // eslint-disable-line node-core/prefer-primordials + output?.then(checkShortCircuited); + } else { + checkShortCircuited(output); + } + return output; }, 'name', { __proto__: null, value: nextHookName }, @@ -421,8 +429,11 @@ class ESMLoader { ); } - const { format, url } = - await this.resolve(specifier, parentURL, importAssertionsForResolve); + const { format, url } = this.resolve( + specifier, + parentURL, + importAssertionsForResolve, + ); let job = this.moduleMap.get(url, importAssertions.type); @@ -778,7 +789,7 @@ class ESMLoader { * statement or expression. * @returns {{ format: string, url: URL['href'] }} */ - async resolve( + resolve( originalSpecifier, parentURL, importAssertions = ObjectCreate(null) @@ -804,13 +815,22 @@ class ESMLoader { hookName: 'resolve', shortCircuited: false, }; - const context = { conditions: DEFAULT_CONDITIONS, importAssertions, parentURL, }; - const validate = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => { + + const validate = (hookErrIdentifier, output) => { + if (output instanceof Promise) { // eslint-disable-line node-core/prefer-primordials + throw ERR_INVALID_RETURN_VALUE( + 'an object', + hookErrIdentifier, + output, + ); + } + + const { 0: suppliedSpecifier, 1: ctx } = output; validateString( suppliedSpecifier, @@ -822,10 +842,13 @@ class ESMLoader { const nextResolve = nextHookFactory(chain, meta, validate); - const resolution = await nextResolve(originalSpecifier, context); + const resolution = nextResolve(originalSpecifier, context); const { hookErrIdentifier } = meta; // Retrieve the value after all settled - if (typeof resolution !== 'object') { // [2] + if ( + typeof resolution !== 'object' || // [2] + resolution instanceof Promise // eslint-disable-line node-core/prefer-primordials + ) { throw new ERR_INVALID_RETURN_VALUE( 'an object', hookErrIdentifier, diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 79ea42233b66a0..1aacfb60753258 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -1081,7 +1081,7 @@ function throwIfUnsupportedURLScheme(parsed, experimentalNetworkImports) { } } -async function defaultResolve(specifier, context = {}) { +function defaultResolve(specifier, context = {}) { let { parentURL, conditions } = context; if (parentURL && policy?.manifest) { const redirects = policy.manifest.getDependencyMapper(parentURL); @@ -1227,11 +1227,11 @@ const { if (policy) { const $defaultResolve = defaultResolve; - module.exports.defaultResolve = async function defaultResolve( + module.exports.defaultResolve = function defaultResolve( specifier, context ) { - const ret = await $defaultResolve(specifier, context, $defaultResolve); + const ret = $defaultResolve(specifier, context); // This is a preflight check to avoid data exfiltration by query params etc. policy.manifest.mightAllow(ret.url, () => new ERR_MANIFEST_DEPENDENCY_MISSING( diff --git a/test/es-module/test-esm-import-meta-resolve.mjs b/test/es-module/test-esm-import-meta-resolve.mjs index 1fac362172ae34..daa9abf0d07ff9 100644 --- a/test/es-module/test-esm-import-meta-resolve.mjs +++ b/test/es-module/test-esm-import-meta-resolve.mjs @@ -7,31 +7,31 @@ const fixtures = dirname.slice(0, dirname.lastIndexOf('/', dirname.length - 2) + 1) + 'fixtures/'; (async () => { - assert.strictEqual(await import.meta.resolve('./test-esm-import-meta.mjs'), + assert.strictEqual(import.meta.resolve('./test-esm-import-meta.mjs'), dirname + 'test-esm-import-meta.mjs'); try { - await import.meta.resolve('./notfound.mjs'); + import.meta.resolve('./notfound.mjs'); assert.fail(); } catch (e) { assert.strictEqual(e.code, 'ERR_MODULE_NOT_FOUND'); } assert.strictEqual( - await import.meta.resolve('../fixtures/empty-with-bom.txt'), + import.meta.resolve('../fixtures/empty-with-bom.txt'), fixtures + 'empty-with-bom.txt'); - assert.strictEqual(await import.meta.resolve('../fixtures/'), fixtures); + assert.strictEqual(import.meta.resolve('../fixtures/'), fixtures); assert.strictEqual( - await import.meta.resolve('../fixtures/', import.meta.url), + import.meta.resolve('../fixtures/', import.meta.url), fixtures); assert.strictEqual( - await import.meta.resolve('../fixtures/', new URL(import.meta.url)), + import.meta.resolve('../fixtures/', new URL(import.meta.url)), fixtures); - await Promise.all( + Promise.all( [[], {}, Symbol(), 0, 1, 1n, 1.1, () => {}, true, false].map((arg) => assert.rejects(import.meta.resolve('../fixtures/', arg), { code: 'ERR_INVALID_ARG_TYPE', }) ) ); - assert.strictEqual(await import.meta.resolve('baz/', fixtures), + assert.strictEqual(import.meta.resolve('baz/', fixtures), fixtures + 'node_modules/baz/'); })().then(mustCall()); diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs index 977c20dbbbab37..ca0776a2682bb3 100644 --- a/test/es-module/test-esm-loader-chaining.mjs +++ b/test/es-module/test-esm-loader-chaining.mjs @@ -165,6 +165,25 @@ const commonArgs = [ assert.strictEqual(status, 0); } +{ // Verify error thrown for an async resolve hook + const { status, stderr, stdout } = spawnSync( + process.execPath, + [ + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-resolve-async-fn.mjs'), + ...commonArgs, + ], + { encoding: 'utf8' }, + ); + + assert.match(stderr, /ERR_INVALID_RETURN_VALUE/); + assert.match(stderr, /Promise/); + assert.match(stderr, /loader-resolve-async-fn\.mjs/); + assert.match(stderr, /'resolve'/); + assert.strictEqual(stdout, ''); + assert.strictEqual(status, 1); +} + { // Verify error thrown for incomplete resolve chain, citing errant loader & hook const { status, stderr, stdout } = spawnSync( process.execPath, diff --git a/test/es-module/test-esm-loader-search.js b/test/es-module/test-esm-loader-search.js index 0440d3d7775cff..068cadbb879404 100644 --- a/test/es-module/test-esm-loader-search.js +++ b/test/es-module/test-esm-loader-search.js @@ -10,7 +10,7 @@ const { defaultResolve: resolve } = require('internal/modules/esm/resolve'); -assert.rejects( +assert.throws( resolve('target'), { code: 'ERR_MODULE_NOT_FOUND', diff --git a/test/es-module/test-esm-resolve-type.js b/test/es-module/test-esm-resolve-type.js index 05e908cd32fc34..bf3b77c34f7769 100644 --- a/test/es-module/test-esm-resolve-type.js +++ b/test/es-module/test-esm-resolve-type.js @@ -41,10 +41,10 @@ try { [ '/es-modules/package-type-commonjs/index.js', 'commonjs' ], [ '/es-modules/package-without-type/index.js', 'commonjs' ], [ '/es-modules/package-without-pjson/index.js', 'commonjs' ], - ].forEach(async (testVariant) => { + ].forEach((testVariant) => { const [ testScript, expectedType ] = testVariant; const resolvedPath = path.resolve(fixtures.path(testScript)); - const resolveResult = await resolve(url.pathToFileURL(resolvedPath)); + const resolveResult = resolve(url.pathToFileURL(resolvedPath)); assert.strictEqual(resolveResult.format, expectedType); }); @@ -59,7 +59,7 @@ try { [ 'test-module-mainmjs', 'mjs', 'module', 'module'], [ 'test-module-cjs', 'js', 'commonjs', 'commonjs'], [ 'test-module-ne', 'js', undefined, 'commonjs'], - ].forEach(async (testVariant) => { + ].forEach((testVariant) => { const [ moduleName, moduleExtenstion, moduleType, @@ -89,7 +89,7 @@ try { fs.writeFileSync(script, 'export function esm-resolve-tester() {return 42}'); - const resolveResult = await resolve(`${moduleName}`); + const resolveResult = resolve(`${moduleName}`); assert.strictEqual(resolveResult.format, expectedResolvedType); fs.rmSync(nmDir, { recursive: true, force: true }); @@ -102,7 +102,7 @@ try { } }; - async function testDualPackageWithJsMainScriptAndModuleType() { + function testDualPackageWithJsMainScriptAndModuleType() { // Create a dummy dual package // /** @@ -172,7 +172,7 @@ try { ); // test the resolve - const resolveResult = await resolve(`${moduleName}`); + const resolveResult = resolve(`${moduleName}`); assert.strictEqual(resolveResult.format, 'module'); assert.ok(resolveResult.url.includes('my-dual-package/es/index.js')); } @@ -192,7 +192,7 @@ try { [ 'hmod', 'index.js', 'imp.js', 'commonjs', 'module', 'module', '#Key'], [ 'qhmod', 'index.js', 'imp.js', 'commonjs', 'module', 'module', '?k=v#h'], [ 'ts-mod-com', 'index.js', 'imp.ts', 'module', 'commonjs', undefined], - ].forEach(async (testVariant) => { + ].forEach((testVariant) => { const [ moduleName, mainRequireScript, @@ -240,7 +240,7 @@ try { ); // test the resolve - const resolveResult = await resolve(`${moduleName}`); + const resolveResult = resolve(`${moduleName}`); assert.strictEqual(resolveResult.format, expectedResolvedFormat); assert.ok(resolveResult.url.endsWith(`${moduleName}/subdir/${mainImportScript}${mainSuffix}`)); }); diff --git a/test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs b/test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs index e303ec196f6c6d..ab113341582b42 100644 --- a/test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs +++ b/test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs @@ -13,8 +13,8 @@ export function globalPreload() { `; } -export async function resolve(specifier, context, next) { - const def = await next(specifier, context); +export function resolve(specifier, context, next) { + const def = next(specifier, context); if (def.url.startsWith('node:')) { return { diff --git a/test/fixtures/es-module-loaders/hook-resolve-type.mjs b/test/fixtures/es-module-loaders/hook-resolve-type.mjs index 5068d6265c57b2..86e2bffe650002 100644 --- a/test/fixtures/es-module-loaders/hook-resolve-type.mjs +++ b/test/fixtures/es-module-loaders/hook-resolve-type.mjs @@ -6,8 +6,8 @@ export async function load(url, context, next) { return next(url, context, next); } -export async function resolve(specifier, context, next) { - const nextResult = await next(specifier, context); +export function resolve(specifier, context, next) { + const nextResult = next(specifier, context); const { format } = nextResult; if (format === 'module' || specifier.endsWith('.mjs')) { diff --git a/test/fixtures/es-module-loaders/loader-invalid-format.mjs b/test/fixtures/es-module-loaders/loader-invalid-format.mjs index 438d50dacba433..1521e447c5d0d5 100644 --- a/test/fixtures/es-module-loaders/loader-invalid-format.mjs +++ b/test/fixtures/es-module-loaders/loader-invalid-format.mjs @@ -1,4 +1,4 @@ -export async function resolve(specifier, { parentURL, importAssertions }, defaultResolve) { +export function resolve(specifier, { parentURL, importAssertions }, defaultResolve) { if (parentURL && specifier === '../fixtures/es-modules/test-esm-ok.mjs') { return { shortCircuit: true, diff --git a/test/fixtures/es-module-loaders/loader-invalid-url.mjs b/test/fixtures/es-module-loaders/loader-invalid-url.mjs index 87d1a6a564b461..f89d0eb23de510 100644 --- a/test/fixtures/es-module-loaders/loader-invalid-url.mjs +++ b/test/fixtures/es-module-loaders/loader-invalid-url.mjs @@ -1,4 +1,4 @@ -export async function resolve(specifier, { parentURL, importAssertions }, defaultResolve) { +export function resolve(specifier, { parentURL, importAssertions }, nextResolve) { if (parentURL && specifier === '../fixtures/es-modules/test-esm-ok.mjs') { return { shortCircuit: true, @@ -6,5 +6,5 @@ export async function resolve(specifier, { parentURL, importAssertions }, defaul importAssertions, }; } - return defaultResolve(specifier, {parentURL, importAssertions}, defaultResolve); + return nextResolve(specifier, {parentURL, importAssertions}); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-42.mjs b/test/fixtures/es-module-loaders/loader-resolve-42.mjs index 5c90bceed2b07e..23650893048835 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-42.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-42.mjs @@ -1,4 +1,4 @@ -export async function resolve(specifier, context, next) { +export function resolve(specifier, context, next) { console.log('resolve 42'); // This log is deliberate console.log('next:', next.name); // This log is deliberate diff --git a/test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs b/test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs new file mode 100644 index 00000000000000..57b7d7f96b9c2d --- /dev/null +++ b/test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs @@ -0,0 +1,3 @@ +export async function resolve() { + return 'whatever'; +} diff --git a/test/fixtures/es-module-loaders/loader-resolve-bad-next-context.mjs b/test/fixtures/es-module-loaders/loader-resolve-bad-next-context.mjs index 881f5875dd0206..1e42a6d6ddf665 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-bad-next-context.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-bad-next-context.mjs @@ -1,3 +1,3 @@ -export async function resolve(specifier, context, next) { +export function resolve(specifier, context, next) { return next(specifier, []); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-bad-next-specifier.mjs b/test/fixtures/es-module-loaders/loader-resolve-bad-next-specifier.mjs index a23785d3d956db..a01d40ad31f1c3 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-bad-next-specifier.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-bad-next-specifier.mjs @@ -1,3 +1,3 @@ -export async function resolve(specifier, context, next) { +export function resolve(specifier, context, next) { return next([], context); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-foo.mjs b/test/fixtures/es-module-loaders/loader-resolve-foo.mjs index 595385e12a0cf7..d19e9c271b293f 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-foo.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-foo.mjs @@ -1,4 +1,4 @@ -export async function resolve(specifier, context, next) { +export function resolve(specifier, context, next) { console.log('resolve foo'); // This log is deliberate return next('file:///foo.mjs', context); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-incomplete.mjs b/test/fixtures/es-module-loaders/loader-resolve-incomplete.mjs index 9eb1617f30130e..d71dc48249c9d8 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-incomplete.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-incomplete.mjs @@ -1,4 +1,4 @@ -export async function resolve() { +export function resolve() { return { url: 'file:///incomplete-resolve-chain.js', }; diff --git a/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs b/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs index 88d333c2404a3c..7bbc9ea08fffcf 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-multiple-next-calls.mjs @@ -1,6 +1,6 @@ -export async function resolve(specifier, context, next) { - const { url: first } = await next(specifier, context); - const { url: second } = await next(specifier, context); +export function resolve(specifier, context, next) { + const { url: first } = next(specifier, context); + const { url: second } = next(specifier, context); return { format: 'module', diff --git a/test/fixtures/es-module-loaders/loader-resolve-next-modified.mjs b/test/fixtures/es-module-loaders/loader-resolve-next-modified.mjs index a973345a82ff21..8b0d24016b3fe7 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-next-modified.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-next-modified.mjs @@ -1,8 +1,8 @@ -export async function resolve(url, context, next) { +export function resolve(url, context, next) { const { format, url: nextUrl, - } = await next(url, context); + } = next(url, context); return { format, diff --git a/test/fixtures/es-module-loaders/loader-resolve-passing-modified-context.mjs b/test/fixtures/es-module-loaders/loader-resolve-passing-modified-context.mjs index 6a92a6cd8f6a8e..1a8a7768ebccc7 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-passing-modified-context.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-passing-modified-context.mjs @@ -1,4 +1,4 @@ -export async function resolve(specifier, context, next) { +export function resolve(specifier, context, next) { return next(specifier, { ...context, foo: 'bar', diff --git a/test/fixtures/es-module-loaders/loader-resolve-passthru.mjs b/test/fixtures/es-module-loaders/loader-resolve-passthru.mjs index 1a373bab90ba57..0ce3fdf2eef941 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-passthru.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-passthru.mjs @@ -1,4 +1,4 @@ -export async function resolve(specifier, context, next) { +export function resolve(specifier, context, next) { console.log('resolve passthru'); // This log is deliberate return next(specifier, context); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-receiving-modified-context.mjs b/test/fixtures/es-module-loaders/loader-resolve-receiving-modified-context.mjs index 83aa83104e96e4..cf651eb57d2ac7 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-receiving-modified-context.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-receiving-modified-context.mjs @@ -1,4 +1,4 @@ -export async function resolve(specifier, context, next) { +export function resolve(specifier, context, next) { console.log(context.foo); // This log is deliberate return next(specifier, context); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-shortcircuit.mjs b/test/fixtures/es-module-loaders/loader-resolve-shortcircuit.mjs index d886b3dfcbf237..ff0346a34c2e37 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-shortcircuit.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-shortcircuit.mjs @@ -1,4 +1,4 @@ -export async function resolve(specifier) { +export function resolve(specifier) { return { shortCircuit: true, url: specifier, diff --git a/test/fixtures/es-module-loaders/loader-shared-dep.mjs b/test/fixtures/es-module-loaders/loader-shared-dep.mjs index 387575794c00dc..4a893686302bfb 100644 --- a/test/fixtures/es-module-loaders/loader-shared-dep.mjs +++ b/test/fixtures/es-module-loaders/loader-shared-dep.mjs @@ -5,7 +5,7 @@ import { createRequire } from '../../common/index.mjs'; const require = createRequire(import.meta.url); const dep = require('./loader-dep.js'); -export function resolve(specifier, { parentURL, importAssertions }, defaultResolve) { +export function resolve(specifier, { parentURL, importAssertions }, nextResolve) { assert.strictEqual(dep.format, 'module'); - return defaultResolve(specifier, { parentURL, importAssertions }, defaultResolve); + return nextResolve(specifier, { parentURL, importAssertions }); } diff --git a/test/fixtures/es-module-loaders/loader-with-custom-condition.mjs b/test/fixtures/es-module-loaders/loader-with-custom-condition.mjs index 3aefed51d57d3e..e8d1c638b26eff 100644 --- a/test/fixtures/es-module-loaders/loader-with-custom-condition.mjs +++ b/test/fixtures/es-module-loaders/loader-with-custom-condition.mjs @@ -1,6 +1,6 @@ import { ok, deepStrictEqual } from 'assert'; -export async function resolve(specifier, context, defaultResolve) { +export function resolve(specifier, context, nextResolve) { ok(Array.isArray(context.conditions), 'loader receives conditions array'); deepStrictEqual([...context.conditions].sort(), [ @@ -9,7 +9,7 @@ export async function resolve(specifier, context, defaultResolve) { 'node-addons', ]); - return defaultResolve(specifier, { + return nextResolve(specifier, { ...context, conditions: ['custom-condition', ...context.conditions], }); diff --git a/test/fixtures/es-module-loaders/loader-with-dep.mjs b/test/fixtures/es-module-loaders/loader-with-dep.mjs index 1b5fd6c3c1642a..8a0d57e907b2ae 100644 --- a/test/fixtures/es-module-loaders/loader-with-dep.mjs +++ b/test/fixtures/es-module-loaders/loader-with-dep.mjs @@ -3,9 +3,9 @@ import {createRequire} from '../../common/index.mjs'; const require = createRequire(import.meta.url); const dep = require('./loader-dep.js'); -export async function resolve(specifier, { parentURL, importAssertions }, defaultResolve) { +export function resolve(specifier, { parentURL, importAssertions }, nextResolve) { return { - url: (await defaultResolve(specifier, { parentURL, importAssertions }, defaultResolve)).url, + url: (nextResolve(specifier, { parentURL, importAssertions })).url, format: dep.format }; } diff --git a/test/fixtures/es-module-loaders/missing-dynamic-instantiate-hook.mjs b/test/fixtures/es-module-loaders/missing-dynamic-instantiate-hook.mjs index ec15eb0bb8fc24..b0e41dd3e5f433 100644 --- a/test/fixtures/es-module-loaders/missing-dynamic-instantiate-hook.mjs +++ b/test/fixtures/es-module-loaders/missing-dynamic-instantiate-hook.mjs @@ -1,10 +1,10 @@ -export function resolve(specifier, { parentURL }, defaultResolve) { +export function resolve(specifier, { parentURL }, nextResolve) { if (specifier === 'test') { return { url: 'file://' }; } - return defaultResolve(specifier, {parentURL}, defaultResolve); + return nextResolve(specifier, {parentURL}); } export function getFormat(url, context, defaultGetFormat) { diff --git a/test/fixtures/es-module-loaders/mock-loader.mjs b/test/fixtures/es-module-loaders/mock-loader.mjs index 062be39603e851..f0b1fdf9103c92 100644 --- a/test/fixtures/es-module-loaders/mock-loader.mjs +++ b/test/fixtures/es-module-loaders/mock-loader.mjs @@ -168,7 +168,7 @@ export function globalPreload({port}) { // Rewrites node: loading to mock-facade: so that it can be intercepted -export async function resolve(specifier, context, defaultResolve) { +export function resolve(specifier, context, nextResolve) { if (specifier === 'node:mock') { return { shortCircuit: true, @@ -176,7 +176,7 @@ export async function resolve(specifier, context, defaultResolve) { }; } doDrainPort(); - const def = await defaultResolve(specifier, context); + const def = nextResolve(specifier, context); if (context.parentURL?.startsWith('mock-facade:')) { // Do nothing, let it get the "real" module } else if (mockedModuleExports.has(def.url)) { diff --git a/test/fixtures/es-module-loaders/not-found-assert-loader.mjs b/test/fixtures/es-module-loaders/not-found-assert-loader.mjs index 5213ddedb34e8d..611354fadc3aba 100644 --- a/test/fixtures/es-module-loaders/not-found-assert-loader.mjs +++ b/test/fixtures/es-module-loaders/not-found-assert-loader.mjs @@ -3,13 +3,13 @@ import assert from 'assert'; // a loader that asserts that the defaultResolve will throw "not found" // (skipping the top-level main of course) let mainLoad = true; -export async function resolve(specifier, { parentURL, importAssertions }, defaultResolve) { +export function resolve(specifier, { parentURL, importAssertions }, nextResolve) { if (mainLoad) { mainLoad = false; - return defaultResolve(specifier, {parentURL, importAssertions}, defaultResolve); + return nextResolve(specifier, {parentURL, importAssertions}); } try { - await defaultResolve(specifier, {parentURL, importAssertions}, defaultResolve); + nextResolve(specifier, {parentURL, importAssertions}); } catch (e) { assert.strictEqual(e.code, 'ERR_MODULE_NOT_FOUND'); From edb6ba5b52befda224cc627bc60f6028e1dd5b31 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Sun, 12 Jun 2022 14:56:47 +0200 Subject: [PATCH 02/15] fixup: fix import.meta.resolve --- doc/api/esm.md | 6 +- .../modules/esm/initialize_import_meta.js | 16 ++++- .../test-esm-import-meta-resolve.mjs | 72 +++++++++++-------- 3 files changed, 60 insertions(+), 34 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 1207a00bc6a9e4..07c22425466c74 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -338,7 +338,7 @@ command flag enabled. * `specifier` {string} The module specifier to resolve relative to `parent`. * `parent` {string|URL} The absolute parent module URL to resolve from. If none is specified, the value of `import.meta.url` is used as the default. -* Returns: {Promise} +* Returns: {string} Provides a module-relative resolution function scoped to each module, returning the URL string. @@ -346,7 +346,7 @@ the URL string. ```js -const dependencyAsset = await import.meta.resolve('component-lib/asset.css'); +const dependencyAsset = import.meta.resolve('component-lib/asset.css'); ``` `import.meta.resolve` also accepts a second argument which is the parent module @@ -355,7 +355,7 @@ from which to resolve from: ```js -await import.meta.resolve('./dep', import.meta.url); +import.meta.resolve('./dep', import.meta.url); ``` This function is asynchronous because the ES module resolver in Node.js is diff --git a/lib/internal/modules/esm/initialize_import_meta.js b/lib/internal/modules/esm/initialize_import_meta.js index 3242d6028f12d4..d8e8fe587c413e 100644 --- a/lib/internal/modules/esm/initialize_import_meta.js +++ b/lib/internal/modules/esm/initialize_import_meta.js @@ -6,8 +6,20 @@ const experimentalImportMetaResolve = const asyncESM = require('internal/process/esm_loader'); function createImportMetaResolve(defaultParentUrl) { - return async function resolve(specifier, parentUrl = defaultParentUrl) { - return asyncESM.esmLoader.resolve(specifier, parentUrl); + return function resolve(specifier, parentUrl = defaultParentUrl) { + let url; + + try { + ({ url } = asyncESM.esmLoader.resolve(specifier, parentUrl)); + } catch (error) { + if (error.code === 'ERR_UNSUPPORTED_DIR_IMPORT') { + ({ url } = error); + } else { + throw error; + } + } + + return url; }; } diff --git a/test/es-module/test-esm-import-meta-resolve.mjs b/test/es-module/test-esm-import-meta-resolve.mjs index daa9abf0d07ff9..60d7755296172d 100644 --- a/test/es-module/test-esm-import-meta-resolve.mjs +++ b/test/es-module/test-esm-import-meta-resolve.mjs @@ -1,37 +1,51 @@ // Flags: --experimental-import-meta-resolve -import { mustCall } from '../common/index.mjs'; +import '../common/index.mjs'; import assert from 'assert'; const dirname = import.meta.url.slice(0, import.meta.url.lastIndexOf('/') + 1); const fixtures = dirname.slice(0, dirname.lastIndexOf('/', dirname.length - 2) + 1) + 'fixtures/'; -(async () => { - assert.strictEqual(import.meta.resolve('./test-esm-import-meta.mjs'), - dirname + 'test-esm-import-meta.mjs'); - try { - import.meta.resolve('./notfound.mjs'); - assert.fail(); - } catch (e) { - assert.strictEqual(e.code, 'ERR_MODULE_NOT_FOUND'); - } - assert.strictEqual( - import.meta.resolve('../fixtures/empty-with-bom.txt'), - fixtures + 'empty-with-bom.txt'); - assert.strictEqual(import.meta.resolve('../fixtures/'), fixtures); - assert.strictEqual( - import.meta.resolve('../fixtures/', import.meta.url), - fixtures); - assert.strictEqual( - import.meta.resolve('../fixtures/', new URL(import.meta.url)), - fixtures); - Promise.all( - [[], {}, Symbol(), 0, 1, 1n, 1.1, () => {}, true, false].map((arg) => - assert.rejects(import.meta.resolve('../fixtures/', arg), { - code: 'ERR_INVALID_ARG_TYPE', - }) - ) +assert.strictEqual( + import.meta.resolve('./test-esm-import-meta.mjs'), + dirname + 'test-esm-import-meta.mjs', +); + +try { + import.meta.resolve('./notfound.mjs'); + assert.fail(); +} catch (e) { + assert.strictEqual(e.code, 'ERR_MODULE_NOT_FOUND'); +} + +assert.strictEqual( + import.meta.resolve('../fixtures/empty-with-bom.txt'), + fixtures + 'empty-with-bom.txt', +); + +assert.strictEqual( + import.meta.resolve('../fixtures/'), + fixtures, +); + +assert.strictEqual( + import.meta.resolve('../fixtures/', import.meta.url), + fixtures, +); + +assert.strictEqual( + import.meta.resolve('../fixtures/', new URL(import.meta.url)), + fixtures, +); + +[[], {}, Symbol(), 0, 1, 1n, 1.1, () => {}, true, false].forEach((arg) => { + assert.throws( + () => import.meta.resolve('../fixtures/', arg), + { code: 'ERR_INVALID_ARG_TYPE' }, ); - assert.strictEqual(import.meta.resolve('baz/', fixtures), - fixtures + 'node_modules/baz/'); -})().then(mustCall()); +}); + +assert.strictEqual( + import.meta.resolve('baz/', fixtures), + fixtures + 'node_modules/baz/', +); From ae8ef630109e4bd1810f2f6d9f33c505efde65df Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Sun, 12 Jun 2022 16:10:30 +0200 Subject: [PATCH 03/15] fixup: correct `assert.throws()` --- test/es-module/test-esm-import-meta-resolve.mjs | 10 ++++------ test/es-module/test-esm-loader-search.js | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/test/es-module/test-esm-import-meta-resolve.mjs b/test/es-module/test-esm-import-meta-resolve.mjs index 60d7755296172d..70abe272ca0421 100644 --- a/test/es-module/test-esm-import-meta-resolve.mjs +++ b/test/es-module/test-esm-import-meta-resolve.mjs @@ -11,12 +11,10 @@ assert.strictEqual( dirname + 'test-esm-import-meta.mjs', ); -try { - import.meta.resolve('./notfound.mjs'); - assert.fail(); -} catch (e) { - assert.strictEqual(e.code, 'ERR_MODULE_NOT_FOUND'); -} +assert.throws( + () => import.meta.resolve('./notfound.mjs'), + { code: 'ERR_MODULE_NOT_FOUND' }, +); assert.strictEqual( import.meta.resolve('../fixtures/empty-with-bom.txt'), diff --git a/test/es-module/test-esm-loader-search.js b/test/es-module/test-esm-loader-search.js index 068cadbb879404..3c451409b356db 100644 --- a/test/es-module/test-esm-loader-search.js +++ b/test/es-module/test-esm-loader-search.js @@ -11,7 +11,7 @@ const { } = require('internal/modules/esm/resolve'); assert.throws( - resolve('target'), + () => resolve('target'), { code: 'ERR_MODULE_NOT_FOUND', name: 'Error', From 8bf7965fd99f7824e2b1132649c6d85173ebd2a4 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Sun, 12 Jun 2022 15:39:37 +0200 Subject: [PATCH 04/15] fixup: support detecting when a resolve hook returns a promise --- lib/internal/modules/esm/loader.js | 30 ++++++++++++------- test/es-module/test-esm-loader-chaining.mjs | 11 +++++-- .../loader-resolve-async-fn.mjs | 4 +-- 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 3e8d60a46f7fab..6897c0e9bbfae2 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -164,8 +164,22 @@ function nextHookFactory(chain, meta, validate) { if (output?.shortCircuit === true) { meta.shortCircuited = true; } } - if (output instanceof Promise) { // eslint-disable-line node-core/prefer-primordials - output?.then(checkShortCircuited); + const then = output?.then; + if (typeof then === 'function') { + if (!meta.isChainAsync) { + throw ERR_INVALID_RETURN_VALUE( + 'an object', + // MUST use generatedHookIndex because the chain has already advanced, + // causing meta.hookIndex to advance + `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`, + output, + ); + } + + ReflectApply(then, output, [ + checkShortCircuited, + // TODO: handle error case + ]); } else { checkShortCircuited(output); } @@ -568,6 +582,7 @@ class ESMLoader { hookErrIdentifier: '', hookIndex: chain.length - 1, hookName: 'load', + isChainAsync: true, shortCircuited: false, }; @@ -813,6 +828,7 @@ class ESMLoader { hookErrIdentifier: '', hookIndex: chain.length - 1, hookName: 'resolve', + isChainAsync: false, shortCircuited: false, }; const context = { @@ -822,14 +838,6 @@ class ESMLoader { }; const validate = (hookErrIdentifier, output) => { - if (output instanceof Promise) { // eslint-disable-line node-core/prefer-primordials - throw ERR_INVALID_RETURN_VALUE( - 'an object', - hookErrIdentifier, - output, - ); - } - const { 0: suppliedSpecifier, 1: ctx } = output; validateString( @@ -847,7 +855,7 @@ class ESMLoader { if ( typeof resolution !== 'object' || // [2] - resolution instanceof Promise // eslint-disable-line node-core/prefer-primordials + typeof resolution?.then === 'function' ) { throw new ERR_INVALID_RETURN_VALUE( 'an object', diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs index ca0776a2682bb3..7bfe00f5ede842 100644 --- a/test/es-module/test-esm-loader-chaining.mjs +++ b/test/es-module/test-esm-loader-chaining.mjs @@ -166,11 +166,17 @@ const commonArgs = [ } { // Verify error thrown for an async resolve hook - const { status, stderr, stdout } = spawnSync( + const { status, stderr } = spawnSync( process.execPath, [ + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-resolve-shortcircuit.mjs'), + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-resolve-42.mjs'), '--loader', fixtures.fileURL('es-module-loaders', 'loader-resolve-async-fn.mjs'), + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-load-foo-or-42.mjs'), ...commonArgs, ], { encoding: 'utf8' }, @@ -180,7 +186,8 @@ const commonArgs = [ assert.match(stderr, /Promise/); assert.match(stderr, /loader-resolve-async-fn\.mjs/); assert.match(stderr, /'resolve'/); - assert.strictEqual(stdout, ''); + // Cannot expect stdout to be empty because detecting whether a hook has + // returned a promise requires the hook to be executed assert.strictEqual(status, 1); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs b/test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs index 57b7d7f96b9c2d..6cf60d403bbfb7 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs @@ -1,3 +1,3 @@ -export async function resolve() { - return 'whatever'; +export async function resolve(specifier, context, next) { + return next(specifier, context); } From ab935b6fda8a6522634ebc66106eabfc7320ca69 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Sun, 12 Jun 2022 15:50:25 +0200 Subject: [PATCH 05/15] fixup: change async/thenable detection --- lib/internal/modules/esm/loader.js | 85 +++++++++++++++--------------- 1 file changed, 43 insertions(+), 42 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 6897c0e9bbfae2..da43474a5724c2 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -15,6 +15,8 @@ const { ObjectDefineProperty, ObjectSetPrototypeOf, PromiseAll, + PromiseResolve, + PromisePrototypeThen, ReflectApply, RegExpPrototypeExec, SafeArrayIterator, @@ -109,12 +111,14 @@ let emittedSpecifierResolutionWarning = false; * position in the hook chain. * @param {string} meta.hookName The kind of hook the chain is (ex 'resolve') * @param {boolean} meta.shortCircuited Whether a hook signaled a short-circuit. - * @param {(hookErrIdentifier, hookArgs) => void} validate A wrapper function + * @param {object} validators A wrapper function * containing all validation of a custom loader hook's intermediary output. Any * validation within MUST throw. + * @param {(hookErrIdentifier, hookArgs) => void} validateArgs + * @param {(hookErrIdentifier, output) => void} validateOutput * @returns {function next(...hookArgs)} The next hook in the chain. */ -function nextHookFactory(chain, meta, validate) { +function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { // First, prepare the current const { hookName } = meta; const { @@ -137,7 +141,7 @@ function nextHookFactory(chain, meta, validate) { // factory generates the next link in the chain. meta.hookIndex--; - nextNextHook = nextHookFactory(chain, meta, validate); + nextNextHook = nextHookFactory(chain, meta, { validateArgs, validateOutput }); } else { // eslint-disable-next-line func-name-matching nextNextHook = function chainAdvancedTooFar() { @@ -152,34 +156,25 @@ function nextHookFactory(chain, meta, validate) { // Update only when hook is invoked to avoid fingering the wrong filePath meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`; - validate(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args); + validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args); + + const outputErrIdentifier = `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`; // Set when next is actually called, not just generated. if (generatedHookIndex === 0) { meta.chainFinished = true; } ArrayPrototypePush(args, nextNextHook); - const output = ReflectApply(hook, undefined, args); + let output = ReflectApply(hook, undefined, args); + + validateOutput(outputErrIdentifier, output); function checkShortCircuited(output) { if (output?.shortCircuit === true) { meta.shortCircuited = true; } } - const then = output?.then; - if (typeof then === 'function') { - if (!meta.isChainAsync) { - throw ERR_INVALID_RETURN_VALUE( - 'an object', - // MUST use generatedHookIndex because the chain has already advanced, - // causing meta.hookIndex to advance - `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`, - output, - ); - } - - ReflectApply(then, output, [ - checkShortCircuited, - // TODO: handle error case - ]); + if (meta.isChainAsync) { + output = PromiseResolve(output); + PromisePrototypeThen(output, checkShortCircuited); } else { checkShortCircuited(output); } @@ -586,7 +581,7 @@ class ESMLoader { shortCircuited: false, }; - const validate = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => { + const validateArgs = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => { if (typeof nextUrl !== 'string') { // non-strings can be coerced to a url string // validateString() throws a less-specific error @@ -612,19 +607,22 @@ class ESMLoader { validateObject(ctx, `${hookErrIdentifier} context`); }; + const validateOutput = (hookErrIdentifier, output) => { + if (typeof output !== 'object') { // [2] + throw new ERR_INVALID_RETURN_VALUE( + 'an object', + hookErrIdentifier, + output, + ); + } + }; - const nextLoad = nextHookFactory(chain, meta, validate); + const nextLoad = nextHookFactory(chain, meta, { validateArgs, validateOutput }); const loaded = await nextLoad(url, context); const { hookErrIdentifier } = meta; // Retrieve the value after all settled - if (typeof loaded !== 'object') { // [2] - throw new ERR_INVALID_RETURN_VALUE( - 'an object', - hookErrIdentifier, - loaded, - ); - } + validateOutput(hookErrIdentifier, loaded); if (loaded?.shortCircuit === true) { meta.shortCircuited = true; } @@ -837,7 +835,7 @@ class ESMLoader { parentURL, }; - const validate = (hookErrIdentifier, output) => { + const validateArgs = (hookErrIdentifier, output) => { const { 0: suppliedSpecifier, 1: ctx } = output; validateString( @@ -847,22 +845,25 @@ class ESMLoader { validateObject(ctx, `${hookErrIdentifier} context`); }; + const validateOutput = (hookErrIdentifier, output) => { + if ( + typeof output !== 'object' || // [2] + typeof output.then === 'function' + ) { + throw new ERR_INVALID_RETURN_VALUE( + 'an object', + hookErrIdentifier, + output, + ); + } + }; - const nextResolve = nextHookFactory(chain, meta, validate); + const nextResolve = nextHookFactory(chain, meta, { validateArgs, validateOutput }); const resolution = nextResolve(originalSpecifier, context); const { hookErrIdentifier } = meta; // Retrieve the value after all settled - if ( - typeof resolution !== 'object' || // [2] - typeof resolution?.then === 'function' - ) { - throw new ERR_INVALID_RETURN_VALUE( - 'an object', - hookErrIdentifier, - resolution, - ); - } + validateOutput(hookErrIdentifier, resolution); if (resolution?.shortCircuit === true) { meta.shortCircuited = true; } From 28d813f0b0bea2a8645c73d821326db761cc1d77 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Sun, 12 Jun 2022 20:44:52 +0200 Subject: [PATCH 06/15] fixup: update ESM doc for resolve hook --- doc/api/esm.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 07c22425466c74..62ecde19943b31 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -730,6 +730,9 @@ prevent unintentional breaks in the chain. ```js @@ -766,6 +769,9 @@ changes: terminate the chain of `resolve` hooks. **Default:** `false` * `url` {string} The absolute URL to which this input resolves +> **Caveat** The `defaultResolveHook()` contains synchronous file-system +> operations, which can impact performance. + The `resolve` hook chain is responsible for resolving file URL for a given module specifier and parent URL, and optionally its format (such as `'module'`) as a hint to the `load` hook. If a format is specified, the `load` hook is diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 5e389db65588ad..3c64dd6220d1cb 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -174,7 +174,7 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { if (meta.isChainAsync) { output = PromiseResolve(output); - PromisePrototypeThen(output, checkShortCircuited); + PromisePrototypeThen(output, checkShortCircuited, () => {}); } else { checkShortCircuited(output); } From 6c8de269ed5f69e6d33f2db7f3abdd3ff156e389 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Wed, 15 Jun 2022 12:27:58 +0200 Subject: [PATCH 09/15] fixup: wordsmith resolve hook perf caveat --- doc/api/esm.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 24d052a617ad15..bf2337fc53b5a0 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -769,8 +769,8 @@ changes: terminate the chain of `resolve` hooks. **Default:** `false` * `url` {string} The absolute URL to which this input resolves -> **Caveat** The `defaultResolveHook()` contains synchronous file-system -> operations, which can impact performance. +> **Caveat** A resolve hook can contain synchronous file-system operations +> (as `defaultResolveHook()` does), which can impact performance. The `resolve` hook chain is responsible for resolving file URL for a given module specifier and parent URL, and optionally its format (such as `'module'`) From 6416e1dc302dd12d87dd33ae4dd95def79fdeb36 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Wed, 15 Jun 2022 12:29:09 +0200 Subject: [PATCH 10/15] fixup: re-connect async hook promise chain --- lib/internal/modules/esm/loader.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 3c64dd6220d1cb..27ed963af14faf 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -170,16 +170,18 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { function checkShortCircuited(output) { if (output?.shortCircuit === true) { meta.shortCircuited = true; } + + return output; } if (meta.isChainAsync) { - output = PromiseResolve(output); - PromisePrototypeThen(output, checkShortCircuited, () => {}); - } else { - checkShortCircuited(output); + return PromisePrototypeThen( + PromiseResolve(output), + checkShortCircuited, + ); } - return output; + return checkShortCircuited(output); }, 'name', { __proto__: null, value: nextHookName }, From f02b8699e2a917b91819c26eea2f7d2fc214331b Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Wed, 15 Jun 2022 13:41:18 +0200 Subject: [PATCH 11/15] fixup: de-lint --- lib/internal/modules/esm/loader.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 27ed963af14faf..d5a2682706d974 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -114,8 +114,8 @@ let emittedSpecifierResolutionWarning = false; * @param {object} validators A wrapper function * containing all validation of a custom loader hook's intermediary output. Any * validation within MUST throw. - * @param {(hookErrIdentifier, hookArgs) => void} validateArgs - * @param {(hookErrIdentifier, output) => void} validateOutput + * @param {(hookErrIdentifier, hookArgs) => void} validators.validateArgs + * @param {(hookErrIdentifier, output) => void} validators.validateOutput * @returns {function next(...hookArgs)} The next hook in the chain. */ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { @@ -164,7 +164,7 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { if (generatedHookIndex === 0) { meta.chainFinished = true; } ArrayPrototypePush(args, nextNextHook); - let output = ReflectApply(hook, undefined, args); + const output = ReflectApply(hook, undefined, args); validateOutput(outputErrIdentifier, output); From 57d66864cf562ac6091b9509846185096280de20 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Wed, 15 Jun 2022 18:31:58 +0200 Subject: [PATCH 12/15] fixup: correct missed doc refs to `asynchronous` --- doc/api/esm.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index bf2337fc53b5a0..a814f3aac38cc2 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -361,8 +361,8 @@ from which to resolve from: import.meta.resolve('./dep', import.meta.url); ``` -This function is asynchronous because the ES module resolver in Node.js is -allowed to be asynchronous. +This function is synchronous because the ES module resolver in Node.js is +synchronous. ## Interoperability with CommonJS @@ -735,7 +735,7 @@ prevent unintentional breaks in the chain. changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/43363 - description: Convert hook from asynchronous to asynchronous. + description: Convert hook from asynchronous to synchronous. - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/42623 description: Add support for chaining resolve hooks. Each hook must either From 539f7a5505efb1c69b2c845810b2cab83df28fa2 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 17 Jun 2022 11:30:49 +0200 Subject: [PATCH 13/15] fixup: add browser-alignment note to `import.meta.resolve()` --- doc/api/esm.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index a814f3aac38cc2..f25c568d7a16b1 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -323,6 +323,9 @@ added: - v13.9.0 - v12.16.2 changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/43363 + description: Convert from asynchronous to synchronous. - version: - v16.2.0 - v14.18.0 @@ -341,7 +344,8 @@ command flag enabled. * Returns: {string} Provides a module-relative resolution function scoped to each module, returning -the URL string. +the URL string. In alignment with browser behaviour, this now returns +synchronously. > **Caveat** This can result in synchronous file-system operations, which > can impact performance. From 83c2072c05fd303cc41c5f561a52c6f684daa82d Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 17 Jun 2022 11:39:49 +0200 Subject: [PATCH 14/15] fixup: account for `null` hook return & first assert `url` is nullish --- lib/internal/modules/esm/loader.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d5a2682706d974..ef9b32f4deb8ed 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -848,7 +848,8 @@ class ESMLoader { const validateOutput = (hookErrIdentifier, output) => { if ( typeof output !== 'object' || // [2] - typeof output.then === 'function' + output === null || + (output.url == null && typeof output.then === 'function') ) { throw new ERR_INVALID_RETURN_VALUE( 'an object', From e0511b60b3a740b5262f535ecf2c7147fbeb0cd2 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 17 Jun 2022 16:58:54 -0400 Subject: [PATCH 15/15] fixup: tidy docs changes Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com> --- doc/api/esm.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index f25c568d7a16b1..09d8bbb08fbd14 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -344,11 +344,11 @@ command flag enabled. * Returns: {string} Provides a module-relative resolution function scoped to each module, returning -the URL string. In alignment with browser behaviour, this now returns +the URL string. In alignment with browser behavior, this now returns synchronously. > **Caveat** This can result in synchronous file-system operations, which -> can impact performance. +> can impact performance similarly to `require.resolve`. @@ -773,7 +773,7 @@ changes: terminate the chain of `resolve` hooks. **Default:** `false` * `url` {string} The absolute URL to which this input resolves -> **Caveat** A resolve hook can contain synchronous file-system operations +> **Caveat** A resolve hook can contain synchronous file-system operations > (as `defaultResolveHook()` does), which can impact performance. The `resolve` hook chain is responsible for resolving file URL for a given