From 56ecf2928311a8f859ab536e8cbee24b08d1bf16 Mon Sep 17 00:00:00 2001
From: Antoine du Hamel <duhamelantoine1995@gmail.com>
Date: Sun, 17 Sep 2023 02:48:48 +0200
Subject: [PATCH] esm: fix support for `URL` instances in `register`

PR-URL: https://github.com/nodejs/node/pull/49655
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 doc/api/module.md                        | 12 ++++++---
 lib/internal/modules/esm/loader.js       | 14 +++++------
 test/es-module/test-esm-loader-hooks.mjs | 31 ++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/doc/api/module.md b/doc/api/module.md
index 0f44eac1c41f43..e28df26ba84d2b 100644
--- a/doc/api/module.md
+++ b/doc/api/module.md
@@ -84,14 +84,18 @@ isBuiltin('wss'); // false
 
 <!-- YAML
 added: v20.6.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/49655
+    description: Add support for WHATWG URL instances.
 -->
 
 > Stability: 1.2 - Release candidate
 
-* `specifier` {string} Customization hooks to be registered; this should be the
-  same string that would be passed to `import()`, except that if it is relative,
-  it is resolved relative to `parentURL`.
-* `parentURL` {string} If you want to resolve `specifier` relative to a base
+* `specifier` {string|URL} Customization hooks to be registered; this should be
+  the same string that would be passed to `import()`, except that if it is
+  relative, it is resolved relative to `parentURL`.
+* `parentURL` {string|URL} If you want to resolve `specifier` relative to a base
   URL, such as `import.meta.url`, you can pass that URL here. **Default:**
   `'data:'`
 * `options` {Object}
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
index 5d1307565885de..7712e6359882f1 100644
--- a/lib/internal/modules/esm/loader.js
+++ b/lib/internal/modules/esm/loader.js
@@ -14,7 +14,7 @@ const {
   ERR_UNKNOWN_MODULE_FORMAT,
 } = require('internal/errors').codes;
 const { getOptionValue } = require('internal/options');
-const { pathToFileURL } = require('internal/url');
+const { pathToFileURL, isURL } = require('internal/url');
 const { emitExperimentalWarning } = require('internal/util');
 const {
   getDefaultConditions,
@@ -321,7 +321,7 @@ class ModuleLoader {
       // eslint-disable-next-line no-use-before-define
       this.setCustomizations(new CustomizedModuleLoader());
     }
-    return this.#customizations.register(specifier, parentURL, data, transferList);
+    return this.#customizations.register(`${specifier}`, `${parentURL}`, data, transferList);
   }
 
   /**
@@ -542,11 +542,11 @@ function getHooksProxy() {
 
 /**
  * Register a single loader programmatically.
- * @param {string} specifier
- * @param {string} [parentURL] Base to use when resolving `specifier`; optional if
+ * @param {string|import('url').URL} specifier
+ * @param {string|import('url').URL} [parentURL] Base to use when resolving `specifier`; optional if
  * `specifier` is absolute. Same as `options.parentUrl`, just inline
  * @param {object} [options] Additional options to apply, described below.
- * @param {string} [options.parentURL] Base to use when resolving `specifier`
+ * @param {string|import('url').URL} [options.parentURL] Base to use when resolving `specifier`
  * @param {any} [options.data] Arbitrary data passed to the loader's `initialize` hook
  * @param {any[]} [options.transferList] Objects in `data` that are changing ownership
  * @returns {void} We want to reserve the return value for potential future extension of the API.
@@ -571,12 +571,12 @@ function getHooksProxy() {
  */
 function register(specifier, parentURL = undefined, options) {
   const moduleLoader = require('internal/process/esm_loader').esmLoader;
-  if (parentURL != null && typeof parentURL === 'object') {
+  if (parentURL != null && typeof parentURL === 'object' && !isURL(parentURL)) {
     options = parentURL;
     parentURL = options.parentURL;
   }
   moduleLoader.register(
-    `${specifier}`,
+    specifier,
     parentURL ?? 'data:',
     options?.data,
     options?.transferList,
diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs
index 7ff1fc520d90b5..a4c2f52d0df367 100644
--- a/test/es-module/test-esm-loader-hooks.mjs
+++ b/test/es-module/test-esm-loader-hooks.mjs
@@ -517,6 +517,37 @@ describe('Loader hooks', { concurrency: true }, () => {
       assert.strictEqual(signal, null);
     });
 
+    it('should have `register` accept URL objects as `parentURL`', async () => {
+      const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
+        '--no-warnings',
+        '--import',
+        `data:text/javascript,${encodeURIComponent(
+          'import{ register } from "node:module";' +
+          'import { pathToFileURL } from "node:url";' +
+          'register("./hooks-initialize.mjs", pathToFileURL("./"));'
+        )}`,
+        '--input-type=module',
+        '--eval',
+        `
+        import {register} from 'node:module';
+        register(
+          ${JSON.stringify(fixtures.fileURL('es-module-loaders/loader-load-foo-or-42.mjs'))},
+          new URL('data:'),
+        );
+
+        import('node:os').then((result) => {
+          console.log(JSON.stringify(result));
+        });
+        `,
+      ], { cwd: fixtures.fileURL('es-module-loaders/') });
+
+      assert.strictEqual(stderr, '');
+      assert.deepStrictEqual(stdout.split('\n').sort(), ['hooks initialize 1', '{"default":"foo"}', ''].sort());
+
+      assert.strictEqual(code, 0);
+      assert.strictEqual(signal, null);
+    });
+
     it('should have `register` work with cjs', async () => {
       const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
         '--no-warnings',