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.

Sign up for GitHub

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

Jump to bottom

build: update node to v22.12 #28368

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

build: update node to v22.12 #28368

wants to merge 2 commits into from
+13 −13

Conversation

HowardBraham
Copy link
Contributor

@HowardBraham HowardBraham commented Nov 7, 2024 •
edited
Loading

Description

Node LTS just moved to 22, let's start using it.

Keeping this as-is in package.json for now:
"node": ">= 20",

Was previously blocked by 101arrowz/fflate#227
Just merged this and now it's unblocked #29177

Open in GitHub Codespaces

@HowardBraham HowardBraham requested review from kumavis and a team as code owners November 7, 2024 20:45
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the team-tiger Tiger team (for tech debt reduction + performance improvements) label Nov 7, 2024
@HowardBraham HowardBraham self-assigned this Nov 7, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [48d757f]
Page Load Metrics (1833 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16142270183416378
domContentLoaded16002184181015675
load16132250183316177
domInteractive137347178
backgroundConnect874282010
firstReactRender48169982411
getState45514157
initialActions01000
loadScripts11251641132613665
setupStore65414168
uiStartup18002468203417886
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [b0dcd1e]
Page Load Metrics (1613 ± 38 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1501190016398340
domContentLoaded1490181315927235
load1503188316137938
domInteractive178748209
backgroundConnect76623168
firstReactRender4610287168
getState45512147
initialActions00000
loadScripts1078137311666732
setupStore647994
uiStartup1684209117839244
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

davidmurdoch
davidmurdoch reviewed Nov 8, 2024
View reviewed changes
.circleci/config.yml Outdated
@@ -3,13 +3,13 @@ version: 2.1
executors:
node-browsers-small:
docker:
- image: cimg/node:20.17-browsers
- image: cimg/node:current-browsers
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use current. Tests must always be as deterministic as possible.

@metamaskbot
Copy link
Collaborator

Builds ready [14d098e]
Page Load Metrics (1940 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17892253194211153
domContentLoaded17362234191811153
load17872255194011053
domInteractive18105582311
backgroundConnect97629178
firstReactRender492901126833
getState580272613
initialActions01000
loadScripts1252169413879847
setupStore65114147
uiStartup19902638219918388
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@HowardBraham HowardBraham marked this pull request as draft November 8, 2024 20:53
auto-merge was automatically disabled November 8, 2024 20:53

Pull request was converted to draft

@HowardBraham
Copy link
Contributor Author

@davidmurdoch in development/webpack/utils/plugins/ManifestPlugin/index.ts, are we supposed to be using AsyncZipDeflate or ZipDeflate? Because I think AsyncZipDeflate is what's hanging in Node 22.

@davidmurdoch
Copy link
Contributor

@davidmurdoch in development/webpack/utils/plugins/ManifestPlugin/index.ts, are we supposed to be using AsyncZipDeflate or ZipDeflate? Because I think AsyncZipDeflate is what's hanging in Node 22.

It uses AsyncZipDeflate, as this allows it to compress many files on many Workers simultaneously (utilizing all of the machine's available threads/cores), rather than blocking on the main thread.

from the docs:

there is an asynchronous version of every method as well. [...] this will cause the compression or decompression run in a separate thread entirely and automatically by using [...] Workers. This means that the processing will not block the main thread at all.

I benchmarked this on my local machine when I implemented it and it measurably was faster to use AsyncZipDeflate, despite the overhead of starting up the workers.

@davidmurdoch davidmurdoch mentioned this pull request Nov 19, 2024
Open
@davidmurdoch davidmurdoch mentioned this pull request Dec 12, 2024
Merged
@HowardBraham HowardBraham changed the title build: update node to v22.11 build: update node to v22.12 Dec 13, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [5fb6104]
Page Load Metrics (1689 ± 76 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint47019621552380183
domContentLoaded14162132167116177
load14242140168915976
domInteractive248043188
backgroundConnect75321147
firstReactRender1599482813
getState45414168
initialActions01000
loadScripts10301683122414972
setupStore685172010
uiStartup16632501198419795
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

github-merge-queue bot pushed a commit that referenced this pull request Dec 16, 2024
@davidmurdoch
build(webpack): fix --zip in Node.js 22 by cloning assets into `Uin…
58ca7af 
…t8Array`s before zipping (#29177)

Use a copy of the Buffer via `Buffer.from(asset)`, as Zip will *consume*
it, which breaks things if we are compiling for multiple browsers at
once. `Buffer.from` uses the internal pool, so it's superior to `new
Uint8Array` if we don't need to pass it off to a worker thread.

Additionally, in Node.js 22+ a Buffer marked as "Untransferable" (like
ours) can't be passed to a worker, which `AsyncZipDeflate` uses. See:
101arrowz/fflate#227 (comment)

This can probably be simplified to `zipFile.push(Buffer.from(asset),
true);` if the above issue is resolved.

This fix should hopefully unblock
#28368


<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.


## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29177?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**


### **Before**


### **After**


## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

-->
@HowardBraham HowardBraham marked this pull request as ready for review December 16, 2024 18:13
@HowardBraham HowardBraham requested review from a team as code owners December 16, 2024 18:13
@HowardBraham HowardBraham changed the base branch from main to prep-deps-and-self-hosted December 16, 2024 18:16
@HowardBraham HowardBraham requested review from a team as code owners December 16, 2024 18:16
@HowardBraham HowardBraham changed the base branch from prep-deps-and-self-hosted to main December 16, 2024 18:16
@metamaskbot
Copy link
Collaborator

Builds ready [8d7e500]
Page Load Metrics (1665 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint34020521614332159
domContentLoaded14271978164014268
load14492053166514871
domInteractive21121422612
backgroundConnect96128178
firstReactRender1597463014
getState476222211
initialActions01000
loadScripts9901516122712560
setupStore66313157
uiStartup169824141959227109
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@HowardBraham HowardBraham removed request for a team and kumavis December 16, 2024 19:03
@davidmurdoch
Copy link
Contributor

I think this PR should also update @tsconfig/node20 to @tsconfig/node22 (needs to be updated in depcheck as well). Also the readme references Node v20 (Install [Node.js](https://nodejs.org) version 20).

danjm pushed a commit that referenced this pull request Dec 18, 2024
@davidmurdoch @danjm
build(webpack): fix --zip in Node.js 22 by cloning assets into `Uin…
6b143a9 
…t8Array`s before zipping (#29177)

Use a copy of the Buffer via `Buffer.from(asset)`, as Zip will *consume*
it, which breaks things if we are compiling for multiple browsers at
once. `Buffer.from` uses the internal pool, so it's superior to `new
Uint8Array` if we don't need to pass it off to a worker thread.

Additionally, in Node.js 22+ a Buffer marked as "Untransferable" (like
ours) can't be passed to a worker, which `AsyncZipDeflate` uses. See:
101arrowz/fflate#227 (comment)

This can probably be simplified to `zipFile.push(Buffer.from(asset),
true);` if the above issue is resolved.

This fix should hopefully unblock
#28368


<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.


## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29177?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**


### **Before**


### **After**


## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

-->
@socket-security Socket Security
Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@tsconfig/[email protected] None 0 2.36 kB typescript-deploys

🚮 Removed packages: npm/@tsconfig/[email protected]

View full report↗︎

@HowardBraham
Copy link
Contributor Author

I think this PR should also update @tsconfig/node20 to @tsconfig/node22 (needs to be updated in depcheck as well). Also the readme references Node v20 (Install [Node.js](https://nodejs.org) version 20).

@davidmurdoch good call, done

@metamaskbot
Copy link
Collaborator

Builds ready [613236d]
Page Load Metrics (1607 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint50419221534283136
domContentLoaded13331921158417082
load13401950160717685
domInteractive23124392612
backgroundConnect95723157
firstReactRender156631199
getState46416178
initialActions01000
loadScripts9461415117513264
setupStore671202110
uiStartup15242241183020397
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@HowardBraham HowardBraham added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dabossdana dabossdana dabossdana approved these changes

@davidmurdoch davidmurdoch davidmurdoch approved these changes

@Gudahtt Gudahtt Awaiting requested review from Gudahtt

@MajorLift MajorLift Awaiting requested review from MajorLift

@hjetpoluru hjetpoluru Awaiting requested review from hjetpoluru

At least 2 approving reviews are required to merge this pull request.

Assignees

@HowardBraham HowardBraham

Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-tiger Tiger team (for tech debt reduction + performance improvements)
Projects
PR review queue
Status: Needs dev review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HowardBraham @metamaskbot @davidmurdoch @dabossdana