-
-
Notifications
You must be signed in to change notification settings - Fork 691
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(react-router): allow narrowing of matches with useMatches
#2058
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 18faefc. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
match: TMatch, | ||
path: ValidateMatchSuggestions<TMatch, TPath>, | ||
): match is SuggestMatchesByPath<TPath, TMatch>['match'] => { | ||
const parts = path.split('.') |
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.
This runtime logic is really basic. I'm going to be adding tests so I assume it might get better if we like the idea
TValue = TMatch, | ||
TParentPath extends string = '', | ||
> = TPath extends `${string}.${string}` | ||
? TPath extends `${infer TFirst}.${infer TRest}` |
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.
Optimised the types a bit but I have some ideas to go further if we like it
@@ -1140,6 +1140,7 @@ export class Router< | |||
scripts: route.options.scripts?.(), | |||
staticData: route.options.staticData || {}, | |||
loadPromise: createControlledPromise(), | |||
fullPath: route.fullPath, |
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.
I think it would be good to have fullPath
on a match to enable <Link from={match.fullPath} />
const matchesWithCrumbs = matches.filter((match) => | ||
isMatch(match, 'loaderData.crumb'), | ||
) |
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.
looking at the sandbox, matchesWithCrumbs
is always RouteMatch<any, any, any, any, any, any, any, any>[]
:
Is that because the sandbox is using the wrong TS version?
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.
also - why couldn't we just check for match.loaderData?.crumb !== undefined
?
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.
I noticed this. It's weird. Works well in vscode and sometimes it works in the sandbox but not all the time
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.
also - why couldn't we just check for
match.loaderData?.crumb !== undefined
?
The issue is TS is not able to narrow this. If you have loader data in matches which do not have crumb
, you can't get to the property without using in
. Furthermore TS is not able to narrow the parent type of a property if it's not a literal type. This is why routeId
works but crumb
does not. Discriminated unions work but other properties do not
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.
I noticed this. It's weird. Works well in vscode and sometimes it works in the sandbox but not all the time
Maybe the types just need optimising more. I think there's still too much instantiations happening in the recursive type due to it distributing over the recursive type. Maybe stackblitz is more limited in resources or uses an older version of TS which kicks the bucket with more complex types?
useMatches returns a union of all matches
Currently
useMatches
returns single match type with different properties which are a union of everything.loaderData
is a union of allloaderData
,routeId
is a union of allrouteId
,search
is a union of allsearch
. The problem with this approach is its impossible for TS to narrow a match based on therouteId
of thematch
.For example this code doesn't type check in TS 5.5:
Another example that would not type check
But now it does with this PR
This is because TS 5.5 can narrow a
match
based on therouteId
of amatch
. This is made possible by creating amatch
type for each route which is a part of a unionisMatch type predicate (idea?)
Narrowing a union based on
routeId
isn't always feasible. Sometimes you want to narrow to matches which contain a certain shape or data. For example, a match might contain data necessary to build breadcrumbs and we want to find matches with the necessary data.This can be achieved with the
isMatch
type predicateisMatch
will narrow a the type of matches to onlymatches
which containloaderData.crumb
. TypeScript is happy because the match types are narrowed