Skip to content

Commit

Permalink
esm: fix support for URL instances in register
Browse files Browse the repository at this point in the history
PR-URL: nodejs/node#49655
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
sercher committed Apr 25, 2024
1 parent 4328950 commit 79c5d33
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 11 deletions.
12 changes: 8 additions & 4 deletions graal-nodejs/doc/api/module.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,18 @@ isBuiltin('wss'); // false
<!-- YAML
added: REPLACEME
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49655
description: Add support for WHATWG URL instances.
-->
> Stability: 1.1 - Active development
* `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}
Expand Down
14 changes: 7 additions & 7 deletions graal-nodejs/lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,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,
Expand Down Expand Up @@ -329,7 +329,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);
}

/**
Expand Down Expand Up @@ -529,11 +529,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.
Expand All @@ -558,12 +558,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,
Expand Down
31 changes: 31 additions & 0 deletions graal-nodejs/test/es-module/test-esm-loader-hooks.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,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',
Expand Down

0 comments on commit 79c5d33

Please sign in to comment.