Improve Flow typing of 'navigation' props, rework how params are extracted #3437
Conversation
…ams' This change preserves the behavior and is almost a pure refactor. Using `getParam` is the preferred way of accessing the navigation's state parameters and has some advantages, as documented here: https://reactnavigation.org/docs/en/params.html `navigation.state.params` can be `null` if no parameters are passed. While we do not do that in our code, the types, nor our code is acknowledging that possibility. `getParam` will always execute and we can optionally provide a default value when none exists. Sometimes, not giving a default value is good enough. Prime example of how this approach simplifies our code (if written correctly) is the `RealmScreen.js` file: * we don't actually need to verify if `navigation` is defined, it always is, when the component is used as a navigation screen * we don't need to check if `navigation.state.param` is defined, as `getParam` will do that for us * `getParam` also provides us the default value
I had some doubts about this change before doing some research and
experimentation with Flow typing our previous code. It looks like
we will lose some Flow typing by using this approach. Not really.
First things first. Our Flow types were completely bogus.
They look correct but they are not helping. They look like this:
```
navigation: NavigationScreenProp<*> & {
state: {
params: {
someValue: SomeType,
},
},
},
```
If one renames the `someValue` name in code but not in this type,
they will quickly realize Flow does not catch that. So we are just
pretentind to be typing the parameters.
Use the more precise typing for `navigation` which is:
`navigation: NavigationScreenProp<NavigationStateRoute>`
This would actually catch errors if we did have any, as now the
value is properly typed.
We do not lose any typing from the:
```
state: {
params: {
narrow: Narrow,
},
},
```
As the `<*>` would allow anything as alternative.
I experimented with typing that did not work, similar to:
```
export type NavigationParams<T> = NavigationScreenProp<{
...NavigationStateRoute,
params: T,
}>;
```
|
@gnprice ready for review/discussion/merge. |
|
Thanks @borisyankov ! These navigation types have certainly bugged me for a while. I don't love the But the fact that Given that we don't currently have working type-checking on these, I could see making the change, knowing that we'd end up changing back if we later see an opportunity to get working type-checking. But I'd want to better understand what we're gaining from the change. Re What we do in most components is just directly get properties off of independent of this change. |
|
BTW as it happens I did some work on these types last week, as part of my cleaning up of most of the remaining If it'll help with #3422, I can go back in the next day or two and finish and merge those changes. What was the issue you were seeing? |
|
As promised above, a couple of weeks ago I went and made a bunch of cleanups and fixes to the navigation types; see 05bfc6c^..7293860. I think that resolves what this PR was aiming for, and without sacrificing any type-checking (as |
Prerequisite for #3422
More details in each commit.
navigation.getParam()instead ofnavigation.state.paramsnavigation.state.paramswithgetParamnavigationproperties