-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Upgrade @react-native-community/netinfo to latest. #75864
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
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
|
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Krishna2323 I believe we should remove the NoQA tag add test steps to check whether offline status is being detected properly and indicated since this module directly affects that functionality in the app. |
|
@akinwale I have updated the title, test steps and added testing videos. |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safari75864-web.mp4 |
akinwale
left a comment
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.
|
I have a few questions.
I don't see them mentioning supporting new-arc or the ones listed above in their releases.
Also, react-native-netinfo/react-native-netinfo#655 hasn't been merged yet. How the |
|
I am not sure how this PR was tested by both author and reviewer but before I submit updated proposal, I tested locally without patch and faced runtime error on iOS because new arch not supported. |
|
@mollfpr I'm really sorry for overlooking the response from AI web search. I should have dug deeper and verified the data thoroughly. The builds were successful and the app was working fine, which made my assumption stronger and pushed me to not verify the response properly. I know that was a really dumb mistake on my part, and I'm genuinely sorry about it. Thanks a lot for catching the issue — it would have created more trouble for me later. I truly feel guilty about this. I'm reverting the deletion and making the necessary changes in the patches now. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@mollfpr I’ve been sitting on this for the past 5-6 hours without a break and tried literally everything to make old patches work, but unfortunately I still haven’t been successful. Here’s what I tried today:
At this point, I’m not sure what else to try. If anyone can guide me on how to fix this, that would be super helpful. Or if someone familiar with handling patch failures or the NetInfo PR can take a look, they might solve it much more easily. Maybe there is an easy solution, but I’ve already tried everything I could think of. @war-in do you have any suggestions? I saw you reacted 👍 here.
|
|
Usually when I migrate RN patches, I go through each line of the patch and try to apply the changes if possible (if not I resolve conflicts). The turbomodule patch is a huge one so migrating it will be tough but to make sure we won't break anything, I'd recommend this approach. I found those two commits adding changes to the turbomodule patch where they should be extracted to a separate patch file as they're unrelated. Maybe we could extract them now? Also I'm not sure if the upstream PR has been updated so it might be outdated with the main branch of the repo but I can also see the possibly unrelated changes in the patch ( so I'd
|
|
@war-in thanks a lot for the suggestion. I also think the safest option is to go through each line of the patch and apply the changes carefully. It will definitely take a good amount of time, but I don’t think there’s another option. |
|
I’ll try to create a new patch over the weekend. The current patch is quite large and confusing, so it will require some time and effort. Thanks for waiting. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Update: New patch has been created and applied successfully, but the iOS app build is failing for some reason. I’m investigating the cause and will update here as soon as possible. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@akinwale the new patch file has been created, and the native app build was also successful. Could you please test this PR on your end as well? Thanks! |
|
@akinwale friendly bump! |
|
@Krishna2323 Please fix merge conflicts. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@akinwale all done. |
|
Re-tested and works well. Ready to merge. cc @mollfpr |
|
@Krishna2323 @akinwale Do we need to update the hybrid app too? |
|
@mollfpr the PR in the hybrid repo is up: https://github.com/Expensify/Mobile-Expensify/pull/13816 |
|
✋ 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/mollfpr in version: 9.2.93-0 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.93-1 🚀
|
…/75699 Upgrade @react-native-community/netinfo to latest.





Explanation of Change
What
@react-native-community/netinfoto^11.4.1.Fixed Issues
$ #75699
PROPOSAL: #75699 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4