Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[No QA] Upgrade react native CLI to 6.2.0 #7117

Merged
merged 3 commits into from
Jan 11, 2022
Merged

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Jan 11, 2022

Details

Fixes a local build error I'm seeing on Android: https://expensify.slack.com/archives/C01GTK53T8Q/p1641859243262700

Note: I followed the custom upgrade instructions here.
Note: I had to manually install fbjs wonday/react-native-pdf#592

Fixed Issues

$ n/a

Tests

  1. Run rm -rf node_modules
  2. Run npm i
  3. Run react-native start --reset-cache
  4. Run the app on all platforms – verify that they build and run successfully.
  • Verify that no (new) errors appear in the JS console

QA Steps

None.

Tested On

  • Web
  • Mobile Web This is the same build as web, the client makes no difference wrt to the CLI
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Desktop

image

iOS

Android

@roryabraham roryabraham self-assigned this Jan 11, 2022
@roryabraham roryabraham marked this pull request as ready for review January 11, 2022 01:23
@roryabraham roryabraham requested a review from a team as a code owner January 11, 2022 01:23
@MelvinBot MelvinBot requested review from neil-marcellini and removed request for a team January 11, 2022 01:24
@roryabraham
Copy link
Contributor Author

Tagging in @Jag96 for another review because we chatted about this 1:1

@roryabraham
Copy link
Contributor Author

Curious why there is a diff in the compiled GitHub actions? I am too. I'm not sure I have a 100% crystal clear understanding of exactly what happened here, but my hand-wavy explanation is:

  1. The old version of the CLI came bundled with https://www.npmjs.com/package/fbjs as a peer dependency. The new version does not, so upgrading implicitly removed that code from our node_modules.
  2. Anyways, one of the dependencies used in our GitHub Actions must have somehow been wrapped up with the fbjs module such that ncc was including fbjs (or some subset thereof) in the compiled GitHub Actions output. But now that fbjs is included as its own separate dependency, ncc knows that it's not imported by our GitHub Actions. So all that code is no longer in the compiled ncc output.

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

I've got a Podfile.lock diff but the changes look unrelated, just confirming that there wasn't anything that had to be pushed there.

The GH actions diff is interesting, I built the actions locally and confirmed there isn't a diff so not sure what the cause is.

Anyways, one of the dependencies used in our GitHub Actions must have somehow been wrapped up with the fbjs module such that ncc was including fbjs (or some subset thereof) in the compiled GitHub Actions output. But now that fbjs is included as its own separate dependency, ncc knows that it's not imported by our GitHub Actions. So all that code is no longer in the compiled ncc output.

👍 Idk if there's a better way to confirm that, but I'm not sure we need to.

Copy link
Contributor

@neil-marcellini neil-marcellini left a comment

Choose a reason for hiding this comment

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

I followed the test steps and got it running on all platforms. Thanks for doing the upgrade.

@neil-marcellini neil-marcellini merged commit 9c19841 into main Jan 11, 2022
@neil-marcellini neil-marcellini deleted the Rory-UpgradeRNCLI branch January 11, 2022 16:46
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @neil-marcellini in version: 1.1.27-2 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @neil-marcellini in version: 1.1.27-3 🚀

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

@kidroca
Copy link
Contributor

kidroca commented Jan 12, 2022

Regarding "@react-native-community/cli": "6.2.0" - we don't need it in our dependencies - it's a transitive dependency meaning it's listed in react-native's dependencies and is installed along react-native

IMO we only had cli by mistake or as a leftover - by default npx react-native init would not add it to package.json

We can remove it from package.json following this migration guide: https://github.com/react-native-community/cli#updating-the-cli

  1. find all the @react-native-community/cli prefixed entries in package-lock.json and remove them
  2. run npm install

Which it seems you already did, so we can just remove it from our package.json


Finally,

We do use @react-native-community/cli-tools for development purposes:

const {isPackagerRunning} = require('@react-native-community/cli-tools');

It should be listed in our dev-dependencies

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @neil-marcellini in version: 1.1.27-3 🚀

platform result
🤖 android 🤖 skipped 🚫
🖥 desktop 🖥 skipped 🚫
🍎 iOS 🍎 skipped 🚫
🕸 web 🕸 skipped 🚫

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.29-5 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants