-
Notifications
You must be signed in to change notification settings - Fork 3k
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 reanimated to 3.1.0 #18073
Upgrade reanimated to 3.1.0 #18073
Conversation
Unfortunately, upgrading Reanimated to 3.1.0 results into a crash when navigating to the chat. This seems to be related to this issue: software-mansion/react-native-reanimated#2285 Looking into the cause, maybe it reproduces and fix this crash /cc @tomekzaw 2023-05-02.12.07.29.movCrash log2023-05-02 11:59:26.085353+0300 New Expensify[89330:15255769] [db] Failed to initialize client context with error Error Domain=NSOSStatusErrorDomain Code=-10817 "(null)" UserInfo={_LSFunction=_LSSchemaConfigureForStore, ExpectedSimulatorHash={length = 32, bytes = 0xed662242 5fb7c0a8 1bf3f783 33b5c275 ... 5cef5e61 5d97a96c }, _LSLine=478, WrongSimulatorHash={length = 32, bytes = 0x3190f8e7 217931fc b7f7be3d 298bbb9a ... 8472784b 30a84b1d }} |
Found the cause of the crash: Commenting out this line resolves the crash. |
After testing can confirm this PR fixes our iOS crash: |
a8f6a30
to
a3485a7
Compare
In the meantime we used patch-package to fix the crash on iOS. |
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.
Looks good to me! Tagging @roryabraham for another set of eyes on this.
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
It's used on web as well 🙂 |
Also, the tests failing on this branch are weird. I think it's to do with |
Yeah, definitely not a standard thing: https://caniuse.com/?search=setImmediate |
very interesting, worth submitting an issue in their repo, I'll do it on Monday, setTimeout should cover it though. |
You can fix some of the failing tests by:
But still |
Yeah I'll look into it for sure. I tested everything locally and it worked well so falling tests are a surprise. |
Yes, adding We can of course just add another patch for this. But But also, legacy API should not work in the Drawer anyway, since it's a long time deprecated and should be removed. |
package.json
Outdated
@@ -131,6 +131,7 @@ | |||
"react-web-config": "^1.0.0", | |||
"save": "^2.4.0", | |||
"semver": "^7.3.8", | |||
"setimmediate": "^1.0.5", |
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.
Oh, I think this should be a dev dependency since it's only needed by jest?
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.
yeah makes sense, hope no one's gonna use it in the app though
package.json
Outdated
"react-native-picker-select": "git+https://github.com/Expensify/react-native-picker-select.git#107b3786ae6bc155dec05c7fc5ee525d3421dc21", | ||
"react-native-plaid-link-sdk": "^10.0.0", | ||
"react-native-quick-sqlite": "^8.0.0-beta.2", | ||
"react-native-reanimated": "3.1.0", |
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.
prettier screwed the diff
new reanimated version here
package.json
Outdated
"react-native-performance-flipper-reporter": "^2.0.0", | ||
"react-native-svg-transformer": "^1.0.0", | ||
"react-test-renderer": "18.1.0", | ||
"setimmediate": "^1.0.5", |
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.
and setimmediate as a dev dependency since it used to be part of reanimated and is still used in tests right now
Sorry @terrysahaidak, you've got conflicts again |
@roryabraham it looks like some of the branches still have non-formatted |
Used the same 2-space indentation as the code on main so it's not causing more problems for now, but it might need lint-staged to check for prettier formatting on merge at least |
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.
@terrysahaidak I'm getting a prettier diff on this branch
@roryabraham looks to be ready to go |
Sorry for the delay, reviewing now |
Running ad-hoc test build: https://github.com/Expensify/App/actions/runs/5017429511 |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSRPReplay_Final1684452532.MP4Androidscreen-20230518-163346.mp4 |
@MariaHCD I'm going to finish your checklist to make the PR checks pass |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.17-0 🚀
|
@roryabraham @MariaHCD Do we need to QA anything here? |
@mvtglobally I think it would be good to QA the following on iOS and Android (specifically the animation):
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.17-5 🚀
|
Details
Reanimated upgrade is required to unblock #16356
In this PR I also fixed some of the bugs in the code, but also used
patch-package
for iOS to address the crash we had, it was resolved in a recent PR software-mansion/react-native-reanimated#4403 and touches only iOS files so it's safe to use patch-package.We decided to use patch-package here.
Reanimated is used on mobile (iOS and Android)
Fixed Issues
$ GH_LINK
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
I tested all components that use Reanimated on both iOS and Android:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
No changes
Mobile Web - Chrome
No changes
Mobile Web - Safari
No changes
Desktop
No changes
iOS
No changes
Android
No changes