Skip to content
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

[Wallet] Upgrade react-navigation libraries #5896

Merged
merged 22 commits into from
Nov 19, 2020

Conversation

etuleu
Copy link
Contributor

@etuleu etuleu commented Nov 17, 2020

Description

This seems to fix the top crash we are currently experiencing.
Thanks @cmcewen for smelling the right fix here. We should also start a PR for upgrading all the other react packages, but I wanted to do this one as a smaller PR to make sure it fixes it and doesn't introduce anything new.

TODO: fix the any types that are added by asking @jeanregisser

Other changes

Added a new e2e test for opening the app using a dappkit url, which is the common source of crashes.

Tested

Manually by opening the app with a deep link. Committed the script used also.

Related issues

Backwards compatibility

Yes.

@@ -112,6 +112,7 @@ export const modalScreenOptions = ({ route, navigation }: NavigationOptions) =>
gestureEnabled: true,
cardOverlayEnabled: true,
headerStatusBarHeight:
// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIXME

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaving this with the ts-ignore for now because I think the library fixed it, but we need to test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var route: Route<string, unknown> Argument of type 'Route<string, unknown>' is not assignable to parameter of type 'NavigationRoute<StackParamList, Screens.AccountKeyEducation | Screens.BackupComplete | Screens.BackupIntroduction | Screens.BackupPhrase | Screens.BackupQuiz | Screens.DappKitAccountAuth | Screens.DappKitSignTxScreen | Screens.DappKitTxDataScreen | ... 57 more ... | Screens.WithdrawCeloScreen>'. Type 'Route<string, unknown>' is not assignable to type 'Readonly<{ key: string; name: Screens.AccountKeyEducation | Screens.BackupComplete | Screens.BackupIntroduction | Screens.BackupPhrase | Screens.BackupQuiz | Screens.DappKitAccountAuth | Screens.DappKitSignTxScreen | Screens.DappKitTxDataScreen | ... 57 more ... | Screens.WithdrawCeloScreen; }>'. Types of property 'name' are incompatible. Type 'string' is not assignable to type 'Screens.AccountKeyEducation | Screens.BackupComplete | Screens.BackupIntroduction | Screens.BackupPhrase | Screens.BackupQuiz | Screens.DappKitAccountAuth | Screens.DappKitSignTxScreen | Screens.DappKitTxDataScreen | Screens.Debug | ... 56 more ... | Screens.WithdrawCeloScreen'.ts(2345)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I can have a look tomorrow morning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome! thank you!

@etuleu etuleu added the wallet label Nov 17, 2020
@etuleu etuleu added this to the Milestone 7 milestone Nov 17, 2020
Copy link
Contributor

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great!! 👍

yarn.lock Outdated
Comment on lines 28168 to 28194
react-native-iphone-x-helper@^1.0.3, react-native-iphone-x-helper@^1.2.1:
react-native-iphone-x-helper@^1.0.3:
version "1.2.1"
resolved "https://registry.yarnpkg.com/react-native-iphone-x-helper/-/react-native-iphone-x-helper-1.2.1.tgz#645e2ffbbb49e80844bb4cbbe34a126fda1e6772"
integrity sha512-/VbpIEp8tSNNHIvstuA3Swx610whci1Zpc9mqNkqn14DkMbw+ORviln2u0XyHG1kPvvwTNGZY6QpeFwxYaSdbQ==

react-native-iphone-x-helper@^1.3.0:
version "1.3.1"
resolved "https://registry.yarnpkg.com/react-native-iphone-x-helper/-/react-native-iphone-x-helper-1.3.1.tgz#20c603e9a0e765fd6f97396638bdeb0e5a60b010"
integrity sha512-HOf0jzRnq2/aFUcdCJ9w9JGzN3gdEg0zFE4FyYlp4jtidqU03D5X7ZegGKfT1EWteR0gPBGp9ye5T5FvSWi9Yg==

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason yarn duplicates these, but if you manually remove both and re-run yarn it will combine them again since they are compatible versions.

yarn.lock Outdated
Comment on lines 27787 to 27795
query-string@^6.13.6:
version "6.13.7"
resolved "https://registry.yarnpkg.com/query-string/-/query-string-6.13.7.tgz#af53802ff6ed56f3345f92d40a056f93681026ee"
integrity sha512-CsGs8ZYb39zu0WLkeOhe0NMePqgYdAuCqxOYKDR5LVCytDZYMGx3Bb+xypvQvPHVPijRXB0HZNFllCzHRe4gEA==
dependencies:
decode-uri-component "^0.2.0"
split-on-first "^1.0.0"
strict-uri-encode "^2.0.0"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same yarn issue about duplicating query-string though versions are compatible.

@@ -112,6 +112,7 @@ export const modalScreenOptions = ({ route, navigation }: NavigationOptions) =>
gestureEnabled: true,
cardOverlayEnabled: true,
headerStatusBarHeight:
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the error?

@jeanregisser jeanregisser added the automerge Have PR merge automatically when checks pass label Nov 19, 2020
@mergify mergify bot merged commit dbac500 into master Nov 19, 2020
@mergify mergify bot deleted the etuleu/upgrade-navigation-fix-crash branch November 19, 2020 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass wallet
Projects
None yet
2 participants