-
-
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
feat: makes NavigationContainerRef.getCurrentRoute type safe #11525
base: main
Are you sure you want to change the base?
feat: makes NavigationContainerRef.getCurrentRoute type safe #11525
Conversation
Hey @lucasloisp! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #11525 +/- ##
=======================================
Coverage 75.96% 75.96%
=======================================
Files 207 207
Lines 5942 5942
Branches 2302 2302
=======================================
Hits 4514 4514
Misses 1378 1378
Partials 50 50 ☔ View full report in Codecov by Sentry. |
✅ Deploy Preview for react-navigation-example ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
: Route<Extract<RouteName, string>, ParamList[RouteName]>; | ||
}[keyof ParamList]; | ||
|
||
type MaybeParamListRoute<ParamList extends {}> = ParamList extends ParamListBase |
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.
Less than sure on the naming for this one. Need a second type to manage the case were ParamList
does not extend ParamListBase
, as the restriction on the ref is extends {}
.
Would need the name to signify that it might be a "well typed" Route
or it could be Route<string>
depending on what you pass.
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 you please check in tests if works for the nested routes?
So, I've been playing with that. It seems like const DemoStackToNest = createStackNavigator({
screens: {
Groups: () => null,
Chat: (_: StaticScreenProps<{id: number}>) => null,
}
})
const DemoStackWithNested = createStackNavigator({
screens: {
Home: DemoStackToNest
}
})
type DemoParamListOfNest = StaticParamList<typeof DemoStackToNest>
type DemoParamList = StaticParamList<typeof DemoStackWithNested>
declare const pl: DemoParamList
expectTypeOf(pl['Home']).toEqualTypeOf<NavigatorScreenParams<DemoParamListOfNest> | undefined>()
expectTypeOf(pl['Home']).toEqualTypeOf<NavigatorScreenParams<DemoParamListOfNest>>() // Fails TS, missing undefined
type CommonDemoParamListOfNest = {
Groups: undefined
Chat: { id: number }
}
type CommonDemoParamList = {
Home: NavigatorScreenParams<CommonDemoParamListOfNest>
}
declare const plc: CommonDemoParamList
expectTypeOf(plc['Home']).toEqualTypeOf<NavigatorScreenParams<DemoParamListOfNest> | undefined>() // Fails TS, extra undefined
expectTypeOf(plc['Home']).toEqualTypeOf<NavigatorScreenParams<DemoParamListOfNest>>() Working on that one and will then add tests for the new Route type |
Seems to stem from
This type, ultimately, seems to only be used for defining |
06bd61f
to
72eea65
Compare
Because you can navigate to a screen without needing to specify any child screens - which renders the initial route defined in the navigator. The types need to handle |
72eea65
to
9b53010
Compare
Got it! Missed that on I'll post updates if I can make progress! |
9b53010
to
232fd5b
Compare
Okay, I've figured this one out. I'm now doing a "two way extends" with Seems to cover all desired cases, in both common and static + the cases where |
Motivation
When doing analytics, it has been quite helpful to know the current screen and its params on the
onStateChange
callback directly, instead of deriving that fromstate
by running through nested routes.Even then, there are other cases that use
getCurrentRoute
and could use a more specific type thanRoute<string>
based on the already presentParamList
generic onNavigationContainerRef
.Currently, the typed returned is
Route<string>
, which provides no info on the valid "names" (as defined on theParamList
), nor on theparams
, whose type could be restricted by checking forname
.Test plan
All changes presented are typescript-only, validated with tests on
example/__typechecks__
for both the cases (static or otherwise) with a definedParamList
as well as an empty type{}
returningRoute<string>
which could be useful for projects adopting TS but that haven't yet typed their routes.