-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
fix: wrong setParams type if route does not have params #10512
fix: wrong setParams type if route does not have params #10512
Conversation
If a RouteList type has a route with params set to undefined, for example: ```ts type ExampleRouteList = { HasArgs: {someArg?: string}; NoArgs: undefined; }; ``` Then `navigation.setParams` for `NoArgs` would be typed as `(params: Partial<unknown>): void`, since `NavigationProp` would type it as `(params: Partial<RouteList[RouteName]>): void`. This would cause issues with `CompositeScreenProps` because a route with `undefined` params would not extend the `NavigationProp` type correctly, so that screen would not actually get all the options the parent navigators have available. Conditionally setting `setParams` to `undefined` if `RouteList[RouteName]` extends `undefined` fixes this issue.
Hey Gustash! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines. |
✅ Deploy Preview for react-navigation-example ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@satya164 can I get a review here? Shouldn't take too long |
Waiting on this one. |
Codecov Report
@@ Coverage Diff @@
## main #10512 +/- ##
=======================================
Coverage 73.87% 73.87%
=======================================
Files 160 160
Lines 4992 4992
Branches 1949 1949
=======================================
Hits 3688 3688
Misses 1267 1267
Partials 37 37 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it 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.
Thanks for the PR
Motivation
Fixes #10471
If a RouteList type has a route with params set to undefined, for example:
Then
navigation.setParams
forNoArgs
would be typed as(params: Partial<unknown>): void
, sinceNavigationProp
would type it as(params: Partial<RouteList[RouteName]>): void
.This would cause issues with
CompositeScreenProps
because a route withundefined
params would not extend theNavigationProp
type correctly, so that screen would not actually get all the options the parent navigators have available.Conditionally setting
setParams
toundefined
ifRouteList[RouteName]
extendsundefined
fixes this issue.Test plan
Adding a test case in
example/types.check.tsx
forLatestScreen
, which has params set toundefined
, showed that this was indeed an issue becausesetOptions
would fail the checks.After the fix, these checks now pass for all screens, including the
LatestScreen
.I also fixed the
setOptions
type checks so that they explicitly check that all the parent navigator screen options are available, instead of just a hand-full.Code formatting
yarn test
,yarn lint
andyarn typescript
all pass with these changes, and I have ran them locally before commiting.