-
Notifications
You must be signed in to change notification settings - Fork 156
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
feat: upgrade to RN 0.73.6 #1439
base: main
Are you sure you want to change the base?
feat: upgrade to RN 0.73.6 #1439
Conversation
bac8e18
to
bb4e828
Compare
Signed-off-by: Mostafa Gamal <[email protected]> Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
…penwallet-foundation#1433) Signed-off-by: fc-santos <[email protected]> Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
…ndation#1427) Signed-off-by: fc-santos <[email protected]> Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
…tion#1438) Signed-off-by: ClaudeArs <[email protected]> Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
Signed-off-by: Mostafa Gamal <[email protected]> Signed-off-by: Mostafa Gamal <[email protected]> Co-authored-by: Bryce McMath <[email protected]> Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
Signed-off-by: Bryce McMath <[email protected]> Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
8b9707e
to
67219c4
Compare
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
cb500bb
to
634439e
Compare
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
634439e
to
2c420df
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1439 +/- ##
==========================================
+ Coverage 55.52% 55.60% +0.07%
==========================================
Files 223 228 +5
Lines 7985 8176 +191
Branches 2241 2334 +93
==========================================
+ Hits 4434 4546 +112
- Misses 3528 3607 +79
Partials 23 23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Mostafa Gamal <[email protected]>
d7770d7
to
743c7dc
Compare
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
743c7dc
to
c28f2d9
Compare
…d-wallet into refactor/moscd3/upgrade-rn-73-5 Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
817e44d
to
1809cd8
Compare
PR Summary: React Native Upgrade from v0.72.17 to v0.73.6Based on the file structure you've shared, I can see this PR implements a significant upgrade of the Bifold Wallet from React Native v0.72.17 to v0.73.6. Here's a summary of the changes: Core Changes
Build & Configuration Updates
Testing Infrastructure
Developer Experience
# Add this to your .zshrc
autoload -U add-zsh-hook
load-nvmrc() {
local node_version="$(nvm version)"
local nvmrc_path="$(nvm_find_nvmrc)"
if [ -n "$nvmrc_path" ]; then
local nvmrc_node_version=$(nvm version "$(cat "${nvmrc_path}")")
if [ "$nvmrc_node_version" = "N/A" ]; then
nvm install
elif [ "$nvmrc_node_version" != "$node_version" ]; then
nvm use
fi
elif [ "$node_version" != "$(nvm version default)" ]; then
echo "Reverting to nvm default version"
nvm use default
fi
}
add-zsh-hook chpwd load-nvmrc
load-nvmrc For more information and support for other platforms, refer to https://github.com/nvm-sh/nvm#zsh This upgrade provides the benefits of React Native 0.73.6, including improved performance, bug fixes, and support for newer platform features, while maintaining the core functionality of the Bifold Wallet. |
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.
Some small comments. If it's easier I can make these changes if you invite me to collab on this branch
.github/workflows/main.yaml
Outdated
env: | ||
# Explicitly tell React Native where to find Node | ||
REACT_NATIVE_NODE_BINARY: /Users/runner/hostedtoolcache/node/18.18.2/arm64/bin/node |
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.
Is this still needed with the $(which node)
below? Using which
seems like the less brittle approach to me
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 think $(which node)
is sufficient and yes it is less brittle but i was trying to find out why the GHA was failing so i was ruling this out
@@ -1,12 +1,10 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
xmlns:tools="http://schemas.android.com/tools"> | |||
<uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/> |
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.
Just wondering, why was this removed?
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.
@bryce-mcmath you can read this facebook/react-native#5886 (comment) about
Removing unused permissions added at build time and this common unused permission you usually use it in debugging mode
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.
Ah nice, good catch 👍
packages/legacy/app/ios/Podfile
Outdated
# Enables Flipper. | ||
# | ||
# shall we Enable Flipper? |
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 don't think anyone uses Flipper on this project or that it was ever fully enabled. Safe to remove completely I think
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.
that is just a comment and it is removed
"ios": "react-native run-ios", | ||
"start": "react-native start", | ||
"ios": "react-native run-ios --scheme AriesBifold", | ||
"start": "react-native start --experimental-debugger", |
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.
If the debugging code will be added in a future PR, should we remove this flag?
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.
@bryce-mcmath i dont think we should as it is anyway working what i removed is the workaround for the remote debugging but the experimental is the default for hermes so no i think we shouldnt but if you want we can add a new script like start:debug
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.
Should be fine then 👍
// no need for the waitFor it might cause infinte loop and cause the test to fail | ||
// await waitFor(() => { | ||
// timeTravel(1000) | ||
// }) | ||
|
||
// use this instead since anyway u r waiting the promise to resolve to set setBiometryAvailable in the useEffect | ||
// this is cleaner approach than advanceTimersByTime |
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.
Agree, can remove this comment and use the findBy
approach
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.
done!
// await waitFor(() => { | ||
// timeTravel(1000) | ||
// }) | ||
// await act(async () => { | ||
// await new Promise((resolve) => setTimeout(resolve, 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.
Can remove this 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.
done!
|
||
expect(tree).toMatchSnapshot() | ||
}) | ||
|
||
test('Toggles use biometrics ok', async () => { | ||
authContext.isBiometricsActive = jest.fn().mockResolvedValueOnce(true) | ||
// authContext.isBiometricsActive = jest.fn().mockResolvedValueOnce(true) |
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 remove
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.
done!
// await waitFor(() => { | ||
// timeTravel(1000) | ||
// }) |
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 remove
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.
done!
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
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 was the reason for this change? Going from .m
to .mm
I mean
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.
@bryce-mcmath i mentioned this in the PR Summary #1439 (comment)
Changed AppDelegate from Objective-C to Objective-C++ for compatibility
please check the summary to know more
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.
Couple more small things related to the GHA that the summary comment didn't clear up for me, sorry to be a bother
- name: Create .xcode.env.local file | ||
working-directory: ./packages/legacy/app/ios | ||
run: | | ||
echo "export NODE_BINARY=$(which node)" > .xcode.env.local | ||
cat .xcode.env.local |
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.
Is this needed or was it just added for troubleshooting?
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.
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.
yes it is needed and cant be removed
.github/workflows/main.yaml
Outdated
env: | ||
REACT_NATIVE_NODE_BINARY: $(which node) | ||
NODE_BINARY: $(which node) |
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.
Are these needed here as well as on line 21? It should still work if they were both set up top only, no?
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.
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.
done and removed
.github/workflows/main.yaml
Outdated
env: | ||
REACT_NATIVE_NODE_BINARY: $(which node) | ||
NODE_BINARY: $(which node) |
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 comment as above
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.
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.
removed
@MaSMas0 @MosCD3 the only thing (other than the GHA stuff) that I think should be changed before this PR is good to go, is Might even be worth adding to the resolutions block as well |
@bryce-mcmath for the react-native-reanimated i dont see the issue and how it is related to carret as all minor versions of react-native-reanimated is supported by RN 0.73.6 as mentioned here in this link : https://docs.swmansion.com/react-native-reanimated/docs/guides/compatibility ![]() as for this https://github.com/software-mansion/react-native-reanimated/blob/bd4752b55935d3057d21d8141f98c2e6e2dae9c7/packages/react-native-reanimated/scripts/reanimated_utils.rb#L56-L57) ![]() P.S: react-native-reanimated isnt really used in the project at all maybe it is planned to be but right now it exist as additional dependency but has no real usage in the app so maybe it should be removed till you decide to use it instead of react native built in animation |
4c1b60f
to
619e0f5
Compare
@bryce-mcmath I think we should remove the re-animated package if its not directly used |
619e0f5
to
13ccfce
Compare
In semantic versioning, a package with version x.y.z means x is the major version, y is the minor, and z is the patch. A tilde (~) will allow packages to upgrade the patch version automatically, and a caret (^) will allow packages to upgrade the minor and patch versions automatically. In this example, it means
I'm fairly sure even if it is not used directly it is required to support other packages we use |
you are right i confused the patch with minor :D and tilde with caret ok no problem |
i am not sure that this is the case can you please check as from my side i dont see which package is depending on react-native-reanimated more clarification would be of a help as this is too big of package to be existing without a usage |
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
13ccfce
to
16ce4b9
Compare
It's a dependency and peer dependency of |
i think that this is not the case as you can see the peerDependenciesMeta showing that it is optional that the package wont break the app for example you can see @shopify/react-native-skia and you dont have it in project and yet the app works coz it is optional so i still think that you dont need it also if u check the docs of react-native-vision-camera it doesnt state that you need react-native-reanimated as peer dependencies in order for the package to work correctly so i suggest to remove it from the project as long as it is not needed
|
ohhh nice catch! ok let's remove it but if, after we remove it, it still gets installed as a transient dependency by another one of our deps, can we add an entry to the root
|
Signed-off-by: Mohamed Abd El-Samie Ahmad Mansour <[email protected]>
|
Yes, that's exactly the right approach. If react-native-reanimated is being installed as a transitive dependency from another package. we can surely use the resolutions field in root package.json to control its version but lets make this when we have that case i guess right now it is ok to move on with PR as we removed react-native-reanimated |
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.
Will approve again after conflicts are fixed but really great work! Thanks for your patience with all the staggered feedback
Summary of Changes
iOS part done so far
All changes re-reviewed and re-done by @MaSMas0
New version supports experimental and chrome debugger (RN Debugger)
Added menu to start chrome debugger
Replace this text with a high-level summary of the changes included in this PR.
Screenshots, videos, or gifs
Replace this text with embedded media for UI changes if they are included in this PR. If there are none, simply enter N/A
Breaking change guide
Replace this text with any breaking changes included in this PR along with how to address them in downstream projects. If there are none, simply enter N/A
Related Issues
Replace this text with issue #'s that are relevant to this PR. If there are none, simply enter N/A
Pull Request Checklist
Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.
Signed-off-by
line (we use the DCO GitHub app to enforce this)If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
Pro Tip 🤓