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

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

Closed

Conversation

Pranav-yadav
Copy link
Contributor

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

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
Copy link

analysis-bot commented Apr 24, 2023

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
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
Copy link
Contributor

hoxyq commented Apr 25, 2023

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";:

"engines": {
"node": ">=16"
},

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
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
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
Copy link
Contributor

jacdebug commented Apr 25, 2023

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

package.json Outdated Show resolved Hide resolved
@Pranav-yadav
Copy link
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.

package.json Outdated Show resolved Hide resolved
packages/virtualized-lists/package.json Outdated Show resolved Hide resolved
@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 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
Copy link
Contributor Author

Addressed the suggestions and feedback.

@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Apr 25, 2023

@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
Copy link
Contributor

kelset commented Apr 25, 2023

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
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
Copy link
Contributor

jacdebug commented Apr 25, 2023

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
Copy link
Contributor Author

Pranav-yadav commented Apr 25, 2023

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 😃

Copy link
Contributor

@jacdebug jacdebug left a comment

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
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
Contributor

@jacdebug merged this pull request in a58dea1.

jeongshin pushed a commit to jeongshin/react-native that referenced this pull request May 7, 2023
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:

- For RN development and new project created; bump to node 16 was in facebook#36217
- Recently `react-native-windows` also; updated node to v16, microsoft/react-native-windows#11500

#### 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

Pull Request resolved: facebook#37073

Test Plan: - `yarn lint && yarn flow && yarn test-ci` --> _should be green_

Reviewed By: cipolleschi

Differential Revision: D45306108

Pulled By: jacdebug

fbshipit-source-id: e3ba7d0151b86a6a0a3d63fb29c2bd887e1ac1e7
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
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:

- For RN development and new project created; bump to node 16 was in facebook#36217
- Recently `react-native-windows` also; updated node to v16, microsoft/react-native-windows#11500

#### 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

Pull Request resolved: facebook#37073

Test Plan: - `yarn lint && yarn flow && yarn test-ci` --> _should be green_

Reviewed By: cipolleschi

Differential Revision: D45306108

Pulled By: jacdebug

fbshipit-source-id: e3ba7d0151b86a6a0a3d63fb29c2bd887e1ac1e7
@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