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: unflag Module.register and allow nested loader import() #48559

Merged

Conversation

izaakschroeder
Copy link
Contributor

@izaakschroeder izaakschroeder commented Jun 26, 2023

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
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@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 marked this pull request as ready for review June 26, 2023 07:52
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a 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.

lib/internal/modules/esm/utils.js Outdated Show resolved Hide resolved
@izaakschroeder
Copy link
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.

@aduh95
Copy link
Contributor

aduh95 commented Jun 27, 2023

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
Copy link
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 🎉

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

@GeoffreyBooth GeoffreyBooth left a 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.

lib/internal/modules/esm/utils.js Outdated Show resolved Hide resolved
@izaakschroeder izaakschroeder changed the title esm: support nested loader chains esm: refactor DefaultModuleLoader Jun 30, 2023
@izaakschroeder
Copy link
Contributor Author

izaakschroeder commented Jun 30, 2023

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 🎉

lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/initialize_import_meta.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
return this.#delegate.register(specifier, parentUrl);
}
// TODO: Consider flagging this.
const flag = true; // getOptionValue('--experimental-loader-register');
Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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. 😄

Copy link
Contributor

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 😢

lib/internal/modules/esm/loader.js Show resolved Hide resolved
lib/internal/modules/esm/loader.js Show resolved Hide resolved
@GeoffreyBooth
Copy link
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
Copy link
Contributor Author

izaakschroeder commented Jun 30, 2023

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 Outdated Show resolved Hide resolved
lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved
@izaakschroeder
Copy link
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?

@GeoffreyBooth
Copy link
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!

@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
Copy link
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.

UlisesGascon added a commit that referenced this pull request Aug 15, 2023
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
UlisesGascon added a commit that referenced this pull request Aug 16, 2023
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
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
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
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
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
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 18, 2023
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
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 22, 2023
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 22, 2023
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 22, 2023
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 22, 2023
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 22, 2023
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
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 29, 2023
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 31, 2023
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 Sep 1, 2023
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
juanarbol pushed a commit that referenced this pull request Sep 4, 2023
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 added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 7, 2023
@ruyadorno
Copy link
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 😊

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
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
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
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]>
@targos 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 pushed a commit that referenced this pull request Nov 23, 2023
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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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
Development

Successfully merging this pull request may close these issues.

9 participants