esm: unflag `Module.register` and allow nested loader `import()` #48559

izaakschroeder · 2023-06-26T07:31:15Z

Overview

Major functional changes:

Allow import() to work within loaders that require other loaders,
Unflag the use of Module.register.

A new interface Customizations has been created in order to unify ModuleLoader (previously DefaultModuleLoader), Hooks and CustomizedModuleLoader all of which now implement it:

interface LoadResult {
  format: ModuleFormat;
  source: ModuleSource;
}

interface ResolveResult {
  format: string;
  url: URL['href'];
}

interface Customizations {
  allowImportMetaResolve: boolean;
  load(url: string, context: object): Promise<LoadResult>
  resolve(
    originalSpecifier:
    string, parentURL: string,
    importAssertions: Record<string, string>
  ): Promise<ResolveResult>
  resolveSync(
    originalSpecifier:
    string, parentURL: string,
    importAssertions: Record<string, string>
  ) ResolveResult;
  register(specifier: string, parentUrl: string): any;
  forceLoadHooks(): void;
}

The ModuleLoader class now has setCustomizations which takes an object of this shape and delegates its responsibilities to this object if present.

Note that two properties allowImportMetaResolve and resolveSync exist now as a mechanism for import.meta.resolve – since Hooks does not implement resolveSync other loaders cannot use import.meta.resolve; allowImportMetaResolve is a way of checking for that case instead of invoking resolveSync and erroring.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

nodejs-github-bot · 2023-06-26T07:31:20Z

Review requested:

@nodejs/loaders
@nodejs/modules

GeoffreyBooth

As I mentioned in a code comment, I think this needs to wait until after #48439 lands to know what needs to be done after the refactoring that’s part of that PR.

I read #48515 and I don’t see a consensus there that the current behavior is a problem, and no discussion around what the best potential solution would be. It’s fine to open a PR as a POC to present a potential solution, but I think you need to first make the case for why we need to change the current Loaders API behavior, and then if we agree on that we can look into whether this PR is the best solution.

Also are there no docs updates as part of this? There are no user-visible changes as a result of this PR?

At the very least there should be some new tests, that fail on current main and pass in this branch, so that the change made by this PR is clear.

lib/internal/modules/esm/utils.js

izaakschroeder · 2023-06-27T07:57:36Z

Thanks so much for all the feedback @GeoffreyBooth!

As I mentioned in a code comment, I think this needs to wait until after #48439 lands to know what needs to be done after the refactoring that’s part of that PR.

Fine with me 😄 looks like the state of that is a bit in flux as it stands? I'm happy to rebase my work on top of it when it's stable enough though.

I read #48515 and I don’t see a consensus there that the current behavior is a problem, and no discussion around what the best potential solution would be. It’s fine to open a PR as a POC to present a potential solution, but I think you need to first make the case for why we need to change the current Loaders API behavior, and then if we agree on that we can look into whether this PR is the best solution.

Yeah this is purely a proposal at this point. Would you prefer I make the case here or in the issue? I thought the issue description highlighted the two primary reasons:

It's kind of unintuitive/confusing behaviour when other PRs state "Loaders now apply to subsequent loaders" but this is not true during import()
It prevents users of loaders from doing something like import('some-config') if some-config needs to be resolved using a custom loader, as is the case in yarn workspaces.

Is there a recommended approach here to continuing the discussion?

Also are there no docs updates as part of this? There are no user-visible changes as a result of this PR?

There aren't really any user-visible changes. It is (humbly in my opinion) in line with what I see as the spirit of #43772 which states:

Loaders now apply to subsequent loaders, for example: --experimental-loader ts-node --experimental-loader loader-written-in-typescript.

This is true ONLY if all the imports of those loaders are static – if one of those loaders invokes import() then this behaviour does NOT apply in those cases and this feels like a bug to me. I would posit that this updated behaviour is the "intuitively correct" one… and thus what most users would just "expect" to happen given the current documentation… (at least that was me…) but such is purely my opinion and not the result of some community discussion or anything.

At the very least there should be some new tests, that fail on current main and pass in this branch, so that the change made by this PR is clear.

This is my very first time contributing to node! Happy to write some tests, not really sure where to begin but I'll poke around in the code a bit.

aduh95 · 2023-06-27T09:41:47Z

This is my very first time contributing to node! Happy to write some tests, not really sure where to begin but I'll poke around in the code a bit.

Thanks a lot for sending this PR, Node.js strives from contributions like yours, you did the right thing opening this PR! Welcome and let's make node better together 🚀

This is true ONLY if all the imports of those loaders are static – if one of those loaders invokes import() then this behaviour does NOT apply in those cases and this feels like a bug to me. I would posit that this updated behaviour is the "intuitively correct" one

Could you clarify this point? Your comment makes it sound like ts-node would not be used when a loader is dynamically imported from loader-written-in-typescript, I don't think it's the case 🤔 My understanding that the following happens today when importing from loader-written-in-typescript:

Goes through loader	static `import`	dynamic `import()`
`ts-node`	✅	✅
`loader-written-in-typescript`	❌	✅

With your PR, we could align static and dynamic behavior:

Goes through loader	static `import`	dynamic `import()`
`ts-node`	✅	✅
`loader-written-in-typescript`	❌	❌

Am I getting this right?

There aren't really any user-visible changes

Aren't you opening this PR because you are a user of the hook API and you would like to observe these changes? A helpful test would be one that's failing today on main and would be passing with your PR: such test would help us reviewers understand what specific behavior of the API you are trying to change, and what would the new DX feel like.
A good idea is to start from your own use case, the one that led you into opening this PR, simplify it down to its simplest form, and voilà you have a test. You can add it as test case in test/es-module/test-esm-loader-hooks.mjs. You can check out doc/contributing/writing-tests.md for more info.

izaakschroeder · 2023-06-27T22:58:03Z

Hey @aduh95!

Thanks a lot for sending this PR, Node.js strives from contributions like yours, you did the right thing opening this PR! Welcome and let's make node better together 🚀

Thanks for the positive feedback! 😄

With your PR, we could align static and dynamic behaviour:

That is the idea – import('foo') should have the same behaviour as import 'foo'; within a loader. I added two small tests that should hopefully clarify things a little better.

In the following example:

node \
  --loader a \
  --loader b

Previously b could do this:

import 'magic-thing-that-depends-on-a';

BUT NOT:

export const load = async (...) => {
  // This fails today
  await import('magic-thing-that-depends-on-a');
  return ...;
};

export const resolve = async (...) => {
  // This fails today
  await import('magic-thing-that-depends-on-a');
  return ...;
};

This PR changes it so that BOTH cases now work.

Aren't you opening this PR because you are a user of the hook API and you would like to observe these changes? A helpful test would be one that's failing today on main and would be passing with your PR: such test would help us reviewers understand what specific behaviour of the API you are trying to change, and what would the new DX feel like.

Done! Thanks for the suggestion 🎉

GeoffreyBooth

Thanks for the explanation and the added test. I agree this is something that should be fixed. I want to keep the block on just to ensure that this doesn't land before #48439, because that one is a cleanup PR to let the new register API get released and it's important to get that out as soon as we can; and the author of register is waiting on that and it doesn't feel fair to make the wait even longer because another PR slipped in ahead. If you have time to help with #48439, it covers a lot of the same ground as your PR so you might be able to fix the last few broken tests.

lib/internal/modules/esm/utils.js

izaakschroeder · 2023-06-30T06:46:54Z

Thanks for the explanation and the added test. I agree this is something that should be fixed.

Awesome! 😄

I want to keep the block on just to ensure that this doesn't land before #48439, because that one is a cleanup PR to let the new register API get released and it's important to get that out as soon as we can; and the author of register is waiting on that and it doesn't feel fair to make the wait even longer because another PR slipped in ahead. If you have time to help with #48439, it covers a lot of the same ground as your PR so you might be able to fix the last few broken tests.

I did my best. I apologize profusely for clobbering @JakobJingleheimer's work here 😞 Like you mentioned both PRs kind of overlap a little, and I because I don't understand enough of what's going on in that PR, (first time contributor to node, my knowledge just isn't strong enough), I took a stab at solving the problem myself. I went a different route, favouring composition over inheritance which I think is a little cleaner and is easier for my brain to grok.

I took the test case from @JakobJingleheimer's PR and it passes, as well as the standard node test suite. I believe this now also resolves the TODO from @jlenon7 to reuse the Hooks.register() method for registering loaders.

Thanks again @aduh95 and @GeoffreyBooth for your words of encouragement and feedback throughout all this 🎉

lib/internal/modules/esm/hooks.js

lib/internal/modules/esm/initialize_import_meta.js

lib/internal/modules/esm/loader.js

izaakschroeder · 2023-06-30T09:45:50Z

lib/internal/modules/esm/loader.js

+      return this.#delegate.register(specifier, parentUrl);
+    }
+    // TODO: Consider flagging this.
+    const flag = true; // getOptionValue('--experimental-loader-register');


If you want a flag like --experimental-loader-register @aduh95 or @GeoffreyBooth and show me how I can implement it then I'm happy to add this here. Otherwise this unflags things in the same manner as https://github.com/nodejs/node/pull/48439/files#r1227723703.

We need to make a decision on this @aduh95 @JakobJingleheimer. I’m considering two options:

Introduce a new flag like --experimental-module-customization that needs to be passed in order to enable the Loaders API at all, whether via register or --experimental-loader / --loader.

Decide that we want to unflag the Loaders API, while keeping the printed warning and the experimental status in the docs. We should probably add a special printed warning for globalPreload to say explicitly that it’s disappearing soon.

The advantage of the new flag is to undo the mistake we made of shipping --loader by itself, and really emphasizes to users that this stuff is still experimental. But we’re so close to stability that I’m not sure we want to send that message; we could go the other way and inch closer to stability by dropping the need for a flag, like going to a closer-to-stable experimental status. I’m more inclined to do the latter, as it’s less user-hostile and arguably not much of a change from the status quo where --loader exists. Thoughts?

From my (an end-user's) perspective the act of invoking register is kind of? acknowledging the opt-in (you can't accidentally turn the behaviour on unless you depend on some third-party code that does I guess) and I think your latter suggestion is pretty sensible as it aligns with what I was consider default intuition. 😄

Sorry, GitHub collapsed this and I missed it.

I think ideally we do neither, and instead enable --experimental-loader to be either (effectively) a boolean or a list. If that's not possible, then land but don't release 😢

lib/internal/modules/esm/loader.js

test/es-module/test-esm-loader-programmatically.mjs

GeoffreyBooth · 2023-06-30T16:55:56Z

So does this PR now both fix the import() thing and do what #48439 was attempting? That would be great; @JakobJingleheimer should review.

Would it be feasible to split this into two PRs, one for the register flag stuff and another for the import() stuff? That would make it easier to review, and give us more flexibility in deciding what to backport.

izaakschroeder · 2023-06-30T17:06:30Z

So does this PR now both fix the import() thing and do what #48439 was attempting?

Yes, to my knowledge! 🎉 The programmatic usage of register() no longer requires a "dummy" loader – just like the #48439 (and in fact I stole the test that verifies it right out of #48439). If #48439 does more than just fixing the "register()" issue I might be missing something… but my code covers the core issue at least.

Would it be feasible to split this into two PRs, one for the register flag stuff and another for the import() stuff? That would make it easier to review, and give us more flexibility in deciding what to backport.

This might be possible… it would require a bit of finessing… I'll have to think on this. Let me know how critical this is as it might require some effort 😄

lib/internal/modules/esm/loader.js

lib/internal/modules/esm/hooks.js

test/es-module/test-esm-loader-hooks.mjs

izaakschroeder · 2023-07-02T23:33:47Z

@GeoffreyBooth @aduh95 how would you like me to proceed with this? Should I keep it as-is and wait for feedback? Should I continue iterating on a few possible cleanup ideas I have?

GeoffreyBooth · 2023-07-03T02:27:05Z

I'm traveling until July 12; @JakobJingleheimer is on holiday until July 8. I'll review it when I'm back, and I think this should definitely get Jacob's review too before it lands. Sorry you happened to submit this during a holiday period 😀

If you have improvements that don't increase the scope of the PR any further, sure, do them on this branch; but anything that can be logically separated as a follow-up PR would be better saved for that, so things are easier to review and bisect in case of issues. You can branch off of this branch and even open the follow-up with a note that it's based on this, and ideally include the link to compare the diff of the two branches so that someone can review the subsequent improvements before waiting for this to land. Thanks for your work and thanks for asking!

izaakschroeder · 2023-07-04T08:53:35Z

I'm traveling until July 12; @JakobJingleheimer is on holiday until July 8. I'll review it when I'm back, and I think this should definitely get Jacob's review too before it lands. Sorry you happened to submit this during a holiday period 😀

Ack sorry I didn't realize you were off, enjoy your travels!

If you have improvements that don't increase the scope of the PR any further, sure, do them on this branch; but anything that can be logically separated as a follow-up PR would be better saved for that, so things are easier to review and bisect in case of issues. You can branch off of this branch and even open the follow-up with a note that it's based on this, and ideally include the link to compare the diff of the two branches so that someone can review the subsequent improvements before waiting for this to land. Thanks for your work and thanks for asking!

Thanks for clarifying! 😄 I think I've made the rest of the improvements I want to… as well as addressed the bulk of the cleanup points I made for myself. I'll wait til you're both back before following up further.

Notable changes: esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 PR-URL: #49185

Notable changes: doc: * add new TSC members (Michael Dawson) #48841 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 PR-URL: #49185

Notable changes: doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 PR-URL: #49185

Notable changes: deps: * V8: cherry-pick 93275031284c (Joyee Cheung) #48660 doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 PR-URL: #49185

deps: * V8: cherry-pick 93275031284c (Joyee Cheung) #48660 doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 * initialize cppgc (Daryl Haresign and Joyee Cheung) #48660 and #45704 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 PR-URL: #49185

Notable changes: deps: * V8: cherry-pick 93275031284c (Joyee Cheung) #48660 doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 * initialize cppgc (Daryl Haresign and Joyee Cheung) #48660 and #45704 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 PR-URL: #49185

ruyadorno · 2023-09-07T02:50:28Z

Based on the note from @GeoffreyBooth on #46826 (comment) I'm going ahead and adding a backport-requested label to this PR too. Ideally you can organize it so that it can be a single backport PR containing all the required commits 😊

Notable changes: deps: * V8: cherry-pick 93275031284c (Joyee Cheung) nodejs#48660 doc: * add new TSC members (Michael Dawson) nodejs#48841 * add rluvaton to collaborators (Raz Luvaton) nodejs#49215 esm: * unflag import.meta.resolve (Guy Bedford) nodejs#49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) nodejs#48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) nodejs#48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) nodejs#48765 module: * implement `register` utility (João Lenon) nodejs#46826 * make CJS load from ESM loader (Antoine du Hamel) nodejs#47999 src: * add built-in `.env` file support (Yagiz Nizipli) nodejs#48890 * initialize cppgc (Daryl Haresign and Joyee Cheung) nodejs#48660 and nodejs#45704 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs#48975 PR-URL: nodejs#49185

Major functional changes: - Allow `import()` to work within loaders that require other loaders, - Unflag the use of `Module.register`. A new interface `Customizations` has been created in order to unify `ModuleLoader` (previously `DefaultModuleLoader`), `Hooks` and `CustomizedModuleLoader` all of which now implement it: ```ts interface LoadResult { format: ModuleFormat; source: ModuleSource; } interface ResolveResult { format: string; url: URL['href']; } interface Customizations { allowImportMetaResolve: boolean; load(url: string, context: object): Promise<LoadResult> resolve( originalSpecifier: string, parentURL: string, importAssertions: Record<string, string> ): Promise<ResolveResult> resolveSync( originalSpecifier: string, parentURL: string, importAssertions: Record<string, string> ) ResolveResult; register(specifier: string, parentUrl: string): any; forceLoadHooks(): void; importMetaInitialize(meta, context, loader): void; } ``` The `ModuleLoader` class now has `setCustomizations` which takes an object of this shape and delegates its responsibilities to this object if present. Note that two properties `allowImportMetaResolve` and `resolveSync` exist now as a mechanism for `import.meta.resolve` – since `Hooks` does not implement `resolveSync` other loaders cannot use `import.meta.resolve`; `allowImportMetaResolve` is a way of checking for that case instead of invoking `resolveSync` and erroring. Fixes nodejs#48515 Closes nodejs#48439 PR-URL: nodejs#48559 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Major functional changes: - Allow `import()` to work within loaders that require other loaders, - Unflag the use of `Module.register`. A new interface `Customizations` has been created in order to unify `ModuleLoader` (previously `DefaultModuleLoader`), `Hooks` and `CustomizedModuleLoader` all of which now implement it: ```ts interface LoadResult { format: ModuleFormat; source: ModuleSource; } interface ResolveResult { format: string; url: URL['href']; } interface Customizations { allowImportMetaResolve: boolean; load(url: string, context: object): Promise<LoadResult> resolve( originalSpecifier: string, parentURL: string, importAssertions: Record<string, string> ): Promise<ResolveResult> resolveSync( originalSpecifier: string, parentURL: string, importAssertions: Record<string, string> ) ResolveResult; register(specifier: string, parentUrl: string): any; forceLoadHooks(): void; importMetaInitialize(meta, context, loader): void; } ``` The `ModuleLoader` class now has `setCustomizations` which takes an object of this shape and delegates its responsibilities to this object if present. Note that two properties `allowImportMetaResolve` and `resolveSync` exist now as a mechanism for `import.meta.resolve` – since `Hooks` does not implement `resolveSync` other loaders cannot use `import.meta.resolve`; `allowImportMetaResolve` is a way of checking for that case instead of invoking `resolveSync` and erroring. Fixes #48515 Closes #48439 PR-URL: #48559 Backport-PR-URL: #50669 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Major functional changes: - Allow `import()` to work within loaders that require other loaders, - Unflag the use of `Module.register`. A new interface `Customizations` has been created in order to unify `ModuleLoader` (previously `DefaultModuleLoader`), `Hooks` and `CustomizedModuleLoader` all of which now implement it: ```ts interface LoadResult { format: ModuleFormat; source: ModuleSource; } interface ResolveResult { format: string; url: URL['href']; } interface Customizations { allowImportMetaResolve: boolean; load(url: string, context: object): Promise<LoadResult> resolve( originalSpecifier: string, parentURL: string, importAssertions: Record<string, string> ): Promise<ResolveResult> resolveSync( originalSpecifier: string, parentURL: string, importAssertions: Record<string, string> ) ResolveResult; register(specifier: string, parentUrl: string): any; forceLoadHooks(): void; importMetaInitialize(meta, context, loader): void; } ``` The `ModuleLoader` class now has `setCustomizations` which takes an object of this shape and delegates its responsibilities to this object if present. Note that two properties `allowImportMetaResolve` and `resolveSync` exist now as a mechanism for `import.meta.resolve` – since `Hooks` does not implement `resolveSync` other loaders cannot use `import.meta.resolve`; `allowImportMetaResolve` is a way of checking for that case instead of invoking `resolveSync` and erroring. Fixes nodejs/node#48515 Closes nodejs/node#48439 PR-URL: nodejs/node#48559 Backport-PR-URL: nodejs/node#50669 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jun 26, 2023

izaakschroeder force-pushed the support-nested-loader-chains branch from 26b9ab4 to 0bb4399 Compare June 26, 2023 07:32

izaakschroeder mentioned this pull request Jun 26, 2023

Possible loader chaining problem / possible yarn problem #48515

Closed

izaakschroeder marked this pull request as ready for review June 26, 2023 07:52

GeoffreyBooth requested changes Jun 26, 2023

View reviewed changes

lib/internal/modules/esm/utils.js Outdated Show resolved Hide resolved

izaakschroeder force-pushed the support-nested-loader-chains branch from 0bb4399 to d1c068c Compare June 27, 2023 22:51

izaakschroeder force-pushed the support-nested-loader-chains branch 2 times, most recently from 58d1bd1 to 30cb005 Compare June 27, 2023 22:59

GeoffreyBooth previously requested changes Jun 29, 2023

View reviewed changes

lib/internal/modules/esm/utils.js Outdated Show resolved Hide resolved

izaakschroeder force-pushed the support-nested-loader-chains branch from 30cb005 to 409484a Compare June 30, 2023 06:21

izaakschroeder changed the title ~~esm: support nested loader chains~~ esm: refactor DefaultModuleLoader Jun 30, 2023

izaakschroeder force-pushed the support-nested-loader-chains branch from 409484a to 76f775e Compare June 30, 2023 09:30

izaakschroeder commented Jun 30, 2023

View reviewed changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved

izaakschroeder commented Jun 30, 2023

View reviewed changes

test/es-module/test-esm-loader-hooks.mjs Outdated Show resolved Hide resolved

izaakschroeder force-pushed the support-nested-loader-chains branch 2 times, most recently from 7170a68 to cb657fb Compare July 4, 2023 08:45

GeoffreyBooth mentioned this pull request Jul 10, 2023

Replace globalPreload nodejs/loaders#147

Open

ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 7, 2023

targos mentioned this pull request Nov 11, 2023

[v18.x] Backport most ESM and customization hook changes #50669

Closed

targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Nov 23, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: unflag `Module.register` and allow nested loader `import()` #48559

esm: unflag `Module.register` and allow nested loader `import()` #48559

izaakschroeder commented Jun 26, 2023 •

edited

nodejs-github-bot commented Jun 26, 2023

GeoffreyBooth left a comment

izaakschroeder commented Jun 27, 2023

aduh95 commented Jun 27, 2023

izaakschroeder commented Jun 27, 2023

GeoffreyBooth left a comment

izaakschroeder commented Jun 30, 2023 •

edited

izaakschroeder Jun 30, 2023

GeoffreyBooth Jul 11, 2023

izaakschroeder Jul 12, 2023

JakobJingleheimer Jul 16, 2023

GeoffreyBooth commented Jun 30, 2023

izaakschroeder commented Jun 30, 2023 •

edited

izaakschroeder commented Jul 2, 2023

GeoffreyBooth commented Jul 3, 2023

izaakschroeder commented Jul 4, 2023

ruyadorno commented Sep 7, 2023

esm: unflag Module.register and allow nested loader import() #48559

esm: unflag Module.register and allow nested loader import() #48559

Conversation

izaakschroeder commented Jun 26, 2023 • edited

Overview

nodejs-github-bot commented Jun 26, 2023

GeoffreyBooth left a comment

Choose a reason for hiding this comment

izaakschroeder commented Jun 27, 2023

aduh95 commented Jun 27, 2023

izaakschroeder commented Jun 27, 2023

GeoffreyBooth left a comment

Choose a reason for hiding this comment

izaakschroeder commented Jun 30, 2023 • edited

izaakschroeder Jun 30, 2023

Choose a reason for hiding this comment

GeoffreyBooth Jul 11, 2023

Choose a reason for hiding this comment

izaakschroeder Jul 12, 2023

Choose a reason for hiding this comment

JakobJingleheimer Jul 16, 2023

Choose a reason for hiding this comment

GeoffreyBooth commented Jun 30, 2023

izaakschroeder commented Jun 30, 2023 • edited

izaakschroeder commented Jul 2, 2023

GeoffreyBooth commented Jul 3, 2023

izaakschroeder commented Jul 4, 2023

ruyadorno commented Sep 7, 2023

esm: unflag `Module.register` and allow nested loader `import()` #48559

esm: unflag `Module.register` and allow nested loader `import()` #48559

izaakschroeder commented Jun 26, 2023 •

edited

izaakschroeder commented Jun 30, 2023 •

edited

izaakschroeder commented Jun 30, 2023 •

edited