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

[v19.x backport] esm: move hooks handling into separate class #46511

Closed

Conversation

GeoffreyBooth
Copy link
Member

PR-URL: #45869
Reviewed-By: Antoine du Hamel [email protected]
Reviewed-By: Jacob Smith [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Matteo Collina [email protected]

@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. v19.x labels Feb 6, 2023
GeoffreyBooth added a commit to GeoffreyBooth/node that referenced this pull request Feb 6, 2023
PR-URL: nodejs#45869
Backport-PR-URL: nodejs#46511
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

rubberstamp

@GeoffreyBooth GeoffreyBooth added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Feb 6, 2023
RafaelGSS and others added 17 commits March 13, 2023 15:16
Signed-off-by: RafaelGSS <[email protected]>
PR-URL: nodejs#44004
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#46771
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: nodejs#46799
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: nodejs#46800
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#46801
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#46802
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#46803
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#46804
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Avoid the process 'exit' event handler and use execFile instead of
manual stream operations.

Refs: nodejs#46751
PR-URL: nodejs#46805
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fixes: nodejs#43446
PR-URL: nodejs#46781
Reviewed-By: theanarkh <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
We didn't catch this in nodejs#45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: nodejs#46764
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#46811
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
For example, my `/etc/os-release` file begins with

```
PRETTY_NAME="Ubuntu 22.04.2 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
```

so in order to match the regexp here, the `/m` flag is necessary.

PR-URL: nodejs#46838
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#46842
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
Fixes: nodejs#46765
PR-URL: nodejs#46818
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
tniessen and others added 21 commits March 13, 2023 15:59
Node.js has so far only supported user-defined DHE parameters and even
recommended generating custom parameters. This change lets users set the
dhparam option to 'auto' instead, in which case DHE parameters of
sufficient strength are selected automatically (from a small set of
well-known parameters). This has been recommended by OpenSSL for quite a
while, and it makes it much easier for Node.js TLS servers to properly
support DHE-based perfect forward secrecy.

This also updates the documentation to prioritize ECDHE over DHE, mostly
because the former tends to be more efficient and is enabled by default.

PR-URL: nodejs#46978
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Amend Tier 3 category definition to remove a
tentative clause and introduce a more concrete one.

Refs: nodejs#42802
PR-URL: nodejs#42805
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
- debugger: add string validation for watch(expr)
- debugger: add help document for watch(index)
- test: add test for watch(index) command
- doc: add information on unwatch(index) option

PR-URL: nodejs#46947
Reviewed-By: Kohei Ueno <[email protected]>
Refs: nodejs/performance#56
PR-URL: nodejs#46810
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#47053
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Qingyu Deng <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#45930
Fixes: nodejs#45929
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#46440
Fixes: nodejs#45833
Refs: nodejs#45833
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#46540
Fixes: nodejs#46448
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#46737
Fixes: nodejs#46747
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#46759
Reviewed-By: Deokjin Kim <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#46835
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
PR-URL: nodejs#46577
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#46848
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
PR-URL: nodejs#46867
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
PR-URL: nodejs#46872
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#46888
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#46881
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
This commit updates the test harness and root test to track
when bootstrapping has completed.

PR-URL: nodejs#46962
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
This commit updates the test harness to re-throw uncaught errors
if bootstrapping has not completed. This updates the existing
logic which tried to detect a specific error code.

PR-URL: nodejs#46962
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
This commit addresses a previously untested branch of the code.
It is possible when using the test runner that an error occurs
outside of a test. In this case, the test runner would simply
rethrow the error. This commit updates the logic to handle the
error in the same fashion as other uncaughtExceptions.

PR-URL: nodejs#46962
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@targos
Copy link
Member

targos commented Mar 13, 2023

It looks like this PR was forgotten. @GeoffreyBooth can you update it so I include it in tomorrow's release?

PR-URL: nodejs#45869
Backport-PR-URL: nodejs#46511
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@GeoffreyBooth
Copy link
Member Author

Quickly rebased. Let’s please stop backporting lint/style change PRs that cause conflicts in pending PRs, at least until all the pending PRs are merged in first. It just causes tons of conflicts for little to no gain, and resolving all those conflicts might introduce bugs. Lint/style changes are the lowest priority; they can be backported dead last after everything else they might otherwise introduce conflicts into. cc @aduh95

targos pushed a commit that referenced this pull request Mar 14, 2023
PR-URL: #45869
Backport-PR-URL: #46511
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos
Copy link
Member

targos commented Mar 14, 2023

Thanks. Landed in 5eb5be8

@targos targos closed this Mar 14, 2023
@GeoffreyBooth GeoffreyBooth deleted the backport-hooks-class branch March 14, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.