-
-
Notifications
You must be signed in to change notification settings - Fork 575
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): improve performance of Link #2359
base: main
Are you sure you want to change the base?
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit f6015bf. 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. |
): Array<AnyRouteMatch> | ||
public matchRoutes( | ||
next: ParsedLocation, | ||
opts?: MatchRoutesOpts, | ||
routeFullPath?: string, |
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.
❓ Would it make sense to extend ParsedLocation
with the full path instead?
how did you measure the time? |
const foundRoute = this.flatRoutes.find((route) => { | ||
let foundRoute: AnyRoute | undefined = routeFullPath | ||
? this.routesByPath[routeFullPath] | ||
: 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.
does this mean if to
is not specified, the link will still be slow?
in this case, can we use the current location of router instead?
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.
Is it a valid use case to use a Link without to
? If yes, we could provide the current match's fullPath
as a fallback.
Currently, it only speeds up the resolve process for links with absolute paths, because they can be easily looked up in the routeById
map. Is there any util to quickly resolve relative paths?
I used this code in the large-file-based example and measured the time to render the Home component in react dev tools. It renders 10000 links with generated IDs used as a route param. Is there any performance benchmark setup in this repo? How about https://www.npmjs.com/package/reassure? |
Addresses #2011 to improve performance of
Link
androuter.buildLocation
.matchRoutesInternal
traverses all routes inflatRoutes
, which degrades the performance of Link with the number of routes registered. This PR adds an optimization to look up the route inroutesByPath
in constant time for absolute paths into
.Rendering times of 10000 Links as described in the original discussion thread #2011 (comment)