Skip to content
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

ESM loader chaining while loading loaders regression in 19.8.0 #47526

Closed
clemyan opened this issue Apr 12, 2023 · 3 comments
Closed

ESM loader chaining while loading loaders regression in 19.8.0 #47526

clemyan opened this issue Apr 12, 2023 · 3 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders

Comments

@clemyan
Copy link

clemyan commented Apr 12, 2023

Version

19.8.0

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

esm

What steps will reproduce the bug?

Issue occurs when chaining 3 or more loaders


loader-a.mjs

export function resolve(id, parent, next) { console.log('loader-a', id); return next(id, parent); }

loader-b.mjs

export function resolve(id, parent, next) { console.log('loader-b', id); return next(id, parent); }

loader-c.mjs

export function resolve(id, parent, next) { console.log('loader-c', id); return next(id, parent); }

$ node --loader ./loader-a.mjs --loader ./loader-b.mjs --loader ./loader-c.mjs a.mjs

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

The above node run would output

loader-a ./loader-b.mjs
loader-b ./loader-c.mjs
loader-a ./loader-c.mjs
loader-c <cwd>/a.mjs
loader-b <cwd>/a.mjs
loader-a <cwd>/a.mjs

In other words, each loader would be chained to resolve subsequent loaders as per #43772 and docs:

When hooks are used they apply to each subsequent loader [...]

What do you see instead?

loader-a ./loader-b.mjs
loader-b ./loader-c.mjs
loader-c <cwd>/a.mjs
loader-b <cwd>/a.mjs
loader-a <cwd>/a.mjs

Only the immediately preceding loader is used to load a loader

Additional information

This regression was introduced in #45869, first released in v19.8.0

The feature in question is implemented by added loaded loaders to the internal ESM loader by calling ESMLoader#addCustomLoaders in a loop

for (let i = 0; i < customLoaders.length; i++) {
const customLoader = customLoaders[i];
// Importation must be handled by internal loader to avoid polluting user-land
const keyedExportsSublist = await internalEsmLoader.import(
[customLoader],
parentURL,
kEmptyObject,
);
internalEsmLoader.addCustomLoaders(keyedExportsSublist);
ArrayPrototypePushApply(allLoaders, keyedExportsSublist);
}

Before the regression, ESMLoader#addCustomLoaders mutates this.#hooks, thus multiple calls to it will add to the internal loader chain

addCustomLoaders(
customLoaders = [],
) {
for (let i = 0; i < customLoaders.length; i++) {
const {
exports,
url,
} = customLoaders[i];
const {
globalPreload,
resolve,
load,
} = ESMLoader.pluckHooks(exports);
if (globalPreload) {
ArrayPrototypePush(
this.#hooks.globalPreload,
{
fn: globalPreload,
url,
},
);
}
if (resolve) {
ArrayPrototypePush(
this.#hooks.resolve,
{
fn: resolve,
url,
},
);
}
if (load) {
ArrayPrototypePush(
this.#hooks.load,
{
fn: load,
url,
},
);
}
}
}

In #45869, ESMLoader#addCustomLoaders was changed to (re-)instantiating a Hooks object, making multiple calls to it overwriting this.#hooks

addCustomLoaders(userLoaders) {
const { Hooks } = require('internal/modules/esm/hooks');
this.#hooks = new Hooks(userLoaders);
}

@VoltrexKeyva VoltrexKeyva added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Apr 12, 2023
@arcanis
Copy link
Contributor

arcanis commented Apr 13, 2023

cc @GeoffreyBooth since you refactored this code in #45869

@GeoffreyBooth
Copy link
Member

Can you test again? This has been refactored substantially in the off-thread work.

Even if it works now, it would be good to add a test to confirm that the behavior doesn’t regress.

@clemyan
Copy link
Author

clemyan commented Apr 14, 2023

Can you test again? This has been refactored substantially in the off-thread work.

Even if it works now, it would be good to add a test to confirm that the behavior doesn’t regress.

Just tested on the nightly. It is fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

No branches or pull requests

4 participants