fixup: Apply suggestions from code review

Co-authored-by: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
nodejs · Mar 1, 2022 · 80cf47c · 80cf47c
1 parent 75f2e05
commit 80cf47c
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 16 deletions.
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -1017,8 +1017,10 @@ function wrapSafe(filename, content, cjsModuleInstance) {
       displayErrors: true,
       importModuleDynamically: (specifier, _, importAssertions) => {
         const loader = asyncESM.esmLoader;
-        return loader.import(specifier,
-                             loader.baseURL(normalizeReferrerURL(filename)),
+        return loader.import(
+          specifier,
+          loader.baseURL(normalizeReferrerURL(filename),
+        ),
                              importAssertions);
       },
     });
@@ -1034,8 +1036,10 @@ function wrapSafe(filename, content, cjsModuleInstance) {
       filename,
       importModuleDynamically(specifier, _, importAssertions) {
         const loader = asyncESM.esmLoader;
-        return loader.import(specifier,
-                             loader.baseURL(normalizeReferrerURL(filename)),
+        return loader.import(
+          specifier,
+          loader.baseURL(normalizeReferrerURL(filename),
+        ),
                              importAssertions);
       },
     });

diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -215,8 +215,9 @@ class ESMLoader {
       callbackMap.set(module, {
         importModuleDynamically: (specifier, { url }, importAssertions) => {
           return this.import(specifier,
-                             this.baseURL(url),
-                             importAssertions);
+            this.baseURL(url),
+            importAssertions,
+          );
         }
       });
 
@@ -236,21 +237,21 @@ class ESMLoader {
    * Returns the url to use for the resolution of a given cache key url
    * These are not guaranteed to be the same.
    *
-   * In WHATWG HTTP spec for ESM the cache key is the non-I/O bound
+   * In the WHATWG HTTP spec for ESM, the cache key is the non-I/O bound,
    * synchronous resolution using only string operations
    *   ~= resolveImportMap(new URL(specifier, importerHREF))
    *
-   * The url used for subsequent resolution is the response URL after
+   * The url used for subsequent resolution is the response URL *after*
    * all redirects have been resolved.
    *
-   * https://example.com/foo redirecting to https://example.com/bar
-   * would have a cache key of https://example.com/foo and baseURL
-   * of https://example.com/bar
+   * https://example.com/foo redirects to https://example.com/bar
+   * cache key = https://example.com/foo
+   * baseURL   = https://example.com/bar
    *
    * MUST BE SYNCHRONOUS for import.meta initialization
-   * MUST BE CALLED AFTER receiving the url body due to I/O
-   * @param {string} url
-   * @returns {string}
+   * MUST be called AFTER receiving the url body due to I/O
+   * @param {string} url The URL specifier to look up
+   * @returns {URL['href']} The resolved url for the provided specifier
    */
   baseURL(url) {
     if (

diff --git a/test/es-module/test-http-imports.mjs b/test/es-module/test-http-imports.mjs
@@ -114,8 +114,8 @@ for (const { protocol, createServer } of [
 
     // Redirects have the same import.meta.url but different cache
     // entry on Web
-    const relativeAfterRedirect = new URL(url.href + 'foo/index.js');
-    const redirected = new URL(url.href + 'bar/index.js');
+    const relativeAfterRedirect = new URL('foo/index.js', url.href);
+    const redirected = new URL('bar/index.js', url.href);
     redirected.searchParams.set('body', 'export let relativeDepURL = (await import("./baz.js")).url');
     relativeAfterRedirect.searchParams.set('redirect', JSON.stringify({
       status: 302,