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 7 commits
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
13 changes: 8 additions & 5 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -338,15 +338,15 @@ 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}
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved

Provides a module-relative resolution function scoped to each module, returning
the URL string.

<!-- eslint-skip -->

```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
Expand All @@ -355,7 +355,7 @@ from which to resolve from:
<!-- eslint-skip -->

```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
Expand Down Expand Up @@ -730,6 +730,9 @@ prevent unintentional breaks in the chain.

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/43363
description: Convert hook from asynchronous to asynchronous.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42623
description: Add support for chaining resolve hooks. Each hook must either
Expand Down Expand Up @@ -789,7 +792,7 @@ Node.js module specifier resolution behavior_ when calling `defaultResolve`, the
`context.conditions` array originally passed into the `resolve` hook.

```js
export async function resolve(specifier, context, nextResolve) {
export function resolve(specifier, context, nextResolve) {
const { parentURL = null } = context;

if (Math.random() > 0.5) { // Some condition.
Expand Down Expand Up @@ -1088,7 +1091,7 @@ const baseURL = pathToFileURL(`${cwd()}/`).href;
// CoffeeScript files end in .coffee, .litcoffee, or .coffee.md.
const extensionsRegex = /\.coffee$|\.litcoffee$|\.coffee\.md$/;

export async function resolve(specifier, context, nextResolve) {
export function resolve(specifier, context, nextResolve) {
if (extensionsRegex.test(specifier)) {
const { parentURL = baseURL } = context;

Expand Down
26 changes: 14 additions & 12 deletions lib/internal/modules/esm/initialize_import_meta.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,23 @@
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 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;
};
}

Expand Down
92 changes: 61 additions & 31 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ const {
ObjectDefineProperty,
ObjectSetPrototypeOf,
PromiseAll,
PromiseResolve,
PromisePrototypeThen,
ReflectApply,
RegExpPrototypeExec,
SafeArrayIterator,
Expand Down Expand Up @@ -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<HookName>(...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 {
Expand All @@ -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() {
Expand All @@ -148,21 +152,34 @@ 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}'`;

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<HookName> is actually called, not just generated.
if (generatedHookIndex === 0) { meta.chainFinished = true; }

ArrayPrototypePush(args, nextNextHook);
const output = await ReflectApply(hook, undefined, args);
let output = ReflectApply(hook, undefined, args);

if (output?.shortCircuit === true) { meta.shortCircuited = true; }
return output;
validateOutput(outputErrIdentifier, output);

function checkShortCircuited(output) {
if (output?.shortCircuit === true) { meta.shortCircuited = true; }
}

if (meta.isChainAsync) {
output = PromiseResolve(output);
PromisePrototypeThen(output, checkShortCircuited);
} else {
checkShortCircuited(output);
}

return output;
},
'name',
{ __proto__: null, value: nextHookName },
Expand Down Expand Up @@ -421,8 +438,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 @@ -557,10 +577,11 @@ class ESMLoader {
hookErrIdentifier: '',
hookIndex: chain.length - 1,
hookName: 'load',
isChainAsync: true,
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
Expand All @@ -586,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; }

Expand Down Expand Up @@ -778,7 +802,7 @@ class ESMLoader {
* statement or expression.
* @returns {{ format: string, url: URL['href'] }}
*/
async resolve(
resolve(
originalSpecifier,
parentURL,
importAssertions = ObjectCreate(null)
Expand All @@ -802,36 +826,42 @@ class ESMLoader {
hookErrIdentifier: '',
hookIndex: chain.length - 1,
hookName: 'resolve',
isChainAsync: false,
shortCircuited: false,
};

const context = {
conditions: DEFAULT_CONDITIONS,
importAssertions,
parentURL,
};
const validate = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => {

const validateArgs = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => {
validateString(
suppliedSpecifier,
`${hookErrIdentifier} specifier`,
); // non-strings can be coerced to a url string

validateObject(ctx, `${hookErrIdentifier} context`);
};
const validateOutput = (hookErrIdentifier, output) => {
if (
typeof output !== 'object' || // [2]
typeof output.then === 'function'
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
) {
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 = await nextResolve(originalSpecifier, context);
const resolution = nextResolve(originalSpecifier, context);
const { hookErrIdentifier } = meta; // Retrieve the value after all settled

if (typeof resolution !== 'object') { // [2]
throw new ERR_INVALID_RETURN_VALUE(
'an object',
hookErrIdentifier,
resolution,
);
}
validateOutput(hookErrIdentifier, resolution);

if (resolution?.shortCircuit === true) { meta.shortCircuited = true; }

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
70 changes: 41 additions & 29 deletions test/es-module/test-esm-import-meta-resolve.mjs
Original file line number Diff line number Diff line change
@@ -1,37 +1,49 @@
// 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(await import.meta.resolve('./test-esm-import-meta.mjs'),
dirname + 'test-esm-import-meta.mjs');
try {
await 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'),
fixtures + 'empty-with-bom.txt');
assert.strictEqual(await import.meta.resolve('../fixtures/'), fixtures);
assert.strictEqual(
await import.meta.resolve('../fixtures/', import.meta.url),
fixtures);
assert.strictEqual(
await import.meta.resolve('../fixtures/', new URL(import.meta.url)),
fixtures);
await 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',
);

assert.throws(
() => import.meta.resolve('./notfound.mjs'),
{ 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(await import.meta.resolve('baz/', fixtures),
fixtures + 'node_modules/baz/');
})().then(mustCall());
});

assert.strictEqual(
import.meta.resolve('baz/', fixtures),
fixtures + 'node_modules/baz/',
);
Loading