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 to 0.66 #6914

Merged
merged 11 commits into from
Jan 6, 2022
Merged

[No QA] Upgrade React Native to 0.66 #6914

merged 11 commits into from
Jan 6, 2022

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Dec 28, 2021

Details

This pull request upgrades the following packages:

And adds the react-native-clean-project as a dev dependency, along with a useful clean script. More details on all that does here. Note that we're using an alpha version because it fixed this issue for me.

Note: The React Native Upgrade Helper was used to guide some of these changes.

Note: We should follow-up with an issue to upgrade react-native-modal because it's throwing errors due to the deprecated removeEventListener function. This has been fixed in their latest version.

Note: We should also follow-up with an issue to upgrade urbanairship-react-native, because it's throwing warnings like new NativeEventEmitter() was called with a non-null argument without the required removeListeners method

Fixed Issues

Coming from #6898 (comment) and #6874 (comment)

Tests

  1. (optional) Run npm run clean to clean the project
  2. Install node modules and pods: npm i && cd ios && pod install && cd ..
  3. Run the app
  4. Run various basic regression tests to verify that the app is working as expected.

QA Steps

None, just regression testing.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

Desktop

image
image

iOS

Android

@parasharrajat
Copy link
Member

This is the correct time to upgrade the RN-web as well. We will fix a couple of issues along with this.

@roryabraham
Copy link
Contributor Author

I was thinking we would upgrade RN first, then RN-web in a separate PR. Do you think we should do both in a single PR?

@roryabraham
Copy link
Contributor Author

It actually looks like we're pointing at our fork, which is several commits ahead of the upstream react-native-web. So I'm not sure there's anything to upgrade there.

@parasharrajat
Copy link
Member

Ok. Sounds great. Even if we had to. It should definitely be a separate PR.

@roryabraham
Copy link
Contributor Author

roryabraham commented Dec 30, 2021

I got Android working, but I'm seeing the following warning:

So we'll probably need to follow-up and update UrbanAirship. Going to want to do this in another separate PR.

@roryabraham
Copy link
Contributor Author

roryabraham commented Dec 30, 2021

I'm getting an infinite spinner trying to open the Plaid modal on Android. I wonder if I broke it or if I'm just holding something wrong for testing Plaid on dev. Would be best to get this working on dev, but I did double-check to verify that the Plaid bank account setup is covered by regression tests: https://expensify.slack.com/archives/C9YU7BX5M/p1640874804291700

@parasharrajat
Copy link
Member

parasharrajat commented Dec 30, 2021

Yeah, and also refactor our app to use .remove instead of removeEventListners

@roryabraham
Copy link
Contributor Author

Yeah, and also refactor our app to use .remove instead of removeEventListners

I think this PR does that. Some of our dependencies will need to be upgraded for all these warnings to go away.

@roryabraham
Copy link
Contributor Author

Ahh, got Plaid working on Android. I needed a second ngrok tunnel for the web-secure API.

@roryabraham roryabraham marked this pull request as ready for review January 4, 2022 16:57
@roryabraham roryabraham requested a review from a team as a code owner January 4, 2022 16:57
@roryabraham
Copy link
Contributor Author

Taking this out of draft

@MelvinBot MelvinBot requested review from alex-mechler and removed request for a team January 4, 2022 16:58
Copy link
Contributor

@alex-mechler alex-mechler left a comment

Choose a reason for hiding this comment

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

Changes look good to me, but I'm not super familiar with the gradle changes. Do you know someone else who is and can double check them?

@roryabraham
Copy link
Contributor Author

Tagging in @Julesssss for.a second review.

alex-mechler
alex-mechler previously approved these changes Jan 4, 2022
Copy link
Contributor

@alex-mechler alex-mechler left a comment

Choose a reason for hiding this comment

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

All you Jules

@roryabraham
Copy link
Contributor Author

Regarding the Android build failure, @Julesssss did you try running npm run clean?

@roryabraham
Copy link
Contributor Author

Or at minimum, rm -rf node_modules && npm i (because it looks like the error is coming from a node module which may have been cached from an out-of-date version

@Julesssss
Copy link
Contributor

I tried npm run clean, but didn't completely clear node_modules. Will give that a try tomorrow.

Copy link
Contributor

@alex-mechler alex-mechler left a comment

Choose a reason for hiding this comment

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

Haven't been able to get android / ios to build, but I usually have to struggle to get those to build on my machine. @Julesssss feel free to merge if you can get both of those building for you

@Julesssss
Copy link
Contributor

Or at minimum, rm -rf node_modules && npm i (because it looks like the error is coming from a node module which may have been cached from an out-of-date version

Wiping node_modules resolved the Android terminal issue. I struggled to build iOS from the terminal, but I'm pretty sure it's an issue with my local env as I was able to build from XCode. Merging!

@roryabraham roryabraham merged commit 021ad5d into main Jan 6, 2022
@roryabraham roryabraham deleted the Rory-UpgradeRN branch January 6, 2022 14:54
@Julesssss
Copy link
Contributor

Actually, while I can build to iOS, I have been unable to fully verify iOS.

When building to a physical device I get a connection error, and with simulator, I get an immidiate dyld crash (seems to be related to the RCTTextHeaders library).

@roryabraham
Copy link
Contributor Author

@Julesssss since you said "Merging!" I assumed you had just forgotten to merge this 😬

@roryabraham
Copy link
Contributor Author

Bad timing on my part...

@OSBotify
Copy link
Contributor

OSBotify commented Jan 6, 2022

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

@Julesssss
Copy link
Contributor

Julesssss commented Jan 6, 2022

Yeah... my fault. I started debugging and hadn't yet sent the above message

@Julesssss
Copy link
Contributor

Have you been able to verify iOS? If so I'm less worried as it seems to be issues on my end only.

@roryabraham
Copy link
Contributor Author

roryabraham commented Jan 6, 2022

Well it's not deployed yet. I had no issues building to a simulator, logging in, etc..., but I admittedly did not test with a physical device. Let me try that next

@Julesssss
Copy link
Contributor

Julesssss commented Jan 6, 2022

If you can log in with a physical device and simulator I think we're fine. I don't want to spend too much longer debugging this but will spend another 10 mins or so

@Julesssss
Copy link
Contributor

@roryabraham okay, I finally got it to work on an iOS simulator. Not exactly sure why, but after retargeting iOS 12 in XCode and a bunch of other things it works. It must have been something on my side.

@roryabraham
Copy link
Contributor Author

Thanks for working through it @Julesssss. I'm still going to try and run the app on a physical device

@roryabraham
Copy link
Contributor Author

Yeah, everything worked on a physical device too. As stated in Slack, I've had to build from XCode once and then everything starts working normally from the CLI

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.1.26-2 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.1.27-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.27-1 🚀

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
5 participants