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 all 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
29 changes: 21 additions & 8 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -338,15 +341,19 @@ 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.
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 similarly to `require.resolve`.

<!-- 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,11 +362,11 @@ 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
allowed to be asynchronous.
This function is synchronous because the ES module resolver in Node.js is
synchronous.
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved

## Interoperability with CommonJS

Expand Down Expand Up @@ -730,6 +737,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 synchronous.
- 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 @@ -763,6 +773,9 @@ 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
> (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'`)
as a hint to the `load` hook. If a format is specified, the `load` hook is
Expand All @@ -789,7 +802,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 +1101,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
95 changes: 64 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} validators.validateArgs
* @param {(hookErrIdentifier, output) => void} validators.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,36 @@ 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);
const output = ReflectApply(hook, undefined, args);

validateOutput(outputErrIdentifier, output);

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

return output;
}

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

return checkShortCircuited(output);
},
'name',
{ __proto__: null, value: nextHookName },
Expand Down Expand Up @@ -421,8 +440,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 +579,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 +609,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 +804,7 @@ class ESMLoader {
* statement or expression.
* @returns {{ format: string, url: URL['href'] }}
*/
async resolve(
resolve(
originalSpecifier,
parentURL,
importAssertions = ObjectCreate(null)
Expand All @@ -802,36 +828,43 @@ 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]
output === null ||
(output.url == null && 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 = 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
Loading