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

[Consensus] Support Node 20 #46

Merged
merged 7 commits into from
Feb 19, 2024

Conversation

Rishi556
Copy link
Contributor

@Rishi556 Rishi556 commented Jun 25, 2023

Adds support for Node.JS 20.

The error message logged during deployment needed to be changed from using node's error message to a custom error message as node's internal changed how the message would be spit out. For now I put the deployment date as after hive block 76149072 which is approximately July 25th.

Old message from node: SyntaxError: Unexpected identifier
New message from node: SyntaxError: Unexpected identifier 'CODE'

To handle older blocks, anything with "SyntaxError: Unexpected identifier" forcibly becomes just "SyntaxError: Unexpected identifier", removing the extra stuff which stated what was the unidentified variable.

@bt-cryptomancer bt-cryptomancer marked this pull request as draft July 9, 2023 02:43
@bt-cryptomancer
Copy link
Contributor

@Rishi556 I've converted this to draft status as I'm not comfortable making such a fundamental change for the time being. We can revisit this in September when I'm back from summer travels.

@Rishi556
Copy link
Contributor Author

Rishi556 commented Aug 5, 2023

I do think we should try and update as new releases are made, especially since Node now has a team dedicated to performance. Doing updates in smaller batches and keeping up will make it a lot easier than trying to fix support for a few node versions at once which might have many breaking changes, vs doing 1 at a time might lead to just a few. BTW this will need to be changed before deployment to change the block num.

@Rishi556
Copy link
Contributor Author

Rishi556 commented Feb 9, 2024

@bt-cryptomancer Can you take a look at this. We'll have to change block number but I'd like to get this one merged in before Node 22 gets released in about 2 months

Copy link
Contributor

@eonwarped eonwarped left a comment

Choose a reason for hiding this comment

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

Small comment, looks fine to me.

libs/SmartContracts.js Outdated Show resolved Hide resolved
libs/SmartContracts.js Show resolved Hide resolved
@Rishi556 Rishi556 marked this pull request as ready for review February 12, 2024 22:29
@Rishi556
Copy link
Contributor Author

Rishi556 commented Feb 12, 2024

Node 16 is EOL(and is approaching 6 months of EOL) as well, thoughts on dropping it from the test matrix?

@eonwarped
Copy link
Contributor

Node 16 is EOL(and is approaching 6 months of EOL) as well, thoughts on dropping it from the test matrix?

deferring to @bt-cryptomancer

@eonwarped eonwarped changed the title Support Node 20 [Consensus] Support Node 20 Feb 14, 2024
@bt-cryptomancer
Copy link
Contributor

@Rishi556 I'm fine to have Node 16 removed from the test matrix. If we're going to do that, let's also update the minimum node version in the package.json to 18+

I will update the trigger block number after merging this to QA and after other consensus breaking changes are merged in.

Update minimum engine version to latest 18
@bt-cryptomancer
Copy link
Contributor

Looks good now, but conflict needs to be resolved before merging.

@bt-cryptomancer bt-cryptomancer dismissed eonwarped’s stale review February 19, 2024 04:30

requested changes have been made

@bt-cryptomancer bt-cryptomancer merged commit df66251 into hive-engine:qa Feb 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants