-
Notifications
You must be signed in to change notification settings - Fork 364
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
Upgrade react-native to 0.68.2 #435
Conversation
Unfortunately this PR does not comply with the Contributing Conventions and will be closed automatically.Feel free to reopen this PR once you have browsed through the guidelines. Found Issues:
|
🦋 Changeset detectedLatest commit: 8319ff4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Screenshots: ✅
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
Codecov Report
@@ Coverage Diff @@
## develop #435 +/- ##
===========================================
- Coverage 47.69% 47.68% -0.01%
===========================================
Files 613 613
Lines 27277 27277
Branches 6827 6827
===========================================
- Hits 13009 13008 -1
- Misses 14208 14209 +1
Partials 60 60
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -14,4 +14,15 @@ | |||
<natures> | |||
<nature>org.eclipse.buildship.core.gradleprojectnature</nature> | |||
</natures> | |||
<filteredResources> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I don't know what this is and why it changed 😬
return new ReactActivityDelegateWrapper(this, new ReactActivityDelegate(this, getMainComponentName()) { | ||
@Override | ||
protected ReactRootView createRootView() { | ||
return new RNGestureHandlerEnabledRootView(MainActivity.this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See following section in PR description:
- Upgraded
react-native-gesture-handler
to2.5.0
& Migrating off RNGHEnabledRootView as its setup on Android (inMainActivity.java
) might conflict with react-native stuff later on
@@ -1,7 +1,7 @@ | |||
#!/usr/bin/env sh | |||
#!/bin/sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file taken from upgrade tool
@@ -121,7 +121,7 @@ - (void)applicationDidEnterBackground:(UIApplication *)application { | |||
- (NSURL *)sourceURLForBridge:(RCTBridge *)bridge | |||
{ | |||
#if DEBUG | |||
return [[RCTBundleURLProvider sharedSettings] jsBundleURLForBundleRoot:@"index" fallbackResource:nil]; | |||
return [[RCTBundleURLProvider sharedSettings] jsBundleURLForBundleRoot:@"index"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix necessary to build on RN 0.68.2, see facebook/react-native#33451 (comment)
@@ -183,7 +183,7 @@ function App({ importDataString }: AppProps) { | |||
}); | |||
|
|||
return ( | |||
<View style={styles.root}> | |||
<GestureHandlerRootView style={styles.root}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See following section of PR description
- Upgraded
react-native-gesture-handler
to2.5.0
& Migrating off RNGHEnabledRootView as its setup on Android (inMainActivity.java
) might conflict with react-native stuff later on
@@ -96,38 +96,13 @@ function PortfolioHeader({ | |||
}; | |||
}, [graphCardEndPosition]); | |||
|
|||
const ContainerStyle = useAnimatedStyle(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See following section of PR description:
- Fixed an issue in the portfolio where if there was no assets, scrolling was crashing the app on iOS. This is a mysterious issue and the logs are similar to this issue [iOS] ScrollView event fatal crash software-mansion/react-native-reanimated#2285, for now it has been solved by removing the animation of a border width (border which anyway was invisible so the animation was pointless).
bf0c4c0
to
8489d46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested on a real android device and the ios simulator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍 👌
3dcd365
to
59fce81
Compare
59fce81
to
ebeed76
Compare
ebeed76
to
9358832
Compare
9358832
to
b1cb93d
Compare
b1cb93d
to
79250a5
Compare
79250a5
to
ea9fdcf
Compare
deb8ba0
to
e2c6267
Compare
very impressive work @ofreyssinet-ledger , i hope you manage to fix all remaining topics 🤞 |
@gre haha thanks! It's painful work but I think I'm finally done 🤞 at least with what QA detected in their last test:
|
ae447f9
to
bc6a348
Compare
use JDK 11 in build-mobile tmp fix for flow error (#663) flow Fix anim issue in RatingsModal Fix post rebase
bc6a348
to
8319ff4
Compare
Disclaimer
Yes I know that RN 0.69.0 was released last week 😬 but I was already working on this and it's a 1st step forward that is needed to unblock us on some topics (see below).
📝 Description
I'm doing this because we would like to be ready for
[email protected]
(which requires[email protected]
) when it releases (right now it's in alpha) as it will solve a dependency conflict withstoryly-react-native
which we would like to use.Main changes:
react-native
to0.68.2
, following this guide and picked what works for us:react-native-gradle-plugin
as it's only necessary for the new architecture..AppDelegate.m
to the newAppDelegate.mm
as it's only useful for the new RN arch which we aren't using yet + it was a pain to migrate the existing config (Firebase, Flipper, splash screen)react-native-reanimated
to2.8.0
lottie-react-native
to5.1.3
as it was not building on iOS without upgrading -> I tested the device lotties in the "Debug Lottie" menu and it seems to work fine.react-native-gesture-handler
to2.5.0
& Migrating off RNGHEnabledRootView as its setup on Android (inMainActivity.java
) might conflict with react-native stuff later onWhat I tested:
I didn't test much of the app after fixing the obvious crashes & issues, just tested the very basic functionality of the app on real device for both platforms + things known to be susceptible to break while upgrading react-native (following list). A full round of non-regression testing will have to be done on both platforms before merging.
Android (tested on OnePlus 8, staging build from https://github.com/LedgerHQ/ledger-live/actions/runs/2591069426)
pnpm mobile test-deep-links
)iOS (tested on iPhone X, staging build from: https://github.com/LedgerHQ/ledger-live-build/actions/runs/2623892281)
pnpm mobile test-deep-links
)TODO
react-native-gesture-handler
and Migrating off RNGHEnabledRootView as its setup on Android (inMainActivity.java
) might conflict with react-native stuff later on.Issues detected but not a regression (already happening on prev versions of RN):
[iOS] ScrollView event fatal crash software-mansion/react-native-reanimated#2285
[interpolate] Exception NSException * "Mouting block is expected to not be set" software-mansion/react-native-reanimated#2764
-> it makes the scroll a bit jittery so not ideal
ScrollContainerHeader
)❓ Context
ledger-live-mobile
✅ Checklist
A full round of non-regression testing will have to be done on both platforms before merging.
📸 Demo
🚀 Expectations to reach
Please make sure you follow these Important Steps.
Pull Requests must pass the CI and be internally validated in order to be merged.