Skip to content
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

esm: convert resolve hook to synchronous #43363

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions lib/internal/modules/esm/initialize_import_meta.js
Original file line number Diff line number Diff line change
Expand Up @@ -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))
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
);
return asyncESM.esmLoader.resolve(specifier, parentUrl);
};
}

Expand Down
45 changes: 34 additions & 11 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}'`;

Expand All @@ -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);
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
} else {
checkShortCircuited(output);
}

return output;
},
'name',
{ __proto__: null, value: nextHookName },
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -778,7 +789,7 @@ class ESMLoader {
* statement or expression.
* @returns {{ format: string, url: URL['href'] }}
*/
async resolve(
resolve(
originalSpecifier,
parentURL,
importAssertions = ObjectCreate(null)
Expand All @@ -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;
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

validateString(
suppliedSpecifier,
Expand All @@ -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
) {
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
throw new ERR_INVALID_RETURN_VALUE(
'an object',
hookErrIdentifier,
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/modules/esm/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
// This is a preflight check to avoid data exfiltration by query params etc.
policy.manifest.mightAllow(ret.url, () =>
new ERR_MANIFEST_DEPENDENCY_MISSING(
Expand Down
16 changes: 8 additions & 8 deletions test/es-module/test-esm-import-meta-resolve.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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',
})
)
);
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(await import.meta.resolve('baz/', fixtures),
assert.strictEqual(import.meta.resolve('baz/', fixtures),
fixtures + 'node_modules/baz/');
})().then(mustCall());
19 changes: 19 additions & 0 deletions test/es-module/test-esm-loader-chaining.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion test/es-module/test-esm-loader-search.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const {
defaultResolve: resolve
} = require('internal/modules/esm/resolve');

assert.rejects(
assert.throws(
resolve('target'),
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
{
code: 'ERR_MODULE_NOT_FOUND',
Expand Down
16 changes: 8 additions & 8 deletions test/es-module/test-esm-resolve-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand All @@ -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,
Expand Down Expand Up @@ -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 });
Expand All @@ -102,7 +102,7 @@ try {
}
};

async function testDualPackageWithJsMainScriptAndModuleType() {
function testDualPackageWithJsMainScriptAndModuleType() {
// Create a dummy dual package
//
/**
Expand Down Expand Up @@ -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'));
}
Expand All @@ -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,
Expand Down Expand Up @@ -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}`));
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions test/fixtures/es-module-loaders/hook-resolve-type.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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')) {
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/es-module-loaders/loader-invalid-format.mjs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
4 changes: 2 additions & 2 deletions test/fixtures/es-module-loaders/loader-invalid-url.mjs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
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,
url: specifier,
importAssertions,
};
}
return defaultResolve(specifier, {parentURL, importAssertions}, defaultResolve);
return nextResolve(specifier, {parentURL, importAssertions});
}
2 changes: 1 addition & 1 deletion test/fixtures/es-module-loaders/loader-resolve-42.mjs
Original file line number Diff line number Diff line change
@@ -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<HookName>:', next.name); // This log is deliberate

Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/es-module-loaders/loader-resolve-async-fn.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export async function resolve() {
return 'whatever';
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export async function resolve(specifier, context, next) {
export function resolve(specifier, context, next) {
return next(specifier, []);
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export async function resolve(specifier, context, next) {
export function resolve(specifier, context, next) {
return next([], context);
}
2 changes: 1 addition & 1 deletion test/fixtures/es-module-loaders/loader-resolve-foo.mjs
Original file line number Diff line number Diff line change
@@ -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);
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export async function resolve() {
export function resolve() {
return {
url: 'file:///incomplete-resolve-chain.js',
};
Expand Down
Original file line number Diff line number Diff line change
@@ -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',
Expand Down
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export async function resolve(specifier, context, next) {
export function resolve(specifier, context, next) {
return next(specifier, {
...context,
foo: 'bar',
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
Original file line number Diff line number Diff line change
@@ -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);
}
Loading