Skip to content

esm: add deregister method #49159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/internal/modules/esm/default_loader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

const { defaultLoad } = require('internal/modules/esm/load');
const { defaultResolve } = require('internal/modules/esm/resolve');

exports.resolve = defaultResolve;
exports.load = defaultLoad;
133 changes: 69 additions & 64 deletions lib/internal/modules/esm/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ const {
} = require('internal/util');

const {
defaultResolve,
throwIfInvalidParentURL,
} = require('internal/modules/esm/resolve');
const {
Expand Down Expand Up @@ -87,51 +86,26 @@ let importMetaInitializer;
// [2] `validate...()`s throw the wrong error

class Hooks {
#chains = {
/**
* Prior to ESM loading. These are called once before any modules are started.
* @private
* @property {KeyedHook[]} globalPreload Last-in-first-out list of preload hooks.
*/
globalPreload: [],

/**
* Phase 1 of 2 in ESM loading.
* The output of the `resolve` chain of hooks is passed into the `load` chain of hooks.
* @private
* @property {KeyedHook[]} resolve Last-in-first-out collection of resolve hooks.
*/
resolve: [
{
fn: defaultResolve,
url: 'node:internal/modules/esm/resolve',
},
],

/**
* Phase 2 of 2 in ESM loading.
* @private
* @property {KeyedHook[]} load Last-in-first-out collection of loader hooks.
*/
load: [
{
fn: require('internal/modules/esm/load').defaultLoad,
url: 'node:internal/modules/esm/load',
},
],
};
#loaderInstances = [];
#lastInstanceId = 0;

// Cache URLs we've already validated to avoid repeated validation
#validatedUrls = new SafeSet();

allowImportMetaResolve = false;

constructor() {
const defaultLoader = 'internal/modules/esm/default_loader';
this.addCustomLoader(`node:${defaultLoader}`, require(defaultLoader));
}

/**
* Import and register custom/user-defined module loader hook(s).
* @param {string} urlOrSpecifier
* @param {string} parentURL
* @param {any} [data] Arbitrary data to be passed from the custom
* loader (user-land) to the worker.
* @returns {Promise<number>} The id of the registered loader.
*/
async register(urlOrSpecifier, parentURL, data) {
const moduleLoader = require('internal/process/esm_loader').esmLoader;
Expand All @@ -143,46 +117,71 @@ class Hooks {
return this.addCustomLoader(urlOrSpecifier, keyedExports, data);
}

deregister(id) {
return this.removeCustomLoader(id);
}

/**
* Collect custom/user-defined module loader hook(s).
* After all hooks have been collected, the global preload hook(s) must be initialized.
* @param {string} url Custom loader specifier
* @param {Record<string, unknown>} exports
* @param {any} [data] Arbitrary data to be passed from the custom loader (user-land)
* to the worker.
* @returns {any} The result of the loader's `initialize` hook, if provided.
* @returns {Promise<number>} The id of the registered loader.
*/
addCustomLoader(url, exports, data) {
async addCustomLoader(url, exports, data) {
const {
globalPreload,
initialize,
resolve,
load,
} = pluckHooks(exports);

if (globalPreload && !initialize) {
emitExperimentalWarning(
'`globalPreload` is planned for removal in favor of `initialize`. `globalPreload`',
);
ArrayPrototypePush(this.#chains.globalPreload, { __proto__: null, fn: globalPreload, url });
}
if (resolve) {
const next = this.#chains.resolve[this.#chains.resolve.length - 1];
ArrayPrototypePush(this.#chains.resolve, { __proto__: null, fn: resolve, url, next });
}
if (load) {
const next = this.#chains.load[this.#chains.load.length - 1];
ArrayPrototypePush(this.#chains.load, { __proto__: null, fn: load, url, next });
const next = this.#loaderInstances[this.#loaderInstances.length - 1];
const instance = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is maybe not a good name: instance usually means instance of a class, but here I think it's being used as "occurrence".

Suggested change
const instance = {
const registrationId = //

Copy link
Member

@JakobJingleheimer JakobJingleheimer Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use something like uid(3) (it looks like crypto.randomUUID() is the only thing already available in node, but that would be massively overkill) or perhaps the loader's url. Chatting with Geoffrey, he pointed out a loader can now be registered multiple times, so if the loader's url, it would need to have something else appended, like a generation id.

Copy link
Contributor

@aduh95 aduh95 Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use symbols for the public facing API, and the index for the internal API; using random string or number would be a mistake.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooow! That's a good idea!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What’s the value of a symbol as opposed to a string like file:///path/to/loader.js?1? At least the latter provides some useful information, such as for debugging, and then we don’t need to have different data structures internally and externally.

Copy link
Member

@JakobJingleheimer JakobJingleheimer Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A symbol is guaranteed to be unique, so then there is no concern for a generational id on the same url:

Symbol(url) ≠ Symbol(url)

Also, the label of a symbol is visible, so it still provides almost identical debugging value 🙂

And bonus: it precludes the possibility of a user getting overly clever and foot-gunning a deregistration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also return a handle, like:

const handle = register(...)
handle.deregister()

Then the handle could potentially get other methods in the future. It could also contain the return value of initialize as a property, like handle.initializationResult.

Not sure I prefer this over other options, just adding it to the mix. We still need to deal with the problem that register currently returns the return value of initialize, so either we add this symbol into initialize for the user to return, and then somehow we get symbols across the thread barrier; or we change the return signature of register.

__proto__: null,
id: ++this.#lastInstanceId,
url,
globalPreload,
initialize,
resolve,
load,
next,
};
ArrayPrototypePush(this.#loaderInstances, instance);
instance.state = await initialize?.(data, { __proto__: null, id: instance.id, url });
return instance.id;
}

removeCustomLoader(id) {
// This loop purposefully has `> 0` in order to prevent people from
// removing the first loader (i.e. the default one).
for (let i = this.#loaderInstances.length - 1; i > 0; --i) {
if (id === this.#loaderInstances[i].id) {
if (i + 1 < this.#loaderInstances.length) {
this.#loaderInstances[i + 1].next = this.#loaderInstances[i - 1];
}
this.#loaderInstances.splice(i, 1);
return true;
}
}
return initialize?.(data);
return false;
}

/**
* Initialize `globalPreload` hooks.
*/
initializeGlobalPreload() {
const preloadScripts = [];
for (let i = this.#chains.globalPreload.length - 1; i >= 0; i--) {
for (const loader of this.#loaderInstances) {
if (!loader.globalPreload) {
continue;
}
const { MessageChannel } = require('internal/worker/io');
const channel = new MessageChannel();
const {
Expand All @@ -193,10 +192,7 @@ class Hooks {
insidePreload.unref();
insideLoader.unref();

const {
fn: preload,
url: specifier,
} = this.#chains.globalPreload[i];
const preload = loader.globalPreload;

const preloaded = preload({
port: insideLoader,
Expand All @@ -207,8 +203,8 @@ class Hooks {
if (typeof preloaded !== 'string') { // [2]
throw new ERR_INVALID_RETURN_VALUE(
'a string',
`${specifier} globalPreload`,
preload,
`${loader.url} globalPreload`,
loader.globalPreload,
);
}

Expand Down Expand Up @@ -240,7 +236,6 @@ class Hooks {
) {
throwIfInvalidParentURL(parentURL);

const chain = this.#chains.resolve;
const context = {
conditions: getDefaultConditions(),
importAssertions,
Expand Down Expand Up @@ -272,7 +267,11 @@ class Hooks {
}
};

const nextResolve = nextHookFactory(chain[chain.length - 1], meta, { validateArgs, validateOutput });
const nextResolve = nextHookFactory(
this.#loaderInstances[this.#loaderInstances.length - 1],
meta,
{ validateArgs, validateOutput },
);

const resolution = await nextResolve(originalSpecifier, context);
const { hookErrIdentifier } = meta; // Retrieve the value after all settled
Expand Down Expand Up @@ -364,7 +363,6 @@ class Hooks {
* @returns {Promise<{ format: ModuleFormat, source: ModuleSource }>}
*/
async load(url, context = {}) {
const chain = this.#chains.load;
const meta = {
chainFinished: null,
context,
Expand Down Expand Up @@ -410,7 +408,11 @@ class Hooks {
}
};

const nextLoad = nextHookFactory(chain[chain.length - 1], meta, { validateArgs, validateOutput });
const nextLoad = nextHookFactory(
this.#loaderInstances[this.#loaderInstances.length - 1],
meta,
{ validateArgs, validateOutput },
);

const loaded = await nextLoad(url, context);
const { hookErrIdentifier } = meta; // Retrieve the value after all settled
Expand Down Expand Up @@ -789,11 +791,13 @@ function pluckHooks({
function nextHookFactory(current, meta, { validateArgs, validateOutput }) {
// First, prepare the current
const { hookName } = meta;
const {
fn: hook,
url: hookFilePath,
next,
} = current;

const { next, state, url: hookFilePath } = current;
const hook = current[hookName];

if (!hook) {
return nextHookFactory(next, meta, { validateArgs, validateOutput });
}

// ex 'nextResolve'
const nextHookName = `next${
Expand Down Expand Up @@ -828,8 +832,9 @@ function nextHookFactory(current, meta, { validateArgs, validateOutput }) {
if (context) { // `context` has already been validated, so no fancy check needed.
ObjectAssign(meta.context, context);
}

const output = await hook(arg0, meta.context, nextNextHook);
const output = hook.length === 4 ?
await hook(arg0, meta.context, state, nextNextHook) :
await hook(arg0, meta.context, nextNextHook);
validateOutput(outputErrIdentifier, output);

if (output?.shortCircuit === true) { meta.shortCircuited = true; }
Expand Down
45 changes: 43 additions & 2 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ require('internal/modules/cjs/loader');
const {
FunctionPrototypeCall,
ObjectSetPrototypeOf,
SafeMap,
SafeWeakMap,
Symbol,
} = primordials;

const {
Expand Down Expand Up @@ -323,6 +325,15 @@ class ModuleLoader {
return this.#customizations.register(specifier, parentURL, data, transferList);
}

deregister(id) {
// They have had to register customizations before this method does
// anything useful.
if (!this.#customizations) {
return false;
}
return this.#customizations.deregister(id);
}

/**
* Resolve the location of the module.
* @param {string} originalSpecifier The specified URL path of the module to
Expand Down Expand Up @@ -435,6 +446,8 @@ ObjectSetPrototypeOf(ModuleLoader.prototype, null);
class CustomizedModuleLoader {

allowImportMetaResolve = true;
#symbolForwardMap = new SafeMap();
#symbolReverseMap = new SafeMap();

/**
* Instantiate a module loader that uses user-provided custom loader hooks.
Expand All @@ -452,10 +465,32 @@ class CustomizedModuleLoader {
* @param {any} [data] Arbitrary data to be passed from the custom loader
* (user-land) to the worker.
* @param {any[]} [transferList] Objects in `data` that are changing ownership
* @returns {{ format: string, url: URL['href'] }}
* @returns {symbol} Unique identifier for the loader instance.
*/
register(originalSpecifier, parentURL, data, transferList) {
return hooksProxy.makeSyncRequest('register', transferList, originalSpecifier, parentURL, data);
const id = hooksProxy.makeSyncRequest('register', transferList, originalSpecifier, parentURL, data);
let symbol = this.#symbolForwardMap.get(id);
if (!symbol) {
symbol = Symbol(`${originalSpecifier}-${id}`);
this.#symbolForwardMap.set(id, symbol);
this.#symbolReverseMap.set(symbol, id);
}
return symbol;
}

/**
* Remove a loader and all its hooks from the module system.
* @param {symbol} symbol The value from calling `register()`
* @returns {boolean} True if the loader was de-registered, false otherwise
*/
deregister(symbol) {
const id = this.#symbolReverseMap.get(symbol);
if (id && hooksProxy.makeSyncRequest('deregister', undefined, id)) {
this.#symbolForwardMap.delete(id);
this.#symbolReverseMap.delete(symbol);
return true;
}
return false;
}

/**
Expand Down Expand Up @@ -582,8 +617,14 @@ function register(specifier, parentURL = undefined, options) {
);
}

function deregister(id) {
const moduleLoader = require('internal/process/esm_loader').esmLoader;
return moduleLoader.deregister(id);
}

module.exports = {
createModuleLoader,
getHooksProxy,
register,
deregister,
};
3 changes: 2 additions & 1 deletion lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@

const { findSourceMap } = require('internal/source_map/source_map_cache');
const { Module } = require('internal/modules/cjs/loader');
const { register } = require('internal/modules/esm/loader');
const { register, deregister } = require('internal/modules/esm/loader');
const { SourceMap } = require('internal/source_map/source_map');

Module.findSourceMap = findSourceMap;
Module.register = register;
Module.deregister = deregister;
Module.SourceMap = SourceMap;
module.exports = Module;
Loading