Revert "esm: convert resolve hook to synchronous"

This reverts commit 90b634a. PR-URL: #43526 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
nodejs · Jun 22, 2022 · 3c04034 · 3c04034
1 parent f5ce6b1
commit 3c04034
Show file tree

Hide file tree

Showing 30 changed files with 127 additions and 216 deletions.
diff --git a/doc/api/esm.md b/doc/api/esm.md
@@ -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
@@ -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
@@ -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
 
@@ -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
@@ -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
@@ -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.
@@ -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;
 

diff --git a/lib/internal/modules/esm/initialize_import_meta.js b/lib/internal/modules/esm/initialize_import_meta.js
@@ -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))
+    );
   };
 }
 

diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -15,8 +15,6 @@ const {
   ObjectDefineProperty,
   ObjectSetPrototypeOf,
   PromiseAll,
-  PromiseResolve,
-  PromisePrototypeThen,
   ReflectApply,
   RegExpPrototypeExec,
   SafeArrayIterator,
@@ -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 {
@@ -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() {
@@ -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 },
@@ -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);
 
@@ -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
@@ -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; }
 
@@ -804,7 +778,7 @@ class ESMLoader {
    *                                              statement or expression.
    * @returns {{ format: string, url: URL['href'] }}
    */
-  resolve(
+  async resolve(
     originalSpecifier,
     parentURL,
     importAssertions = ObjectCreate(null)
@@ -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; }
 

diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -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);
@@ -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(