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-yadavPranav-yadav commented Apr 24, 2023•
edited
Loading

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-botfacebook-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•
edited
Loading

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,625,235+0
androidhermesarmeabi-v7a7,937,970+0
androidhermesx869,112,303+0
androidhermesx86_648,967,000+0
androidjscarm64-v8a9,189,274+0
androidjscarmeabi-v7a8,379,467+0
androidjscx869,247,691+0
androidjscx86_649,506,040+0

Base commit: 09a810a
Branch: main

@Pranav-yadav

This comment was marked as resolved.

Sign in to view
@jacdebug

jacdebug commented Apr 25, 2023

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.

Sign in to view
@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

Pranav-yadav commented Apr 25, 2023

Copy link
Copy Markdown
ContributorAuthor

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

jacdebug commented Apr 25, 2023

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•
edited
Loading

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.

jacdebug
jacdebug reviewed Apr 25, 2023
View reviewed changes
Comment threadpackage.json Outdated
@Pranav-yadav

Pranav-yadav commented Apr 25, 2023

Copy link
Copy Markdown
ContributorAuthor

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.

jacdebug
jacdebug reviewed Apr 25, 2023
View reviewed changes
Comment threadpackage.json Outdated
Comment threadpackages/virtualized-lists/package.json Outdated
@Pranav-yadavPranav-yadav changed the title chore: specify node engine in package.jsonUpdate node to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadavforce-pushed the chore/specify-node-engine branch from 4019dff to 53793b5CompareApril 25, 2023 11:46
@Pranav-yadavPranav-yadav changed the title Update node to v16 in all RN packagesUpdate Node.js to v16 in all RN packagesApr 25, 2023
@Pranav-yadav

Pranav-yadav commented Apr 25, 2023

Copy link
Copy Markdown
ContributorAuthor

Addressed the suggestions and feedback.

@Pranav-yadav

Pranav-yadav commented Apr 25, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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

Pranav-yadav commented Apr 25, 2023

Copy link
Copy Markdown
ContributorAuthor

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•
edited
Loading

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.

@Pranav-yadav
Update Node.js to v16 in all RN packages
f41ba9f
- Enforce minimum Node.js v16 in all RN packages under 'packages/' dir
**NOTE: This is a BREAKING change**.
@Pranav-yadav
Pranav-yadavforce-pushed the chore/specify-node-engine branch from 53793b5 to f41ba9fCompareApril 25, 2023 19:01
@Pranav-yadav

Pranav-yadav commented Apr 25, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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 approved these changes Apr 26, 2023
View reviewed changes

@jacdebugjacdebug 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

facebook-github-bot commented Apr 26, 2023

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-botfacebook-github-bot added the Merged This PR has been merged. label Apr 26, 2023
@facebook-github-bot

facebook-github-bot commented Apr 26, 2023

Copy link
Copy Markdown
Contributor

@jacdebug merged this pull request in a58dea1.

@leotmleotm mentioned this pull request May 4, 2023
Open
76 tasks
@Pranav-yadav
Pranav-yadav deleted the chore/specify-node-engine branch June 15, 2023 09:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacdebugjacdebugjacdebug approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Pranav-yadav@analysis-bot@jacdebug@hoxyq@kelset@facebook-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

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-yadavPranav-yadav
commented
Apr 24, 2023•

edited

Loading

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-botfacebook-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•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,625,235+0
androidhermesarmeabi-v7a7,937,970+0
androidhermesx869,112,303+0
androidhermesx86_648,967,000+0
androidjscarm64-v8a9,189,274+0
androidjscarmeabi-v7a8,379,467+0
androidjscx869,247,691+0
androidjscx86_649,506,040+0
Base commit: 09a810a

Branch: main
@Pranav-yadav

This comment was marked as resolved.

Sign in to view
@jacdebug
jacdebug
commented
Apr 25, 2023

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.

Sign in to view
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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
jacdebug
commented
Apr 25, 2023

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•

edited

Loading

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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

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

Copy link


Copy Markdown

ContributorAuthor
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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

Comment threadpackages/virtualized-lists/package.json
Outdated

@Pranav-yadavPranav-yadav
changed the title
chore: specify node engine in package.jsonUpdate node to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
4019dff to
53793b5CompareApril 25, 2023 11:46
@Pranav-yadavPranav-yadav
changed the title
Update node to v16 in all RN packagesUpdate Node.js to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed the suggestions and feedback.
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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•

edited

Loading

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.
@Pranav-yadav
Update Node.js to v16 in all RN packages
f41ba9f
- Enforce minimum Node.js v16 in all RN packages under 'packages/' dir
**NOTE: This is a BREAKING change**.
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
53793b5 to
f41ba9fCompareApril 25, 2023 19:01
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
approved these changes
Apr 26, 2023
View reviewed changes
@jacdebugjacdebug
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
facebook-github-bot
commented
Apr 26, 2023

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-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Apr 26, 2023
@facebook-github-bot
facebook-github-bot
commented
Apr 26, 2023

Copy link


Copy Markdown

Contributor
@jacdebug merged this pull request in a58dea1.
@leotmleotm
mentioned this pull request
May 4, 2023

Open

76 tasks
@Pranav-yadav
Pranav-yadav
deleted the

chore/specify-node-engine

branch
June 15, 2023 09:08
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jacdebugjacdebugjacdebug approved these changes

Assignees


No one assigned


Labels

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

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Pranav-yadav@analysis-bot@jacdebug@hoxyq@kelset@facebook-github-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

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-yadavPranav-yadav
commented
Apr 24, 2023•

edited

Loading

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-botfacebook-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•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,625,235+0
androidhermesarmeabi-v7a7,937,970+0
androidhermesx869,112,303+0
androidhermesx86_648,967,000+0
androidjscarm64-v8a9,189,274+0
androidjscarmeabi-v7a8,379,467+0
androidjscx869,247,691+0
androidjscx86_649,506,040+0
Base commit: 09a810a

Branch: main
@Pranav-yadav

This comment was marked as resolved.

Sign in to view
@jacdebug
jacdebug
commented
Apr 25, 2023

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.

Sign in to view
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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
jacdebug
commented
Apr 25, 2023

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•

edited

Loading

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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

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

Copy link


Copy Markdown

ContributorAuthor
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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

Comment threadpackages/virtualized-lists/package.json
Outdated

@Pranav-yadavPranav-yadav
changed the title
chore: specify node engine in package.jsonUpdate node to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
4019dff to
53793b5CompareApril 25, 2023 11:46
@Pranav-yadavPranav-yadav
changed the title
Update node to v16 in all RN packagesUpdate Node.js to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed the suggestions and feedback.
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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•

edited

Loading

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.
@Pranav-yadav
Update Node.js to v16 in all RN packages
f41ba9f
- Enforce minimum Node.js v16 in all RN packages under 'packages/' dir
**NOTE: This is a BREAKING change**.
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
53793b5 to
f41ba9fCompareApril 25, 2023 19:01
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
approved these changes
Apr 26, 2023
View reviewed changes
@jacdebugjacdebug
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
facebook-github-bot
commented
Apr 26, 2023

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-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Apr 26, 2023
@facebook-github-bot
facebook-github-bot
commented
Apr 26, 2023

Copy link


Copy Markdown

Contributor
@jacdebug merged this pull request in a58dea1.
@leotmleotm
mentioned this pull request
May 4, 2023

Open

76 tasks
@Pranav-yadav
Pranav-yadav
deleted the

chore/specify-node-engine

branch
June 15, 2023 09:08
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jacdebugjacdebugjacdebug approved these changes

Assignees


No one assigned


Labels

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

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Pranav-yadav@analysis-bot@jacdebug@hoxyq@kelset@facebook-github-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

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-yadavPranav-yadav
commented
Apr 24, 2023•

edited

Loading

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-botfacebook-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•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,625,235+0
androidhermesarmeabi-v7a7,937,970+0
androidhermesx869,112,303+0
androidhermesx86_648,967,000+0
androidjscarm64-v8a9,189,274+0
androidjscarmeabi-v7a8,379,467+0
androidjscx869,247,691+0
androidjscx86_649,506,040+0
Base commit: 09a810a

Branch: main
@Pranav-yadav

This comment was marked as resolved.

Sign in to view
@jacdebug
jacdebug
commented
Apr 25, 2023

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.

Sign in to view
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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
jacdebug
commented
Apr 25, 2023

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•

edited

Loading

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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

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

Copy link


Copy Markdown

ContributorAuthor
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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

Comment threadpackages/virtualized-lists/package.json
Outdated

@Pranav-yadavPranav-yadav
changed the title
chore: specify node engine in package.jsonUpdate node to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
4019dff to
53793b5CompareApril 25, 2023 11:46
@Pranav-yadavPranav-yadav
changed the title
Update node to v16 in all RN packagesUpdate Node.js to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed the suggestions and feedback.
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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•

edited

Loading

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.
@Pranav-yadav
Update Node.js to v16 in all RN packages
f41ba9f
- Enforce minimum Node.js v16 in all RN packages under 'packages/' dir
**NOTE: This is a BREAKING change**.
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
53793b5 to
f41ba9fCompareApril 25, 2023 19:01
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
approved these changes
Apr 26, 2023
View reviewed changes
@jacdebugjacdebug
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
facebook-github-bot
commented
Apr 26, 2023

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-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Apr 26, 2023
@facebook-github-bot
facebook-github-bot
commented
Apr 26, 2023

Copy link


Copy Markdown

Contributor
@jacdebug merged this pull request in a58dea1.
@leotmleotm
mentioned this pull request
May 4, 2023

Open

76 tasks
@Pranav-yadav
Pranav-yadav
deleted the

chore/specify-node-engine

branch
June 15, 2023 09:08
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jacdebugjacdebugjacdebug approved these changes

Assignees


No one assigned


Labels

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

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Pranav-yadav@analysis-bot@jacdebug@hoxyq@kelset@facebook-github-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

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-yadavPranav-yadav
commented
Apr 24, 2023•

edited

Loading

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-botfacebook-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•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,625,235+0
androidhermesarmeabi-v7a7,937,970+0
androidhermesx869,112,303+0
androidhermesx86_648,967,000+0
androidjscarm64-v8a9,189,274+0
androidjscarmeabi-v7a8,379,467+0
androidjscx869,247,691+0
androidjscx86_649,506,040+0
Base commit: 09a810a

Branch: main
@Pranav-yadav

This comment was marked as resolved.

Sign in to view
@jacdebug
jacdebug
commented
Apr 25, 2023

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.

Sign in to view
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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
jacdebug
commented
Apr 25, 2023

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•

edited

Loading

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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

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

Copy link


Copy Markdown

ContributorAuthor
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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

Comment threadpackages/virtualized-lists/package.json
Outdated

@Pranav-yadavPranav-yadav
changed the title
chore: specify node engine in package.jsonUpdate node to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
4019dff to
53793b5CompareApril 25, 2023 11:46
@Pranav-yadavPranav-yadav
changed the title
Update node to v16 in all RN packagesUpdate Node.js to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed the suggestions and feedback.
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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•

edited

Loading

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.
@Pranav-yadav
Update Node.js to v16 in all RN packages
f41ba9f
- Enforce minimum Node.js v16 in all RN packages under 'packages/' dir
**NOTE: This is a BREAKING change**.
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
53793b5 to
f41ba9fCompareApril 25, 2023 19:01
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
approved these changes
Apr 26, 2023
View reviewed changes
@jacdebugjacdebug
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
facebook-github-bot
commented
Apr 26, 2023

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-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Apr 26, 2023
@facebook-github-bot
facebook-github-bot
commented
Apr 26, 2023

Copy link


Copy Markdown

Contributor
@jacdebug merged this pull request in a58dea1.
@leotmleotm
mentioned this pull request
May 4, 2023

Open

76 tasks
@Pranav-yadav
Pranav-yadav
deleted the

chore/specify-node-engine

branch
June 15, 2023 09:08
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jacdebugjacdebugjacdebug approved these changes

Assignees


No one assigned


Labels

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

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Pranav-yadav@analysis-bot@jacdebug@hoxyq@kelset@facebook-github-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

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-yadavPranav-yadav
commented
Apr 24, 2023•

edited

Loading

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-botfacebook-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•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,625,235+0
androidhermesarmeabi-v7a7,937,970+0
androidhermesx869,112,303+0
androidhermesx86_648,967,000+0
androidjscarm64-v8a9,189,274+0
androidjscarmeabi-v7a8,379,467+0
androidjscx869,247,691+0
androidjscx86_649,506,040+0
Base commit: 09a810a

Branch: main
@Pranav-yadav

This comment was marked as resolved.

Sign in to view
@jacdebug
jacdebug
commented
Apr 25, 2023

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.

Sign in to view
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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
jacdebug
commented
Apr 25, 2023

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•

edited

Loading

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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

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

Copy link


Copy Markdown

ContributorAuthor
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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

Comment threadpackages/virtualized-lists/package.json
Outdated

@Pranav-yadavPranav-yadav
changed the title
chore: specify node engine in package.jsonUpdate node to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
4019dff to
53793b5CompareApril 25, 2023 11:46
@Pranav-yadavPranav-yadav
changed the title
Update node to v16 in all RN packagesUpdate Node.js to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed the suggestions and feedback.
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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•

edited

Loading

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.
@Pranav-yadav
Update Node.js to v16 in all RN packages
f41ba9f
- Enforce minimum Node.js v16 in all RN packages under 'packages/' dir
**NOTE: This is a BREAKING change**.
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
53793b5 to
f41ba9fCompareApril 25, 2023 19:01
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
approved these changes
Apr 26, 2023
View reviewed changes
@jacdebugjacdebug
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
facebook-github-bot
commented
Apr 26, 2023

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-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Apr 26, 2023
@facebook-github-bot
facebook-github-bot
commented
Apr 26, 2023

Copy link


Copy Markdown

Contributor
@jacdebug merged this pull request in a58dea1.
@leotmleotm
mentioned this pull request
May 4, 2023

Open

76 tasks
@Pranav-yadav
Pranav-yadav
deleted the

chore/specify-node-engine

branch
June 15, 2023 09:08
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jacdebugjacdebugjacdebug approved these changes

Assignees


No one assigned


Labels

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

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Pranav-yadav@analysis-bot@jacdebug@hoxyq@kelset@facebook-github-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

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-yadavPranav-yadav
commented
Apr 24, 2023•

edited

Loading

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-botfacebook-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•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,625,235+0
androidhermesarmeabi-v7a7,937,970+0
androidhermesx869,112,303+0
androidhermesx86_648,967,000+0
androidjscarm64-v8a9,189,274+0
androidjscarmeabi-v7a8,379,467+0
androidjscx869,247,691+0
androidjscx86_649,506,040+0
Base commit: 09a810a

Branch: main
@Pranav-yadav

This comment was marked as resolved.

Sign in to view
@jacdebug
jacdebug
commented
Apr 25, 2023

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.

Sign in to view
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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
jacdebug
commented
Apr 25, 2023

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•

edited

Loading

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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

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

Copy link


Copy Markdown

ContributorAuthor
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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

Comment threadpackages/virtualized-lists/package.json
Outdated

@Pranav-yadavPranav-yadav
changed the title
chore: specify node engine in package.jsonUpdate node to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
4019dff to
53793b5CompareApril 25, 2023 11:46
@Pranav-yadavPranav-yadav
changed the title
Update node to v16 in all RN packagesUpdate Node.js to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed the suggestions and feedback.
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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•

edited

Loading

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.
@Pranav-yadav
Update Node.js to v16 in all RN packages
f41ba9f
- Enforce minimum Node.js v16 in all RN packages under 'packages/' dir
**NOTE: This is a BREAKING change**.
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
53793b5 to
f41ba9fCompareApril 25, 2023 19:01
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
approved these changes
Apr 26, 2023
View reviewed changes
@jacdebugjacdebug
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
facebook-github-bot
commented
Apr 26, 2023

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-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Apr 26, 2023
@facebook-github-bot
facebook-github-bot
commented
Apr 26, 2023

Copy link


Copy Markdown

Contributor
@jacdebug merged this pull request in a58dea1.
@leotmleotm
mentioned this pull request
May 4, 2023

Open

76 tasks
@Pranav-yadav
Pranav-yadav
deleted the

chore/specify-node-engine

branch
June 15, 2023 09:08
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jacdebugjacdebugjacdebug approved these changes

Assignees


No one assigned


Labels

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

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Pranav-yadav@analysis-bot@jacdebug@hoxyq@kelset@facebook-github-bot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

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-yadavPranav-yadav
commented
Apr 24, 2023•

edited

Loading

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-botfacebook-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•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,625,235+0
androidhermesarmeabi-v7a7,937,970+0
androidhermesx869,112,303+0
androidhermesx86_648,967,000+0
androidjscarm64-v8a9,189,274+0
androidjscarmeabi-v7a8,379,467+0
androidjscx869,247,691+0
androidjscx86_649,506,040+0
Base commit: 09a810a

Branch: main
@Pranav-yadav

This comment was marked as resolved.

Sign in to view
@jacdebug
jacdebug
commented
Apr 25, 2023

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.

Sign in to view
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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
jacdebug
commented
Apr 25, 2023

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•

edited

Loading

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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

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

Copy link


Copy Markdown

ContributorAuthor
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.
jacdebug
jacdebug
reviewed
Apr 25, 2023
View reviewed changes
Comment threadpackage.json
Outdated

Comment threadpackages/virtualized-lists/package.json
Outdated

@Pranav-yadavPranav-yadav
changed the title
chore: specify node engine in package.jsonUpdate node to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
4019dff to
53793b5CompareApril 25, 2023 11:46
@Pranav-yadavPranav-yadav
changed the title
Update node to v16 in all RN packagesUpdate Node.js to v16 in all RN packagesApr 25, 2023
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Addressed the suggestions and feedback.
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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
Pranav-yadav
commented
Apr 25, 2023

Copy link


Copy Markdown

ContributorAuthor
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•

edited

Loading

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.
@Pranav-yadav
Update Node.js to v16 in all RN packages
f41ba9f
- Enforce minimum Node.js v16 in all RN packages under 'packages/' dir
**NOTE: This is a BREAKING change**.
@Pranav-yadav
Pranav-yadavforce-pushed
the
chore/specify-node-engine
branch
from
53793b5 to
f41ba9fCompareApril 25, 2023 19:01
@Pranav-yadav
Pranav-yadav
commented
Apr 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
approved these changes
Apr 26, 2023
View reviewed changes
@jacdebugjacdebug
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
facebook-github-bot
commented
Apr 26, 2023

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-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Apr 26, 2023
@facebook-github-bot
facebook-github-bot
commented
Apr 26, 2023

Copy link


Copy Markdown

Contributor
@jacdebug merged this pull request in a58dea1.
@leotmleotm
mentioned this pull request
May 4, 2023

Open

76 tasks
@Pranav-yadav
Pranav-yadav
deleted the

chore/specify-node-engine

branch
June 15, 2023 09:08
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jacdebugjacdebugjacdebug approved these changes

Assignees


No one assigned


Labels

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

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@Pranav-yadav@analysis-bot@jacdebug@hoxyq@kelset@facebook-github-bot