chore!: use node v20 throughout monorepo #5730
Merged
Conversation
This file contains 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
wemeetagain
changed the title
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
|
Can you confirm the performance regression locally? Seems legit |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| describe("Override preset", function () { | ||
| // Allow time for ts-node to compile Typescript source | ||
| this.timeout(30_000); | ||
|
|
||
| it("Should correctly override preset", async () => { | ||
| await exec(`${tsNodeBinary} ${path.join(__dirname, scriptNames.ok)}`); | ||
| await exec(`node --loader ts-node/esm ${path.join(__dirname, scriptNames.ok)}`); |
Check warning
Code scanning / CodeQL
Shell command built from environment values
| }); | ||
|
|
||
| it("Should throw trying to override preset in the wrong order", async () => { | ||
| await expect(exec(`${tsNodeBinary} ${path.join(__dirname, scriptNames.error)}`)).to.be.rejectedWith( | ||
| await expect(exec(`node --loader ts-node/esm ${path.join(__dirname, scriptNames.error)}`)).to.be.rejectedWith( |
Check warning
Code scanning / CodeQL
Shell command built from environment values
|
|
||
| describe("setPreset", function () { | ||
| // Allow time for ts-node to compile Typescript source | ||
| this.timeout(30_000); | ||
|
|
||
| it("Should correctly set preset", async () => { | ||
| await exec(`${tsNodeBinary} ${path.join(__dirname, scriptNames.ok)}`); | ||
| await exec(`node --loader ts-node/esm ${path.join(__dirname, scriptNames.ok)}`); |
Check warning
Code scanning / CodeQL
Shell command built from environment values
| }); | ||
|
|
||
| it("Should throw trying to set preset in the wrong order", async () => { | ||
| await expect(exec(`${tsNodeBinary} ${path.join(__dirname, scriptNames.error)}`)).to.be.rejectedWith( | ||
| await expect(exec(`node --loader ts-node/esm ${path.join(__dirname, scriptNames.error)}`)).to.be.rejectedWith( |
Check warning
Code scanning / CodeQL
Shell command built from environment values
|
This error is only happening in CI, not locally |
nflaig
reviewed
Jul 14, 2023
Can't reproduce this either, installed several different node versions. Might have to check which node version is actually used in the CI. |
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
|
No idea what is going on with sim tests, @nazarhussain any ideas? |
This reverts commit e1f07be.
|
Based on the error, seems like the beacon node had not started.
|
|
I think it is somehow related with error thrown after updating cross-fetch, still can't make sense of why this produces correct results locally. |
|
Internally the error says lodestar could not connect to the EL. But Geth is running fine, so it seems like some firewall issue on docker which didn't allowed connecting the host machine to docker container on local IP.
|
|
I updated cross-fetch because the error was present, but it didn't fix it. |
|
It seems that node 20 now has |
This reverts commit d296b2f.
philknows
previously approved these changes
Jul 14, 2023
nflaig
previously approved these changes
Jul 14, 2023
nflaig
reviewed
Jul 16, 2023
| await expect(eth1JsonRpcClient.fetch(payload, {timeout})).to.be.rejectedWith(error); | ||
|
|
||
| try { | ||
| await eth1JsonRpcClient.fetch(payload, {timeout}); |
There was a problem hiding this comment.
@nazarhussain try/catch is problematic here, test case would pass if no error is thrown
Either have to do explicit fail if there is no error like it is done here
or can use .to.be.rejected.then pattern
There was a problem hiding this comment.
Yes I knew that pattern, but we discussed earlier to not use the nested then/catch and prefer try/catch.
There was a problem hiding this comment.
In that case need to explicitly fail the test if there is no error
nflaig
reviewed
Jul 16, 2023
| @@ -8,7 +8,7 @@ | |||
| [](https://github.com/ethereum/consensus-specs/releases/tag/v1.1.10) | |||
| [](https://codecov.io/gh/ChainSafe/lodestar) | |||
|  | |||
|  | |||
|  | |||
There was a problem hiding this comment.
@wemeetagain I updated this to 20.x as well, not sure if it was intentionally not updated before but as we discussed that we want to advocate for using node 20 I think we should update it here as well.
Last reference to 18.x is in package.json but that might be to disruptive for users to update to 20.x, although we could ensure everyone is using node 20 this way
Line 5 in 1887287
nflaig
reviewed
Jul 16, 2023
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| ## Prerequisites | |||
|
|
|||
| Make sure to have [Yarn installed](https://classic.yarnpkg.com/en/docs/install). It is also recommended to [install NVM (Node Version Manager)](https://github.com/nvm-sh/nvm) and use the LTS version (currently v18) of [NodeJS](https://nodejs.org/en/). | |||
| Make sure to have [Yarn installed](https://classic.yarnpkg.com/en/docs/install). It is also recommended to [install NVM (Node Version Manager)](https://github.com/nvm-sh/nvm) and use the LTS version (currently v20) of [NodeJS](https://nodejs.org/en/). | |||
There was a problem hiding this comment.
Just a note: v20 is not yet LTS version until 2023-10-24, see nodejs-release-working-group for schedule
nflaig
approved these changes
Jul 16, 2023
|
Ok, when CI is finished running, I'll change the branch rules and merge this |
Comment on lines
156
to
162
| await expect(eth1JsonRpcClient.fetch(payload, {timeout})).to.be.rejected.then((error) => { | ||
| if (testCase.errorCode) { | ||
| expect((error as FetchError).code).to.eql(testCase.errorCode); | ||
| expect((error as FetchError).code).to.be.equal(testCase.errorCode); | ||
| } else { | ||
| expect((error as Error).message).includes(testCase.error); | ||
| expect((error as Error).message).to.include(testCase.error); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
I think we discussed to no use the nested then/catch and prefer try/catch.
There was a problem hiding this comment.
I was not part of that discussion but in my opinion try/catch in tests should be last resort if there are no other options to use built-in assertion syntax
|
Regarding the failing sim tests on node 20, it looks like the issue is related to node-fetch and Lighthouse. I am using the script mentioned here #5737 (comment) to fetch the state. For There is an open issue for it already Edit: I created an issue to track this |
|
🎉 This PR is included in v1.10.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
Since #5611, we support node v20, but we aren't using it everywhere
Description
Update docs, workflows, types, and docker images to use node 20
TODO