GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — esm: unflag `Module.register` and allow nested loader `import()` by izaakschroeder · Pull Request #48559 · nodejs/node

Conversation

@izaakschroeder

@izaakschroeder izaakschroeder commented Jun 26, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Akinte2 reacted with thumbs up emoji bricss reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 👀 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Jun 26, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot 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
izaakschroeder force-pushed the support-nested-loader-chains branch from 26b9ab4 to 0bb4399 Compare June 26, 2023 07:32
@izaakschroeder izaakschroeder mentioned this pull request Jun 26, 2023
Possible loader chaining problem / possible yarn problem #48515
Closed
@izaakschroeder
izaakschroeder marked this pull request as ready for review June 26, 2023 07:52
GeoffreyBooth
GeoffreyBooth requested changes Jun 26, 2023
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/internal/modules/esm/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@izaakschroeder

izaakschroeder commented Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Jun 27, 2023

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

izaakschroeder commented Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

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 🎉

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@izaakschroeder
izaakschroeder force-pushed the support-nested-loader-chains branch 2 times, most recently from 58d1bd1 to 30cb005 Compare June 27, 2023 22:59
GeoffreyBooth
GeoffreyBooth previously requested changes Jun 29, 2023
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JakobJingleheimer reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread lib/internal/modules/esm/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@izaakschroeder
izaakschroeder force-pushed the support-nested-loader-chains branch from 30cb005 to 409484a Compare June 30, 2023 06:21
@izaakschroeder izaakschroeder changed the title esm: support nested loader chains esm: refactor DefaultModuleLoader Jun 30, 2023
@izaakschroeder

izaakschroeder commented Jun 30, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

image

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 🎉

GeoffreyBooth reacted with thumbs up emoji JakobJingleheimer reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@izaakschroeder
izaakschroeder force-pushed the support-nested-loader-chains branch from 409484a to 76f775e Compare June 30, 2023 09:30
izaakschroeder
izaakschroeder commented Jun 30, 2023
View reviewed changes
Comment thread lib/internal/modules/esm/hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/initialize_import_meta.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/loader.js Outdated

@izaakschroeder izaakschroeder Jun 30, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@GeoffreyBooth GeoffreyBooth Jul 11, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@izaakschroeder izaakschroeder Jul 12, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@JakobJingleheimer JakobJingleheimer Jul 16, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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 😢

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/internal/modules/esm/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/es-module/test-esm-loader-programmatically.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth

GeoffreyBooth commented Jun 30, 2023

Copy link
Copy Markdown
Member

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 and Lupthaet reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@izaakschroeder

izaakschroeder commented Jun 30, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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 😄

Akinte2 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

izaakschroeder
izaakschroeder commented Jun 30, 2023
View reviewed changes
Comment thread lib/internal/modules/esm/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

izaakschroeder
izaakschroeder commented Jun 30, 2023
View reviewed changes
Comment thread test/es-module/test-esm-loader-hooks.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@izaakschroeder

izaakschroeder commented Jul 2, 2023

Copy link
Copy Markdown
Contributor Author

@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?

Akinte2 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth

GeoffreyBooth commented Jul 3, 2023

Copy link
Copy Markdown
Member

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!

Akinte2 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

izaakschroeder commented Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

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.

Akinte2 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth GeoffreyBooth mentioned this pull request Jul 10, 2023
Replace globalPreload nodejs/loaders#147
Closed
@privatenumber privatenumber mentioned this pull request Aug 7, 2023
How to: programmatically register the hooks esbuild-kit/esm-loader#62
Closed
@JakobJingleheimer JakobJingleheimer added this to the ESM Loaders: Stable (CJS parity) milestone Aug 13, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@izaakschroeder @Ceres6
esm: unflag Module.register and allow nested loader import()
63ca7e5
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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@izaakschroeder @Ceres6
esm: unflag Module.register and allow nested loader import()
be858db
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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
@izaakschroeder @UlisesGascon
esm: unflag Module.register and allow nested loader import()
08ac866
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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@izaakschroeder @RafaelGSS
esm: unflag Module.register and allow nested loader import()
0a4f7c6
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
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
v20.6.0 proposal #49185
Merged
UlisesGascon added a commit that referenced this pull request Aug 16, 2023
@UlisesGascon
2023-08-16, Version 20.6.0 (Current)
0df82ac
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
UlisesGascon added a commit that referenced this pull request Aug 17, 2023
@UlisesGascon
2023-08-16, Version 20.6.0 (Current)
c9c813a
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
UlisesGascon added a commit that referenced this pull request Aug 17, 2023
@UlisesGascon
2023-08-21, Version 20.6.0 (Current)
dfe3820
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
UlisesGascon added a commit that referenced this pull request Aug 17, 2023
@UlisesGascon
2023-08-21, Version 20.6.0 (Current)
44d51da
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
UlisesGascon added a commit that referenced this pull request Aug 18, 2023
@UlisesGascon
2023-08-21, Version 20.6.0 (Current)
6673b86
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
UlisesGascon added a commit that referenced this pull request Aug 22, 2023
@UlisesGascon
2023-08-23, Version 20.6.0 (Current)
4148741
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
UlisesGascon added a commit that referenced this pull request Aug 24, 2023
@UlisesGascon
2023-08-28, Version 20.6.0 (Current)
df92c38
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

ruyadorno commented Sep 7, 2023

Copy link
Copy Markdown
Member

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 😊

GeoffreyBooth reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos targos mentioned this pull request Nov 11, 2023
[v18.x] Backport most ESM and customization hook changes #50669
Closed
@targos targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
@inoway46 inoway46 mentioned this pull request Jul 31, 2026
Static and dynamic imports inside loader code should have same behaviour #43244
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@anonrig anonrig anonrig left review comments
@aduh95 aduh95 aduh95 left review comments
@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

commit-queue-squash PRs the Commit Queue should land as one squashed commit. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

ESM Loaders: Stable (CJS parity)

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@izaakschroeder @nodejs-github-bot @aduh95 @GeoffreyBooth @JakobJingleheimer @ruyadorno @ljharb @anonrig @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.