Skip to content

fix: Memoize route component to avoid double render for incoming async routes #72

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.

Sign up for GitHub

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
rschristian merged 2 commits into from
Mar 2, 2025
Merged

fix: Memoize route component to avoid double render for incoming async routes #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9457a30
fix: Try to fix unnecessary double-render
rschristian Feb 26, 2025
9328e58
test: Add test case
rschristian Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,14 @@ export function Router(props) {

/** @type {VNode<any> | undefined} */
let incoming = pathRoute || defaultRoute;

const isHydratingSuspense = cur.current && cur.current.__u & MODE_HYDRATE && cur.current.__u & MODE_SUSPENDED;
const isHydratingBool = cur.current && cur.current.__h;
const routeChanged = useMemo(() => {
prev.current = cur.current;

cur.current = /** @type {VNode<any>} */ (h(RouteContext.Provider, { value: matchProps }, incoming));

// Only mark as an update if the route component changed.
const outgoing = prev.current && prev.current.props.children;
if (!outgoing || !incoming || incoming.type !== outgoing.type || incoming.props.component !== outgoing.props.component) {
Expand All @@ -157,12 +162,8 @@ export function Router(props) {
return true;
}
return false;
}, [url]);
}, [url, JSON.stringify(matchProps)]);
Copy link
Member Author

@rschristian rschristian Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoviDeCroock Have any opinions on what we do here? Obviously JSON.stringify ain't ideal though I'm not sure if we want to start doing deep comparisons ourselves or with a dep either.

As the updated test cases show, your changes in #12 have us rendering routes more often than we should, but to not regress we'd need a dependency on the user-passed props.


const isHydratingSuspense = cur.current && cur.current.__u & MODE_HYDRATE && cur.current.__u & MODE_SUSPENDED;
const isHydratingBool = cur.current && cur.current.__h;
// @ts-ignore
cur.current = /** @type {VNode<any>} */ (h(RouteContext.Provider, { value: matchProps }, incoming));
if (isHydratingSuspense) {
cur.current.__u |= MODE_HYDRATE;
cur.current.__u |= MODE_SUSPENDED;
Expand Down
2 changes: 2 additions & 0 deletions test/router.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ describe('Router', () => {
await sleep(10);

expect(scratch).to.have.property('innerHTML', '<h1>B</h1><p>hello</p>');
expect(B).to.have.been.calledOnce;
Copy link
Member Author

@rschristian rschristian Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this, B (or any async route) would be called twice after route change.

expect(B).to.have.been.calledWith({ path: '/b', query: {}, params: {}, rest: '' });

B.resetHistory();
Expand All @@ -277,6 +278,7 @@ describe('Router', () => {
await sleep(10);

expect(scratch).to.have.property('innerHTML', '<h1>C</h1>');
expect(C).to.have.been.calledOnce;
expect(C).to.have.been.calledWith({ path: '/c', query: {}, params: {}, rest: '' });

// "instant" routing to already-loaded routes
Expand Down