Update to node 16 and npm 8 via `nvm` and updated many node packages #10439

AndrewGable · 2022-08-18T00:06:39Z

Details

Update to Node 16 and npm 8, there were quite a few updates that needed to be applied due to the peer dependency changes in npm 7. In npm 7 peer dependencies are installed by default, which creates a much stricter way of installing peer dependencies. For example, react-native-web upgraded to React 18, so now all other packages needed to be upgraded to use React 18.

This caused us to need to upgrade all of these packages:

@onfido/react-native-sdk - React 18
@pieter-pot/react-native-fast-image - React 18
@react-native-community/cameraroll - React 18
react-dom - React 18
react-pdf - React 18
react-plaid-link - React 18
@storybook/addon-react-native-web - React 18
@welldone-software/why-did-you-render - React 18
babel-plugin-react-native-web - react-native-web upgrade
eslint-plugin-jsx-a11y - Wasn't installed previously
react-native-flipper - React 18/ RN upgrade
rn-async-storage-flipper - Removed, I verified with @marcaaron this was OK.
tests/unit/nativeVersionUpdaterTest.js - Changes to Node 16 updated function return type

Fixed Issues

$ #10331

Tests

Verify that no errors appear in the JS console

Run a general "smoke" test of the app by visiting lots of pages and doing lots of actions.

PR Review Checklist

Contributor (PR Author) Checklist

QA Steps

Run a smoke test of the app, lots of libraries were changed

Screenshots

Web

Desktop

iOS

Android

package.json

AndrewGable · 2022-08-18T00:08:24Z

package.json

    "@storybook/addon-a11y": "^6.5.9",
    "@storybook/addon-essentials": "^6.5.9",
-    "@storybook/addon-react-native-web": "0.0.18",
+    "@storybook/addon-react-native-web": "0.0.19--canary.37.cb55428.0",


Hack: This is a PR, but PR has been open for quite some time.

AndrewGable · 2022-08-18T00:08:47Z

package.json

    "react-native-clean-project": "^4.0.0-alpha4.0",
-    "react-native-flipper": "^0.146.1",
+    "react-native-flipper": "https://gitpkg.now.sh/facebook/flipper/react-native/react-native-flipper?9cacc9b59402550eae866e0e81e5f0c2f8203e6b",


Hack: This was merged a few hours before I made this PR, I assume it will land soon-ish.

According to their GitHub, this should land in 0.160.0 but I haven't seen it hit the npm registry yet.

.github/workflows/platformDeploy.yml

AndrewGable · 2022-08-18T20:00:57Z

GitHub actions are finally happy. I will work on cleaning this up some more, then open it up for review once it's clean!

luacmartins

LGTM and tests well on all platforms!

parasharrajat · 2022-08-19T09:18:05Z

💥 💥

parasharrajat · 2022-08-19T09:26:49Z

.github/workflows/platformDeploy.yml

@@ -31,7 +31,7 @@ jobs:
    if: ${{ fromJSON(needs.validateActor.outputs.IS_DEPLOYER) }}
    runs-on: ubuntu-latest
    steps:
-      - uses: Expensify/App/.github/actions/composite/setupNode@main
+      - uses: Expensify/App/.github/actions/composite/setupNode@andrew-npm-8


What about using the local file https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-an-action-in-the-same-repository-as-the-workflow?

I think the idea is that we want to lock it to a branch, but we cannot lock it to main until this PR is merged. Using local files would work, but it wouldn't lock it to a commit.

Then I would say lock to a commit instead of the branch name. But I got it.

NikkiWines

👍 aside from Rajat's comment!

parasharrajat · 2022-08-19T16:44:01Z

We should change the PR title and more details as a lot of things are changed on this PR.

AndrewGable · 2022-08-19T16:54:34Z

Updated, let me know if you think it's missing anything more!

roryabraham

No blockers, LGTM 👍

roryabraham · 2022-08-19T18:11:16Z

tests/unit/nativeVersionUpdaterTest.js

-            })
-    ));
+    ])('updateAndroidVersion("%s", "%s")', (versionName, versionCode, expected) => {
+        updateAndroidVersion(versionName, versionCode).then(() => {


NAB because I don't think we've standardized on this, but generally .then goes on the next line from what I've seen

roryabraham · 2022-08-19T18:12:43Z

src/setup/platformSetup/index.native.js

@@ -25,9 +25,6 @@ export default function () {
    // Setup Flipper plugins when on dev
    if (__DEV__) {
        require('flipper-plugin-bridgespy-client');
-        const RNAsyncStorageFlipper = require('rn-async-storage-flipper').default;


Why is this not needed anymore?

It was breaking due to peer dependencies and the library not being maintained

I believe @marcaaron said this was a debugging tool that is not used anymore

It might be used by some (but I haven't used it in a long time). There are workarounds to this package e.g. if you need to know the contents of AsyncStorage you can always log out values. Might be interesting to do our own Flipper plugin for Onyx at some point 🤔

marcaaron

npm install worked well for me after adding the correct node version

marcaaron · 2022-08-19T18:17:23Z

ios/Podfile

@@ -7,7 +7,6 @@ install! 'cocoapods', :deterministic_uuids => false
 target 'NewExpensify' do
  permissions_path = '../node_modules/react-native-permissions/ios'

-  pod 'Plaid', '~> 2.3.1'


Is this auto-linked in a newer version?

Yes correct. I don't know the history behind why we added this, but I think we should rely on auto linking all pod libraries unless there is a specific version breaking for us.

IIRC the history is just that the Plaid react native package required manual linking.

I don't understand their documentation.. It says it uses auto linking, so we shouldn't need to add it to the Podfile manually.

Plaid 2.5.1 is installed via autolinking and can be seen in the Podfile.lock: https://github.com/Expensify/App/pull/10439/files#diff-b7d85c35701918f25c9b2c418249f9b66fd9b067676e295a9eff493b73ee1752R231

And the Plaid SDK seems to boot just fine:

AndrewGable · 2022-08-19T18:52:12Z

Ok all reviewers have approved so I'm going to:

mdneyazahmad · 2022-08-20T05:10:57Z

package.json

+  "engines": {
+    "node": "16.15.1",
+    "npm": "8.11.0"
+  },


This produces error when the node and npm version does not match exactly. Developer has to use the exact version. Maybe, we are fine with 16.x and 8.x.

I think the error is intentional and we want to use the exact version to avoid any harder to debug errors. It should be easy to install and change the version of node if you use nvm.

OSBotify · 2022-08-23T19:11:38Z

🚀 Deployed to staging by @AndrewGable in version: 1.1.89-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Update to node 16 and npm 8

c59b3cb

AndrewGable requested a review from roryabraham August 18, 2022 00:06

AndrewGable self-assigned this Aug 18, 2022

AndrewGable commented Aug 18, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

AndrewGable commented Aug 18, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

AndrewGable commented Aug 18, 2022

View reviewed changes

AndrewGable added 8 commits August 17, 2022 18:17

Fix tests

d374f29

Try to use v3

cefecae

Try to use 16 string to test

446d607

Update to use correct version

65a8ce0

Add eslint-plugin-jsx-a11y

d672709

Rebuild actions

02516b0

Tweak where eslint checks

fc40d5e

Merge branch 'main' into andrew-npm-8

454b8bb

AndrewGable commented Aug 18, 2022

View reviewed changes

.github/workflows/platformDeploy.yml Show resolved Hide resolved

AndrewGable added 4 commits August 18, 2022 13:37

Use forked and published package with node support

b221450

Merge branch 'main' into andrew-npm-8

8a3fae0

Rebuild actions

d1da6e5

Fix test

735d9c6

AndrewGable mentioned this pull request Aug 18, 2022

Update to React 18 peerDependency Expensify/react-native-sdk#1

Merged

AndrewGable added 2 commits August 18, 2022 14:36

Use Expensify fork for @onfido/react-native-sdk

aaa8b14

Update iOS

27a6de9

AndrewGable marked this pull request as ready for review August 18, 2022 21:33

AndrewGable requested a review from a team as a code owner August 18, 2022 21:33

AndrewGable requested a review from luacmartins August 18, 2022 21:33

melvin-bot bot requested review from NikkiWines and removed request for a team August 18, 2022 21:33

Update README with change to nvm/npm/node

c2186d2

Make wording more clear

2be58c1

luacmartins approved these changes Aug 18, 2022

View reviewed changes

luacmartins mentioned this pull request Aug 18, 2022

Update react-dom #10411

Closed

parasharrajat reviewed Aug 19, 2022

View reviewed changes

NikkiWines approved these changes Aug 19, 2022

View reviewed changes

AndrewGable changed the title ~~Update to node 16 and npm 8~~ Update to node 16 and npm 8 via nvm and updated many node packages Aug 19, 2022

AndrewGable mentioned this pull request Aug 19, 2022

update react-native-svg version #10334

Merged

91 tasks

parasharrajat approved these changes Aug 19, 2022

View reviewed changes

roryabraham approved these changes Aug 19, 2022

View reviewed changes

marcaaron approved these changes Aug 19, 2022

View reviewed changes

AndrewGable merged commit bef2d47 into main Aug 19, 2022

AndrewGable deleted the andrew-npm-8 branch August 19, 2022 18:52

AndrewGable mentioned this pull request Aug 19, 2022

Use main now that andrew-npm-8 is merged #10463

Merged

mdneyazahmad reviewed Aug 20, 2022

View reviewed changes

OSBotify mentioned this pull request Aug 23, 2022

Deploy Checklist: New Expensify 2022-08-23 #10498

Closed

48 tasks

danieldoglas mentioned this pull request Aug 25, 2022

Browserslist: caniuse-lite is outdated - reported by @parasharrajat #10511

Closed

rayane-djouah mentioned this pull request May 9, 2024

[Snyk] Security upgrade react-pdf from 7.7.1 to 7.7.3 #41869

Merged

50 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to node 16 and npm 8 via `nvm` and updated many node packages #10439

Update to node 16 and npm 8 via `nvm` and updated many node packages #10439

AndrewGable commented Aug 18, 2022 •

edited

Loading

AndrewGable Aug 18, 2022

AndrewGable Aug 18, 2022

AndrewGable Aug 18, 2022

AndrewGable commented Aug 18, 2022

luacmartins left a comment

parasharrajat commented Aug 19, 2022

parasharrajat Aug 19, 2022

AndrewGable Aug 19, 2022

parasharrajat Aug 19, 2022 •

edited

Loading

NikkiWines left a comment

parasharrajat commented Aug 19, 2022

AndrewGable commented Aug 19, 2022

roryabraham left a comment

roryabraham Aug 19, 2022

roryabraham Aug 19, 2022

AndrewGable Aug 19, 2022

marcaaron Aug 19, 2022

marcaaron left a comment

marcaaron Aug 19, 2022

AndrewGable Aug 19, 2022

marcaaron Aug 19, 2022

AndrewGable Aug 19, 2022

AndrewGable commented Aug 19, 2022

mdneyazahmad Aug 20, 2022

luacmartins Aug 20, 2022

OSBotify commented Aug 23, 2022

Update to node 16 and npm 8 via nvm and updated many node packages #10439

Update to node 16 and npm 8 via nvm and updated many node packages #10439

Conversation

AndrewGable commented Aug 18, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

QA Steps

Screenshots

Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewGable commented Aug 18, 2022

luacmartins left a comment

Choose a reason for hiding this comment

parasharrajat commented Aug 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

NikkiWines left a comment

Choose a reason for hiding this comment

parasharrajat commented Aug 19, 2022

AndrewGable commented Aug 19, 2022

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewGable commented Aug 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Aug 23, 2022

Update to node 16 and npm 8 via `nvm` and updated many node packages #10439

Update to node 16 and npm 8 via `nvm` and updated many node packages #10439

AndrewGable commented Aug 18, 2022 •

edited

Loading

parasharrajat Aug 19, 2022 •

edited

Loading