Repository navigation
perf(react-router): bail out of Link re-renders when href and active state are unchanged #7952
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
702eefa
perf(react-router): bail out of Link re-renders when href and active …
matclayton bd4e5b7
refactor: trim redundant comments and brace single-line bodies
matclayton 2ec281e
refactor: publish link state as a tuple
matclayton 8d04b0b
test: assert the published link state, and drop a redundant cast
matclayton 07081d4
refactor: pass activeOptions through instead of destructuring
matclayton 409371b
perf: memoize the href derivation on the built href
matclayton 08735d2
perf: keep _options referentially stable while its contents are equal
matclayton 1f62425
revert: drop the href memoization, it measured no benefit
matclayton 48dc687
docs: explain why useValueStable exists
matclayton 390fe99
perf: cut the bundle cost of the link state selector
matclayton 0dbecc4
perf: pass _options to preloadRoute without the shallow clone
matclayton 8bd81fc
fix: do not collapse explicit-undefined params when stabilising link …
matclayton 8816a29
Merge remote-tracking branch 'origin/main' into link-rerender-bailout
matclayton 2dbcf41
Merge branch 'main' into link-rerender-bailout
Sheraff 68af7c1
chore: empty commit to re-trigger CI
matclayton 80c78e3
Merge branch 'main' into link-rerender-bailout
Sheraff 382efe9
chore: add changeset
matclayton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
why not return the built location from
selectLinkStateand use it here? It's a cheap way to bypass a bunch of work (thouh admittedly "preload" work happens less frequently than "location update" re-renders)(maybe it's not possible without causing a re-render on every location change, just asking in case it is. This might also be the only place in the repo where we use
_builtLocationas a param forpreloadRoute, so if we really don't use it anymore, we might be able to clean that code path up)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 don't think it can work, for the reason you suspected. Two ways to try it:
nextin the compared state. Its identity changes on every location change, so the comparator is always false and every link re-renders on every navigation. That removes the point of the PR.useSyncExternalStoreWithSelectorhands back the previous selection, sonextis stale. Harmless for an absoluteto, wrong for relativetoand inherited params: you'd preload a location built from the old one, silently.On cleaning up the code path, not yet:
_builtLocationis still passed topreloadRoutebypackages/solid-router/src/link.tsx:254andpackages/vue-router/src/link.tsx:255. It'd only become removable once those get the same treatment, which I've deliberately left alone here.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.
once we've confirmed this change is acceptable for React, I think it should also be applied to Solid and Vue to limit drift. And then if what you are saying is correct, we will be able to remove
_builtLocationwhich saves some bytes againThere 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.
Happy to have a go at those but it'll be much more AI driven and less me steering, as honestly I'm not a Vue/solid person, would you like them on this PR?
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 looked at both. Two findings, and they both point away from the plan.
The change does not port, because the problem is React specific. What this PR removes is a React component re-render:
useLinkPropsre-runs and the whole<a>subtree reconciles on every navigation. Solid and Vue have no such thing. In Solid,useLinkPropsruns once per link andnext/hrefOption/isActivearecreateMemos (packages/solid-router/src/link.tsx:136-203), so a location change recomputes those memos and updates only the bound attributes. Vue is the same shape withcomputedand acomputedPropsref built insetup()(packages/vue-router/src/link.tsx:226-244,427-449). Neither re-runs the component body, so there is no re-render to bail out of.Both also already gate on the href the same way this PR's base did, via the location store's equality option:
solid-router/src/link.tsx:128-131andvue-router/src/link.tsx:222-224. Converting either to a tuple selector would do the same work at coarser granularity, and would lose the per-attribute update that fine-grained reactivity gives them. I would leave both alone._builtLocationcannot be removed. It is core redirect plumbing rather than a link optimisation, so it survives regardless of what the adapters do:router-core/src/router.ts:2437throwsredirect({ href, _builtLocation: nextLocation })from core itself.resolveRedirectreads it atrouter.ts:2804-2806to skip rebuilding the location.router.ts:2825gates the dangerous-protocol check on its absence, so a redirect carrying an internally built location deliberately skips validation that an externally suppliedhrefreceives. Removing the field would change that security behaviour.The only piece that becomes dead if all three adapters stop passing it is the
opts._builtLocation ??fallback inpreloadRoute(router.ts:2891), one line. The type, the redirect plumbing and the protocol gate all stay.Worth adding that Solid and Vue pass an already memoized
next()intopreloadRoute, so for them the parameter is a genuine saving. Dropping it there would be a pessimisation, which is the opposite of the byte win. React is the odd one out here only because the selector cannot safely publishnext(my earlier comment in this thread).