Uh oh!
There was an error while loading. Please reload this page.
[v18.x] Backport most ESM and customization hook changes - #50669
Closed
targos wants to merge 75 commits into
Closed
Conversation
PR-URL: nodejs#44710 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com> Co-authored-by: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#47541 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: nodejs#47548 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: nodejs#47551 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>
Those have been deprecated for a while, it's time. PR-URL: nodejs#47580 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Use the default loader as the cascaded loader in the loader worker. Otherwise we spawn loader workers in the loader workers indefinitely. PR-URL: nodejs#47620Fixes: nodejs#47566 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#47668 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#47964Fixes: nodejs#47929 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: nodejs#48247 Refs: nodejs#48240 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: nodejs#48249Fixes: nodejs#48240 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>
This avoids initializing arrays that we never use, and simplifies the implementation overall. PR-URL: nodejs#48296 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#48424 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: nodejs#46826 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#48779Fixes: nodejs#48778Fixes: nodejs#48516 Refs: nodejs#46402 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>
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.
Fixesnodejs#48515Closesnodejs#48439
PR-URL: nodejs#48559
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>PR-URL: nodejs#46662 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: nodejs#48880 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Some tests are assuming they will be run from a directory that do not contain any quote or special character in its path. That assumption is not necessary, using `JSON.stringify` or `pathToFileURL` ensures the test can be run whatever the path looks like. PR-URL: nodejs#48958 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#48960 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Both are valid characters for file names on non-Windows systems. PR-URL: nodejs#48959 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#48999 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Follows @giltayar's proposed API: > `register` can pass any data it wants to the loader, which will be passed to the exported `initialize` function of the loader. Additionally, if the user of `register` wants to communicate with the loader, it can just create a `MessageChannel` and pass the port to the loader as data. The `register` API is now: ```ts interface Options { parentUrl?: string; data?: any; transferList?: any[]; } function register(loader: string, parentUrl?: string): any; function register(loader: string, options?: Options): any; ``` This API is backwards compatible with the old one (new arguments are optional and at the end) and allows for passing data into the new `initialize` hook. If this hook returns data it is passed back to `register`: ```ts function initialize(data: any): Promise<any>; ``` **NOTE**: Currently there is no mechanism for a loader to exchange ownership of something back to the caller. Refs: nodejs/loaders#147 PR-URL: nodejs#48842 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: nodejs#48990 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#49060 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: nodejs#49105 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: nodejs#49038 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: nodejs#49028 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
PR-URL: nodejs#49069Fixes: nodejs#49026 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: nodejs#49158 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit
that referenced
this pull request
Nov 23, 2023
In #39175, better ESM errors were introduced. This commit tweaks the language in the error slightly to make it clear that there are three different options to resolve the error. Refs: #39175 PR-URL: #49521 Backport-PR-URL: #50669 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
targos pushed a commit
that referenced
this pull request
Nov 23, 2023
PR-URL: #48477 Backport-PR-URL: #50669 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
anonrig
commented
Nov 28, 2023
Member
@targos This PR includes changes important for Sentry and instrumentation on Node.js 18 with ESM. Is there any blockers to land this in v18? |
targos
commented
Nov 28, 2023
MemberAuthor
This has already landed. I'm not sure I understand the question. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
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.
List of commits backported in this PR (in reverse order):
test-node-output-errors(Antoine du Hamel) test: refactortest-node-output-errors#48992test-loaders-workers-spawned(Antoine du Hamel) test: fixtest-loaders-workers-spawnedflakiness #50251test-esm-loader-resolve-type(Antoine du Hamel) test: deflaketest-esm-loader-resolve-type#50273--default-type=module#49986getCwdSafeinternal util fn (João Lenon) util: addgetCWDURLinternal util fn #48434--experimental-default-typeflag to flip module defaults #49869import.meta.resolve(Antoine du Hamel) doc: add missing history info forimport.meta.resolve#49700import.meta.resolve(Antoine du Hamel) esm: fix return type ofimport.meta.resolve#49698URLinstances inregister(Antoine du Hamel) esm: fix support forURLinstances inregister#49655Module.registerandinitializehook (Antoine du Hamel) test: increase coverage ofModule.registerandinitializehook #49532globalPreloadtests (Geoffrey Booth) esm: isolate globalPreload #49545ExportedHooks(Antoine du Hamel) typings: fix missing property inExportedHooks#49567Module.register(Antoine du Hamel) esm: remove return value forModule.register#49529globalPreloadtests #49493globalPreloadtests #49493tmpdir.fileURL()(LiviaMedeiros) test: addtmpdir.fileURL()#49040import.meta.resolvedocumentation (Antoine du Hamel) doc: editimport.meta.resolvedocumentation #49247module.register(Geoffrey Booth) doc: add signature formodule.register#49251test-esm-loader-hooks(Antoine du Hamel) test: reduce flakiness oftest-esm-loader-hooks#49248import.meta.resolvein custom loaders (Jacob Smith) doc: caveat unavailability ofimport.meta.resolvein custom loaders #49242test-esm-loader-hooksfor easier debugging (Antoine du Hamel) test: refactortest-esm-loader-hooksfor easier debugging #49131initialize()docs (Antoine du Hamel) doc: fix name of the flag ininitialize()docs #49158globalPreloadwarning (Antoine du Hamel) esm: fixglobalPreloadwarning #49069test-esm-loader-hooks(Antoine du Hamel) test: reduce flakiness oftest-esm-loader-hooks#49105ERR_UNSUPPORTED_DIR_IMPORTagainst prototype pollution (Antoine du Hamel) esm: protectERR_UNSUPPORTED_DIR_IMPORTagainst prototype pollution #49060fixtures.fileURLwhen appropriate (Antoine du Hamel) test: usefixtures.fileURLwhen appropriate #48990initializehook, integrate withregister(Izaak Schroeder) esm: addinitializehook, integrate withregister#48842parentUrl->parentURL(Antoine du Hamel) esm: fix typoparentUrl->parentURL#48999common.mjsin ASCII order (Antoine du Hamel) test: ordercommon.mjsimports and exports in ASCII order #48960mkdtempaccept buffers and URL #48828es-module/test-esm-initialization(Antoine du Hamel) test: fixes-module/test-esm-initialization#48880Module.registerand allow nested loaderimport()(Izaak Schroeder) esm: unflagModule.registerand allow nested loaderimport()#48559globalPreloadtests and fix failing ones (Antoine du Hamel) esm: add backglobalPreloadtests #48779registerutility (João Lenon) module: implementregisterutility #46826importinternal method (Antoine du Hamel) esm: remove support for arrays inimportinternal method #48296globalPreloadhook returning a nullish value (Antoine du Hamel) esm: handleglobalPreloadhook returning a nullish value #48249'beforeExit'on the main thread (Antoine du Hamel) esm: do not use'beforeExit'on the main thread #47964URLCanParseto be consistent (Antoine du Hamel) esm: renameURLCanParseto be consistent #47668import.metaon eval (Antoine du Hamel) esm: initializeimport.metaon eval #47551process.exitfrom the loader thread to the main thread (Antoine du Hamel) esm: propagateprocess.exitfrom the loader thread to the main thread #47548/cc @nodejs/loaders