Skip to content

Update Node.js to v16 in all RN packages#37073

Closed
Pranav-yadav wants to merge 1 commit into
react:mainfrom
Pranav-yadav:chore/specify-node-engine
Closed

Update Node.js to v16 in all RN packages#37073
Pranav-yadav wants to merge 1 commit into
react:mainfrom
Pranav-yadav:chore/specify-node-engine

Conversation

@Pranav-yadav

@Pranav-yadav Pranav-yadav commented Apr 24, 2023

Copy link
Copy Markdown
Contributor

Summary:

NOTE: This is a BREAKING change.
TLDR; Enforce minimum Node.js v16 in all RN packages.

This diff Updates Node.js to v16 across all RN packages.

Context:

Changes:

  • [BREAKING] Update Node.js to v16 across all RN packages under 'packages/' dir

Changelog:

[GENERAL][BREAKING] - Update Node.js to v16 in all RN packages

Test Plan:

  • yarn lint && yarn flow && yarn test-ci --> should be green

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 24, 2023
@analysis-bot

analysis-bot commented Apr 24, 2023

Copy link
Copy Markdown
Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,625,235 +0
android hermes armeabi-v7a 7,937,970 +0
android hermes x86 9,112,303 +0
android hermes x86_64 8,967,000 +0
android jsc arm64-v8a 9,189,274 +0
android jsc armeabi-v7a 8,379,467 +0
android jsc x86 9,247,691 +0
android jsc x86_64 9,506,040 +0

Base commit: 09a810a
Branch: main

@Pranav-yadav

This comment was marked as resolved.

@jacdebug

Copy link
Copy Markdown
Contributor

Thanks for working on this, looks like this will be a breaking change, is there somewhere documented current minimum requirement for Node.js @Pranav-yadav.

@Pranav-yadav

This comment was marked as resolved.

@hoxyq

hoxyq commented Apr 25, 2023

Copy link
Copy Markdown
Contributor

Hi @jacdebug, I forgot to link to the conversation related to this diff.

Context: Recently I asked following question to @cipolleschi #36891 (comment)

Also, a general question:
what is lowest Nodejs engine compatibility provided RN packages (at least in case of codegen pkg)?
Also, for other RN packages whom should I ask this to?
Edit: Actually, the main question is about ECMAScript versions compatibility :)

His reply: #36891 (comment)

Hi @Pranav-yadav, you can see the engine we support from the package.json. Apps created from the template have a similar block in their package.json

In-short the package.json of packages/react-native specifies "node": ">=16";:

https://github.com/facebook/react-native/blob/0e5d54a8ee8f43cf39c3ee9b47acdcb933a762ab/packages/react-native/package.json#L8-L10

and I think it was specified by @hoxyq during Monorepo transition, may be he can give more context.

Bump to node 16 was in #36217

@Pranav-yadav

Copy link
Copy Markdown
Contributor Author

Bump to node 16 was in #36217

Thanks, for pointing to the correct diff. The git blame tricked me ;)
I'll update the PR description to add that context for more accessibility.

@jacdebug

Copy link
Copy Markdown
Contributor

The PR is only enforcing minimum 16 for RN development and new project created. #36217

When we add minimum Node.js version to all other package it will be a breaking change for all other. Given Node.js 20 is released I think this should be fine. (or is bit early to enforce it not sure? cc @robhogan)

Can you change title and desc to reflect actual change in PR, it is not just change in engine. Also update changelog to breaking. TIA

@jacdebug

jacdebug commented Apr 25, 2023

Copy link
Copy Markdown
Contributor

Also just checked RNW landed last day "Update Node to v16" as well - microsoft/react-native-windows#11500 so LGTM on Node.js engine.

Comment thread package.json Outdated
@Pranav-yadav

Copy link
Copy Markdown
Contributor Author

Sure.
I'll do the required changes. That makes sense.

About, if it's too early to enforce, most of the node pkgs enforce node 16 these days. Soon, we'll start getting errors and warnings about the same from RN external deps while trying to install deps and building.

PS. Release of node 20 could be one of the reasons, but, IIRC node 20 will be LTS after Oct 2023.

Comment thread package.json Outdated
Comment thread packages/virtualized-lists/package.json Outdated
@Pranav-yadav Pranav-yadav changed the title chore: specify node engine in package.json Update node to v16 in all RN packages Apr 25, 2023
@Pranav-yadav Pranav-yadav force-pushed the chore/specify-node-engine branch from 4019dff to 53793b5 Compare April 25, 2023 11:46
@Pranav-yadav Pranav-yadav changed the title Update node to v16 in all RN packages Update Node.js to v16 in all RN packages Apr 25, 2023
@Pranav-yadav Pranav-yadav requested a review from jacdebug April 25, 2023 12:28
@Pranav-yadav

Copy link
Copy Markdown
Contributor Author

Addressed the suggestions and feedback.

@Pranav-yadav

Pranav-yadav commented Apr 25, 2023

Copy link
Copy Markdown
Contributor Author

@jacdebug Since we'll be enforcing node.js >=16, shouldn't we add CI tests to check if it builds and pass the tests on those supported versions of Node.js; for public RN packages? Or it's not necessary?

cc @cortinico / @cipolleschi / @kelset

@kelset

kelset commented Apr 25, 2023

Copy link
Copy Markdown
Contributor

IIRC there's already a suite of tests for both the "current" nodesj version and the previous lts, test_js and test_js_prev_lts - so please make sure to upgrade the versions in the circleCI config too -> https://github.com/facebook/react-native/blob/main/.circleci/config.yml

@Pranav-yadav

Copy link
Copy Markdown
Contributor Author

IIRC there's already a suite of tests for both the "current" nodesj version and the previous lts, test_js and test_js_prev_lts - so please make sure to upgrade the versions in the circleCI config too -> https://github.com/facebook/react-native/blob/main/.circleci/config.yml

The config uses 16.18.1 and 18.12.1 so, apart from using the LTS specified on Node.js ie. Gallium for test_js_prev_lts & Hydrogen for test_js, I'm not sure if there needs to any other changes?
If that's correct, let me know, so I can open a PR with bumps to respective LTS, if not; please ask someone from team to do the required changes.

PS. I'm not too familiar with Circle CI configs.

@jacdebug

jacdebug commented Apr 25, 2023

Copy link
Copy Markdown
Contributor

The config uses 16.18.1 and 18.12.1 so, apart from using the LTS specified on Node.js ie. Gallium for test_js_prev_lts & Hydrogen for test_js, I'm not sure if there needs to any other changes?
If that's correct, let me know, so I can open a PR with bumps to respective LTS, if not; please ask someone from team to do the required changes.
PS. I'm not too familiar with Circle CI configs.

Thanks for looking into it and finding inconsistencies. Minimum requirement is correct no need to update now.

- Enforce minimum Node.js v16 in all RN packages under 'packages/' dir

**NOTE: This is a BREAKING change**.
@Pranav-yadav Pranav-yadav force-pushed the chore/specify-node-engine branch from 53793b5 to f41ba9f Compare April 25, 2023 19:01
@Pranav-yadav

Pranav-yadav commented Apr 25, 2023

Copy link
Copy Markdown
Contributor Author

Appreciated, then it is better if you can create a new PR after adding Readme with link? Updating Node.js engine is a good change after all and like to merge that.

@jacdebug keeping this PR atomic; only about the Update to Node.js v16. Reverted other minor touchups which will be added in another separate PR. EDIT: PR for adding missing READMEs and touchups to packages.json for all public RN packages: #37090

This PR is ready to merge after review 😃

@jacdebug jacdebug left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM and thank you for this contribution.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@jacdebug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@jacdebug merged this pull request in a58dea1.

@Pranav-yadav Pranav-yadav deleted the chore/specify-node-engine branch June 15, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Type: Breaking Change💥

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants