esm: convert resolve hook to synchronous

PR-URL: #43363 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
nodejs · Jun 18, 2022 · 90b634a · 90b634a
1 parent 1737c77
commit 90b634a
Show file tree

Hide file tree

Showing 30 changed files with 216 additions and 127 deletions.
diff --git a/doc/api/esm.md b/doc/api/esm.md
@@ -324,6 +324,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
@@ -339,15 +342,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}
 
 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
@@ -356,11 +363,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.
 
 ## Interoperability with CommonJS
 
@@ -731,6 +738,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
@@ -764,6 +774,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
@@ -790,7 +803,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.
@@ -1089,7 +1102,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;
 

diff --git a/lib/internal/modules/esm/initialize_import_meta.js b/lib/internal/modules/esm/initialize_import_meta.js
@@ -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))
-    );
+  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/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} 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 {
@@ -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() {
@@ -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 },
@@ -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);
 
@@ -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
@@ -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; }
 
@@ -778,7 +804,7 @@ class ESMLoader {
    *                                              statement or expression.
    * @returns {{ format: string, url: URL['href'] }}
    */
-  async resolve(
+  resolve(
     originalSpecifier,
     parentURL,
     importAssertions = ObjectCreate(null)
@@ -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; }
 

diff --git 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(