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.

Sign up for GitHub

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

Jump to bottom

Revert "esm: convert resolve hook to synchronous" #43526

Merged
merged 1 commit into from Jun 22, 2022
Merged

Revert "esm: convert resolve hook to synchronous" #43526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 8 additions & 21 deletions doc/api/esm.md
View file
Open in desktop
Expand Up @@ -324,9 +324,6 @@ 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 @@ -342,19 +339,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: {string}
* Returns: {Promise}

Provides a module-relative resolution function scoped to each module, returning
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`.
the URL string.

<!-- eslint-skip -->

```js
const dependencyAsset = import.meta.resolve('component-lib/asset.css');
const dependencyAsset = await import.meta.resolve('component-lib/asset.css');
```

`import.meta.resolve` also accepts a second argument which is the parent module
Expand All @@ -363,11 +356,11 @@ from which to resolve from:
<!-- eslint-skip -->

```js
import.meta.resolve('./dep', import.meta.url);
await import.meta.resolve('./dep', import.meta.url);
```

This function is synchronous because the ES module resolver in Node.js is
synchronous.
This function is asynchronous because the ES module resolver in Node.js is
allowed to be asynchronous.

## Interoperability with CommonJS

Expand Down Expand Up @@ -738,9 +731,6 @@ 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 @@ -774,9 +764,6 @@ 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 @@ -803,7 +790,7 @@ Node.js module specifier resolution behavior_ when calling `defaultResolve`, the
`context.conditions` array originally passed into the `resolve` hook.

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

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

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

Expand Down
26 changes: 12 additions & 14 deletions lib/internal/modules/esm/initialize_import_meta.js
View file
Open in desktop
Expand Up @@ -3,23 +3,21 @@
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 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;
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))
);
};
}

Expand Down
95 changes: 31 additions & 64 deletions lib/internal/modules/esm/loader.js
View file
Open in desktop
Expand Up @@ -15,8 +15,6 @@ const {
ObjectDefineProperty,
ObjectSetPrototypeOf,
PromiseAll,
PromiseResolve,
PromisePrototypeThen,
ReflectApply,
RegExpPrototypeExec,
SafeArrayIterator,
Expand Down Expand Up @@ -111,14 +109,12 @@ 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 {object} validators A wrapper function
* @param {(hookErrIdentifier, hookArgs) => void} validate 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, { validateArgs, validateOutput }) {
function nextHookFactory(chain, meta, validate) {
// First, prepare the current
const { hookName } = meta;
const {
Expand All @@ -141,7 +137,7 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) {
// factory generates the next link in the chain.
meta.hookIndex--;

nextNextHook = nextHookFactory(chain, meta, { validateArgs, validateOutput });
nextNextHook = nextHookFactory(chain, meta, validate);
} else {
// eslint-disable-next-line func-name-matching
nextNextHook = function chainAdvancedTooFar() {
Expand All @@ -152,36 +148,21 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) {
}

return ObjectDefineProperty(
(...args) => {
async (...args) => {
// Update only when hook is invoked to avoid fingering the wrong filePath
meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`;

validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args);

const outputErrIdentifier = `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`;
validate(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args);

// Set when next<HookName> is actually called, not just generated.
if (generatedHookIndex === 0) { meta.chainFinished = true; }

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

validateOutput(outputErrIdentifier, output);
const output = await ReflectApply(hook, undefined, args);

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

return output;
}

if (meta.isChainAsync) {
return PromisePrototypeThen(
PromiseResolve(output),
checkShortCircuited,
);
}
if (output?.shortCircuit === true) { meta.shortCircuited = true; }
return output;

return checkShortCircuited(output);
},
'name',
{ __proto__: null, value: nextHookName },
Expand Down Expand Up @@ -440,11 +421,8 @@ class ESMLoader {
);
}

const { format, url } = this.resolve(
specifier,
parentURL,
importAssertionsForResolve,
);
const { format, url } =
await this.resolve(specifier, parentURL, importAssertionsForResolve);

let job = this.moduleMap.get(url, importAssertions.type);

Expand Down Expand Up @@ -579,11 +557,10 @@ class ESMLoader {
hookErrIdentifier: '',
hookIndex: chain.length - 1,
hookName: 'load',
isChainAsync: true,
shortCircuited: false,
};

const validateArgs = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => {
const validate = (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 @@ -609,22 +586,19 @@ 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, { validateArgs, validateOutput });
const nextLoad = nextHookFactory(chain, meta, validate);

const loaded = await nextLoad(url, context);
const { hookErrIdentifier } = meta; // Retrieve the value after all settled

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

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

Expand Down Expand Up @@ -804,7 +778,7 @@ class ESMLoader {
* statement or expression.
* @returns {{ format: string, url: URL['href'] }}
*/
resolve(
async resolve(
originalSpecifier,
parentURL,
importAssertions = ObjectCreate(null)
Expand All @@ -828,43 +802,36 @@ 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, { validateArgs, validateOutput });
const nextResolve = nextHookFactory(chain, meta, validate);

const resolution = nextResolve(originalSpecifier, context);
const resolution = await nextResolve(originalSpecifier, context);
const { hookErrIdentifier } = meta; // Retrieve the value after all settled

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

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

Expand Down
6 changes: 3 additions & 3 deletions lib/internal/modules/esm/resolve.js
View file
Open in desktop
Expand Up @@ -1081,7 +1081,7 @@ function throwIfUnsupportedURLScheme(parsed, experimentalNetworkImports) {
}
}

function defaultResolve(specifier, context = {}) {
async 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 = function defaultResolve(
module.exports.defaultResolve = async function defaultResolve(
specifier,
context
) {
const ret = $defaultResolve(specifier, context);
const ret = await $defaultResolve(specifier, context, $defaultResolve);
// 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