nav types: Add and demo RouteProp, to keep route-param types next to props #4430
Conversation
We don't have many of these at present, and should probably have more. We'll add some more in the next commit. Before that, add a bit of developer documentation about what these are and how they work.
…props. The type of `props.route.params` is very much like the type of a React component's `props` itself: it's part of the definition of the component's interface, and the most natural place for it to live is next to the definition of the component. With React Navigation upstream's guide for using type-checking: https://reactnavigation.org/docs/typescript/ (of which I'm looking at the 5.x version; sadly those docs don't allow permalinks to the current version, only to versions which are already outdated), this unfortunately gets flipped around, so that the route-params types are all defined in a central place associated with the navigator, and the individual components' definitions refer to that to get their respective types. That works in that it does at least get type-checked, but it makes it harder than it should be to read and understand the components, either to use them or to make changes to them. The effect is a lot like if for an ordinary function, the types of its parameters weren't defined up at the top but instead off in some other file. So, let's do things the other way around: the route-params type gets defined right there among the other props, next to the component's body; and the navigator's central list of params types refers to the components' types to get their respective route-params types. In this commit, we add a couple of small type aliases to use for that arrangement, and demonstrate using them on one of our smaller navigators. There are also some type-tests to check that these aliases behave as expected.
There was a problem hiding this comment.
This looks great, thanks! I'm happy to take this as an example and do it for everything else, if you want—unless you'd like to do so.
I did get a lint error:
Running lint...
/Users/chrisbobbe/dev/zulip-mobile/src/__flow-tests__/nav-test.js
0:0 warning File ignored because of a matching ignore pattern. Use "--no-ignore" to override
✖ 1 problem (0 errors, 1 warning)
ESLint found too many warnings (maximum: 0).
Looking at the .eslintignore, I see a **/__flow-tests__/** line. Removing it and rerunning tools/test lint shows a few lint errors and warnings.
|
|
||
| const Stack = createStackNavigator<NavParamList, NavParamList, NavigationProp<>>(); | ||
|
|
||
| <Stack.Navigator> |
There was a problem hiding this comment.
JSX just hanging out here, with no return and no render—totally valid, and I wouldn't change it at all, but I did a double-take! 😄
|
Thanks for the review!
That sounds great -- please do. (I'll merge this in just a moment, after adding a tweak for the next thing.)
Ah. Yeah, that line is intentional -- basically any file there is likely to have lots of The fact that it fails So it's complaining because it was specifically asked to look at that particular file, and the file is ignored. That's a reasonable behavior for ESLint; it could be helpful in an actual interactive command-line context, even though from a script like this it's just an annoying complication. Ideally we'd persuade |
Also discussing a bug in the interaction of `tools/test` with `eslint` that appears when you're on a branch that's touched one of the ignored files.
|
And merged, with an extra commit on top adding comments to |
This is a followup to #4393, which I discussed doing here, while that was in draft state, and was later discussed again here after it landed.
The type of
props.route.paramsis very much like the type of a React component'spropsitself: it's part of the definition of the component's interface, and the most natural place for it to live is next to the definition of the component.With React Navigation upstream's guide for using type-checking:
https://reactnavigation.org/docs/typescript/
(of which I'm looking at the 5.x version; sadly those docs don't allow permalinks to the current version, only to versions which are already outdated), this unfortunately gets flipped around, so that the route-params types are all defined in a central place associated with the navigator, and the individual components' definitions refer to that to get their respective types.
That works in that it does at least get type-checked, but it makes it harder than it should be to read and understand the components, either to use them or to make changes to them. The effect is a lot like if for an ordinary function, the types of its parameters weren't defined up at the top but instead off in some other file.
So, let's do things the other way around: the route-params type gets defined right there among the other props, next to the component's body; and the navigator's central list of params types refers to the components' types to get their respective route-params types.
In this PR, we add a couple of small type aliases to use for that arrangement, and demonstrate using them on one of our smaller navigators. There are also some type-tests to check that these aliases behave as expected.