[HOLD] Use Node 16 and NPM 8 going forward

[AndrewGable]

Details

This introduces a change to force usage of node 16 and npm 8 in order to better support M1 machines.

When running npm install on a machine with node 14 you will see this error:

andrew ➜ (andrew-node-16) App nvm use 14
Now using node v14.19.3 (npm v6.14.17)
andrew ➜ (andrew-node-16) App npm install
npm ERR! code ENOTSUP
npm ERR! notsup Unsupported engine for [email protected]: wanted: {"node":"16.15.1","npm":"8.11.0"} (current: {"node":"14.19.3","npm":"6.14.17"})
npm ERR! notsup Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Required: {"node":"16.15.1","npm":"8.11.0"}
npm ERR! notsup Actual:   {"npm":"6.14.17","node":"14.19.3"}

I will send out a Action Required email to internal employees in addition to posting a message in Slack for contributors.

Fixed Issues

https://expensify.slack.com/archives/C01GTK53T8Q/p1657213944275459

Tests

1. Run npm install on an M1 and Intel machine- verify it installs ✅
2. Set up the zsh integration for nvm and verify when changing directory into App you see:

Found '/Users/andrew/Expensidev/App/.nvmrc' with version <16.15.1>
Now using node v16.15.1 (npm v8.11.0)

PR Review Checklist

Contributor (PR Author) Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • Any functional components have the displayName property
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

[AndrewGable]

Ok seeing the same error locally as in tests:

andrew ➜ (andrew-node-16) App npm install
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: [email protected]
npm ERR! Found: @react-navigation/[email protected]
npm ERR! node_modules/@react-navigation/native
npm ERR!   @react-navigation/native@"6.0.11" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer @react-navigation/native@"^5.0.5" from @react-navigation/[email protected]
npm ERR! node_modules/@react-navigation/compat
npm ERR!   @react-navigation/compat@"5.3.20" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR! See /Users/andrew/.npm/eresolve-report.txt for a full report.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/andrew/.npm/_logs/2022-07-13T00_13_32_086Z-debug-0.log

[AndrewGable]

Seems like it's the same as this: react-navigation/react-navigation#9932

[AndrewGable]

Ok now npm install works but npm ci does not work

[AndrewGable]

I will attempt to fix this warning before merge, but not sure it needs to block merge:

npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: [email protected]
npm WARN Found: [email protected]
npm WARN node_modules/eslint
npm WARN   dev eslint@"^7.32.0" from the root project
npm WARN   23 more (@babel/eslint-parser, @lwc/eslint-plugin-lwc, ...)
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer eslint@"^8.1.0" from [email protected]
npm WARN node_modules/@react-native-community/eslint-config/node_modules/eslint-plugin-flowtype
npm WARN   eslint-plugin-flowtype@"^8.0.0" from @react-native-community/[email protected]
npm WARN   node_modules/@react-native-community/eslint-config
npm WARN
npm WARN Conflicting peer dependency: [email protected]
npm WARN node_modules/eslint
npm WARN   peer eslint@"^8.1.0" from [email protected]
npm WARN   node_modules/@react-native-community/eslint-config/node_modules/eslint-plugin-flowtype
npm WARN     eslint-plugin-flowtype@"^8.0.0" from @react-native-community/[email protected]
npm WARN     node_modules/@react-native-community/eslint-config

[AndrewGable]

Finally got GitHub actions happy here. There will probably be quite a few conflicts due to the nature of these changes, so I will resolve them all once we feel comfortable with the changes. I do still need to do some client testing since I upgraded a few node dependencies, so I will take off hold once this is ready.

[AndrewGable]

Seeing this error on web

[AndrewGable]

Seems like we have mismatching versions of react-dom 16 and 17 on web due to expensify-common https://github.com/Expensify/expensify-common/blob/main/package.json#L27-L28

https://reactjs.org/warnings/invalid-hook-call-warning.html

[AndrewGable]

Seems like the real reason for this web error is mentioned in this comment here: #8888 (comment)

Something with react-native-web and the way we custom compile is causing this error

[AndrewGable]

OK so here is an interesting predicament.. Our react-native-web version is now pretty out of date because we haven't been keeping it up to date with upstream. If we move back to upstream, it does in fact fix the issue above, however, it reintroduces this issue: #1341 because we never made a PR for this fix for the upstream repo.

My proposed solution would be:

1. Move back to react-native-web upstream latest 0.18.5
2. Create an upstream PR with the fix for copy/paste out of order
3. Live with the copy paste bug to fix M1 development for contributors until we get the upstream PR merged

Thoughts @roryabraham @marcaaron @alex-mechler @tgolen ?

[AndrewGable]

Note to self: Probably should revert this change

[roryabraham]

My proposed solution would be:

1. Move back to react-native-web upstream latest 0.18.5
2. Create an upstream PR with the fix for copy/paste out of order
3. Live with the copy paste bug to fix M1 development for contributors until we get the upstream PR merged

How hard would it be to update the fork to the latest and manually resolve conflicts to keep our copy/paste fixes while we wait for the upstream PR to be reviewed?

we never made a PR for this fix for the upstream repo

We should definitely do this 🙂

[AndrewGable]

I don't think it would be extremely difficult to keep our repo up to date with the upstream changes, but it would still prevent M1 users from running on web until we figured out why our custom compile script is throwing hook errors.

[marcaaron]

I am OK with that plan as it would put some urgency behind getting the fix merged for real.

I read through this and it seems like our advice (but not policy?) is to merge the upstream into the fork?

[marcaaron]

Looks like we tried to open a PR -> necolas/react-native-web#2151

But does not look like it got a response so we are probably taking the wrong approach. Have we tried to contact the maintainer?

[AndrewGable]

Alternative solution provided by @marcaaron as we were chatting, using --legacy-peer-deps as described here: https://stackoverflow.com/questions/66239691/what-does-npm-install-legacy-peer-deps-do-exactly-when-is-it-recommended-wh

[roryabraham]

I don't think it would be extremely difficult to keep our repo up to date with the upstream changes, but it would still prevent M1 users from running on web until we figured out why our custom compile script is throwing hook errors

I don't love how we have to recompile react-native-web every time we run an npm install. A couple alternatives would be:

1. to publish our fork on npm and then use it like we would any other package
2. To use patch-package instead of a fork to apply our fix when we run npm installs

Given that we already have a fork of react-native-web my recommendation would be that we publish it to npm and use it like any other package

[AndrewGable]

So how about an updated proposal:

1. Update our fork with upstream
2. Publish it to npm
3. Remove scripts/react-native-web.sh
4. Create an upstream PR
5. Move off our fork once our PR is merged & deployed

[AndrewGable]

Going to come back to this in smaller PRs. This has gotten way too big to do in one.