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

Use Node 20 #410

Merged
merged 1 commit into from
May 28, 2024
Merged

Use Node 20 #410

merged 1 commit into from
May 28, 2024

Conversation

bestbeforetoday
Copy link
Member

@bestbeforetoday bestbeforetoday commented Nov 4, 2023

  • Code remains compatible with Node 18.
  • Project continues to be built and tested with Node 18.
  • Node 20 is used as the runtime for the nodeenv Docker image to allow chaincode access to latest Node features and security fixes.

Closes #409

@bestbeforetoday bestbeforetoday marked this pull request as ready for review November 5, 2023 00:44
@bestbeforetoday bestbeforetoday requested a review from a team as a code owner November 5, 2023 00:44
@bestbeforetoday bestbeforetoday enabled auto-merge (squash) November 5, 2023 00:44
@bestbeforetoday
Copy link
Member Author

@hyperledger/fabric-chaincode-node-maintainers Please could you review.

@denyeart
Copy link
Contributor

denyeart commented Dec 4, 2023

@bestbeforetoday

Agree we need to move to Node 20, but I think we need to figure out our release strategy first.

I've traditionally told users that a major Node update is a significant enough event that we wouldn't want to surprise people with it in a 3rd digit nodeenv release. e.g. if they are using the latest nodeenv two-digit release v2.5 (which uses Node.js 18) they'd probably expect it to continue using Node.js 18 the next time they update and build their chaincode on the latest v2.5 nodeenv.

This has been true with the last few releases:
2.3 uses Node.js 12 only
2.4 uses Node.js 16 only
2.5 uses Node.js 18 only

We've generally told people if they want to use a later Node.js release, then update to the next nodeenv minor release (2nd digit release).

Taking the logic forward, we would release a v2.6 with Node 20. However, we've also said that we don't want to actively maintain multiple branches of fabric-chaincode-node going forward. Not to mention getting out of sync with the Fabric release numbering.

So we are at a decision point here... I'm thinking we could indeed update v2.5.x to use Node.js 20, but this would be a change of policy and therefore we need to gather some opinions (I'll ask around a bit) and be explicit about the decision.

And if we do it, we'll need to make sure it is documented well. Fabric service offerings may want to switch their peer config from using fabric-nodeenv:$(TWO_DIGIT_VERSION) to using an explicit 3 digit release so as to not surprise users. This however implies that chaincode users could not get regular fixes and security updates without updating to the next Node.js runtime. This is the price of maintaining a single branch.

Thoughts @bestbeforetoday @mbwhite ?

@bestbeforetoday
Copy link
Member Author

I agree that we should deicide on a strategy moving forwards. Breaking compatibility in a patch release is definitely undesirable. Bumping the Node runtime version that we use in a patch release is not necessarily a breaking change. A data point to consider is that fabric-chaincode-node already switched from Node 16 to Node 18 between versions 2.5.2 and 2.5.3.

Let me revisit this PR to see if the test artifacts can stay consistent (i.e. written for older Node versions) while running successfully with more current Node releases. In other words, give us more confidence that changing the runtime version should not impact end-user chaincode.

@denyeart
Copy link
Contributor

denyeart commented Dec 4, 2023

A data point to consider is that fabric-chaincode-node already switched from Node 16 to Node 18 between versions 2.5.2 and 2.5.3.

Right, but we intentionally moved up to Node 18 in v2.5.3 in preparation for the Fabric v2.5.0 release when we thought people would start using it in production.

Let me revisit this PR to see if the test artifacts can stay consistent (i.e. written for older Node versions) while running successfully with more current Node releases. In other words, give us more confidence that changing the runtime version should not impact end-user chaincode.

Thanks.

@mbwhite
Copy link
Member

mbwhite commented Dec 4, 2023

Agree node 20 would be good to move to; an important consideration is how much resource is available for testing etc.

@bestbeforetoday
Copy link
Member Author

How about this? Updated dependencies and avoided some unnecessary ones, use Node 20 at runtime in the chaincode container, but build and test against Node 18 to ensure backwards compatibility.

@denyeart
Copy link
Contributor

Good that it was built and tested against Node 18. But now that it is proven to work, do we need to continue to do that going forward? Or can we safely move everything to Node 20 now that we know it works? I'm just worried that a mix of Node 18 and Node 20 will confuse people. If we do keep a mix of Node 18 and Node 20, we should be very clear in https://github.com/hyperledger/fabric-chaincode-node/blob/main/COMPATIBILITY.md about the strategy (that file needs to be updated regardless in this change).

@bestbeforetoday
Copy link
Member Author

bestbeforetoday commented Jan 1, 2024

I don't think we can move everything to Node 20. I think we need to continue to build and test with Node 18, and declare a requirement of Node >= 18 for the v2.5.x NPM packages. If we switch our packages to require Node 20 and to exploit features only available in Node 20, it might not be possible for end users to continue to build/test v2.5 chaincode with Node 18.

Provided there are no breaking changes for code developed for Node 18. we can use a Node 20 runtime to pick up the latest runtime features and fixes. Chaincode is free to develop using Node 20 and exploit Node 20 features, provided they use a patch level of the Docker image that uses Node 20. The fact that our NPM packages can also work with Node 18 should not affect that.

To be extra safe, we could perhaps unit test with both Node 18 and 20 instead of only Node 18.

The compatibility document will need to be updated to describe the versions used but that could be done as a follow-on change. This PR does not prevent Node 18 chaincode packages from being supported on v2.5, as currently documented. It just enables some additional capability that is currently undocumented.

@bestbeforetoday bestbeforetoday marked this pull request as draft March 4, 2024 12:38
auto-merge was automatically disabled March 4, 2024 12:38

Pull request was converted to draft

@bestbeforetoday bestbeforetoday marked this pull request as ready for review March 21, 2024 18:41
@bestbeforetoday bestbeforetoday enabled auto-merge (squash) March 21, 2024 18:41
@bestbeforetoday
Copy link
Member Author

@denyeart Can you review?

@bestbeforetoday bestbeforetoday removed the request for review from denyeart April 12, 2024 09:43
- Code remains compatible with Node 18.
- Project continues to be built and tested with Node 18.
- Node 20 is used as the runtime for the nodeenv Docker image to allow chaincode access to latest Node features and security fixes.

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
@bestbeforetoday bestbeforetoday merged commit 44c95bd into hyperledger:main May 28, 2024
6 checks passed
@bestbeforetoday bestbeforetoday deleted the node20 branch May 28, 2024 09:55
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.

Use Node 20
3 participants