-
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 to React Native 0.76 #51475
Upgrade to React Native 0.76 #51475
Conversation
This comment has been minimized.
This comment has been minimized.
I tested it with the latest version in incognito mode, and it's not happening now |
@WoLewicki I noticed some differences in the pod lock file locally. Could you confirm if you forgot to push those changes, or if it's just a local difference on my end that we can ignore? |
If possible, could we also fix the ( |
@WoLewicki @mountiny I’ve tested on web, desktop, iOS (locally), and Android. So far, I haven’t found anything unusual during testing; everything seems perfectly fine to me. |
We talked about it already and since those changes are not coming from this PR, I would restrain from doing it.
I retested running |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS.moviOS: mWeb SafariMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
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 🚀
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.
Thanks for the hard work. Lots of testing was done, QA did full regression testing and C+ did a thorough testing through the app, but as it goes with prs like this, there will be some issues stemming from it. @WoLewicki @j-piasecki please make sure to be around tomorrow to help with any issues as we will most likely not revert this PR and we want to avoid holding deploy for a long time, thank you!
"expo-image": "1.12.15", | ||
"expo-image-manipulator": "12.0.5", | ||
"expo": "52.0.14", | ||
"expo-asset": "^11.0.1", |
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.
What do we need this one for?
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.
Expo depends on it. IIRC when bumping expo for RN 0.76, when I was building app without explicitly adding it here, it did not build for some reason. I can see that expo has dependency on it: https://github.com/expo/expo/blob/1a16bb66dcd8b0cc7391d9611d923fd54803af3c/packages/expo/package.json#L81 so I am not sure why it didn't work then.
We can try to remove it in follow-up and maybe it will be properly resolved from expo. It won't change much as for the contents of the app since it is required nevertheless.
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 lets keep an eye on this so we dont increase the bundle size unnecessary
"expo": "52.0.14", | ||
"expo-asset": "^11.0.1", | ||
"expo-av": "^15.0.1", | ||
"expo-font": "^13.0.1", |
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.
Same here
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.
Same as above
@@ -0,0 +1,17 @@ | |||
diff --git a/node_modules/expo-image-manipulator/src/index.ts b/node_modules/expo-image-manipulator/src/index.ts |
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.
Can you please make sure all the new patches added here are tracked? I see you have added them to the list in the PR, but just reminding here if its up to date?
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.
I will go through all of them when bumping to RN 0.77 ok?
The ESLint check is failing because of one file but we will migrate that in another PR to avoid scope creep |
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Not emergency as explained just above |
✋ 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/mountiny in version: 9.0.82-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.82-0 🚀
|
compileSdkVersion = 34 | ||
buildToolsVersion = "35.0.0" | ||
minSdkVersion = 24 | ||
compileSdkVersion = 35 | ||
targetSdkVersion = 34 |
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.
@WoLewicki Shouldn't targetSdkVersion
be 35 as well?
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.
Coming from https://react-native-community.github.io/upgrade-helper/?from=0.75.2&to=0.76.3 it seems like it should be left with 34. Do you have a reason for bumping it though?
Hi guys, this PR is breaking the attachments in the composer (GH), any insight of what could be the solution? |
@marcochavezf lets try to fix it and CP a fix, lets talk in Slack |
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.82-12 🚀
|
Hey, we think this upgrade caused a keyboard bug. We don't have much info here but probably due to some internal changes. |
Hello This upgrade broke the textinput label on iOS. More details #54971 |
Details
This PR upgrades the version of React Native used in the App to 0.76.
Upstream PRs:
onLayout
facebook/react-native#47902CREATE
mutations to be effectively dropped facebook/react-native#47960entries
inFormData
expo/expo#33445export type
instead ofexport
inexpo-image-manipulator
expo/expo#33446import-type
instead ofimport
inexpo-modules-core
expo/expo#33447Notes:
react-native-nitro-sqlite
Fixed Issues
$ #48911
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop