perf: improve typescript performance for Link and useNavigate absolut…#1202
Conversation
…e paths
- SplitPaths seems to be hugely expensive as it splits all paths
- UseNavigate was defaulting TFrom and TDefaultFrom to all paths but it was always a
union of string | RoutePaths. Defaulting this to string and not constraining it to
all paths limits the possibility over distributing over such a huge union and auto complete
can be kept intact by using TDefaultFrom | RoutePaths<TRouteTree> | (string & {})
| >}` | ||
| : never | ||
| : TTo extends `./${infer RestTTo}` | ||
| ? Split<AllPaths, false> extends infer SplitPaths |
There was a problem hiding this comment.
Inline like this should still distribute in the conditional case I think
| : './' | ||
| : never) | ||
| | (TFrom extends `/` ? never : '../') | ||
| | (string extends TFrom |
There was a problem hiding this comment.
string is the current default for Link so I had to check for this. But I don't know if it should be '/'
| TDefaultFrom extends string = string, | ||
| >(_defaultOpts?: { | ||
| from?: | ||
| | TDefaultFrom |
There was a problem hiding this comment.
This still provides auto complete without defaulting TFrom to a huge union
There was a problem hiding this comment.
what exactly is the relevant change here?
would TDefaultFrom extends RoutePaths<TRouteTree> | string = string, result in the same improvement?
just trying to understand this better
There was a problem hiding this comment.
I tried this but it breaks autocomplete. I noticed you get autocomplete with:
TDefaultFrom extends RoutePaths<TRouteTree> | string = RoutePaths<TRouteTree>. Which might be why it was defaulting to the union in the first place.
TDefaultFrom | RoutePaths<RouteTree> | (string & {}) provides loose autocomplete. But hopefully this gets fixed with just TDefaultFrom | RoutePaths<RouteTree> | string soon
https://www.totaltypescript.com/typescript-5-3#automatic-loose-autocomplete-on-strings
This method keeps the TDefaultFrom looser but still provides autocomplete which I think is what you want here because the constraint was RoutePaths<TRouteTree> | string which is already very loose due to the | string
The point is to remove the default to a large union but to keep autocomplete. I think defaulting this to a large union could easily sneak in performance issues as you just need to do one extends and you're dealing with it being distributed and it could be hard to track this down.
There was a problem hiding this comment.
I will also explain a separate issue which is connected to this but it was why useNavigate seemed so slow. useNavigate captured TRouteTree as a type parameter and this was used in navigate which is a function you return. It looks like Typescript was checking TRouteTree again when navigate was called. If I let navigate capture TRouteTree instead, the extra checking didn't happen. With a large tree this checking was taking seconds for me. I'm not sure why but this might be to do with generic constraints on TRouteTree and variance. But TRouteTree will never change so this checking was only slowing things down. Especially if the TRouteTree is large.
Anyway, this change helps because TDefaultFrom is only constrained to string and is therefore cheap even if it has to check it
☁️ Nx Cloud ReportCI is running/has finished running commands for commit b82df4b. 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 1 targetSent with 💌 from NxCloud. |
|
thanks for this! |
2 participants
I've been looking into improving typescript performance for Link and useNavigate.
After tracing I noticed it was related to relative pathing for Link. SplitPaths was being defaulted to the result of splitting all paths. This is very slow when you have a large number of routes. I've inlined SplitPaths because when TFrom isn't specified, you should not need to use it as we're dealing with absolute paths. This improved performance significantly.
useNavigate was even slower, it looks like Typescript was doing some eager checking of the route tree. I've solved this the following - useNavigate was defaulting TFrom and TDefaultFrom to all paths but it was always a union of string | RoutePaths. Defaulting this to string and not constraining it to all paths limits the possibility distributing over such a huge union and auto complete can be kept intact by using TDefaultFrom | RoutePaths | (string & {})
I'm wondering if TFrom should be defaulted to '/' as mentioned in the docs
With testing I was able to get like Link down from around 12s to around 2s with around 600 routes.
Thinking and exploring further performance improvements. It will harder to approach improving performance of relative paths but it would be good to improve this too. But apart from that there is a slightly faster way of creating the ParseRoute union
fixes #1091