Skip to content
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

Reduce RouterProvider re-renders when using View Transitions #11803

Merged
merged 8 commits into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/fuzzy-worms-applaud.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-router-dom": patch
---

Memoize some `RouterProvider` internals to reduce uneccesary re-renders
2 changes: 1 addition & 1 deletion .github/workflows/release-experimental.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ jobs:
SHORT_SHA=$(git rev-parse --short HEAD)
NEXT_VERSION=0.0.0-experimental-${SHORT_SHA}
git checkout -b experimental/${NEXT_VERSION}
pnpm run version ${NEXT_VERSION}
pnpm run version:experimental
git push origin --tags

- name: 🏗 Build
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@
"none": "17.3 kB"
},
"packages/react-router-dom/dist/react-router-dom.production.min.js": {
"none": "17.2 kB"
"none": "17.3 kB"
},
"packages/react-router-dom/dist/umd/react-router-dom.production.min.js": {
"none": "23.6 kB"
Expand Down
79 changes: 77 additions & 2 deletions packages/react-router-dom/__tests__/data-browser-router-test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { ErrorResponse, Fetcher } from "@remix-run/router";
import type { ErrorResponse, Fetcher, RouterState } from "@remix-run/router";
import "@testing-library/jest-dom";
import {
act,
Expand Down Expand Up @@ -37,7 +37,7 @@ import {
} from "react-router-dom";

import getHtml from "../../react-router/__tests__/utils/getHtml";
import { createDeferred } from "../../router/__tests__/utils/utils";
import { createDeferred, tick } from "../../router/__tests__/utils/utils";

testDomRouter("<DataBrowserRouter>", createBrowserRouter, (url) =>
getWindowImpl(url, false)
Expand Down Expand Up @@ -7465,6 +7465,81 @@ function testDomRouter(
await waitFor(() => screen.getByText("D"));
expect(spy).toHaveBeenCalledTimes(2);
});

it("Does not cause extra re-renders due to ViewTransitionContext updates", async () => {
let testWindow = getWindow("/");
testWindow.document.startViewTransition = (cb) => {
cb();
return {
ready: Promise.resolve(),
finished: Promise.resolve(),
updateCallbackDone: Promise.resolve(),
skipTransition: () => {},
};
};

let renders: RouterState[] = [];
let router = createTestRouter(
[
{
path: "/",
Component() {
return (
<>
<Link to="/page" unstable_viewTransition>
/page
</Link>
<Outlet />
</>
);
},
children: [
{
index: true,
async loader() {
await tick();
return "INDEX";
},
Component() {
renders.push(useLocation(), useNavigation());
return <h1>{useLoaderData()}</h1>;
},
},
{
path: "page",
async loader() {
await tick();
return "PAGE";
},
Component() {
renders.push(useLocation(), useNavigation());
return <h1>{useLoaderData()}</h1>;
},
},
],
},
],
{ window: testWindow }
);
render(<RouterProvider router={router} />);
await waitFor(() => screen.getByText("INDEX"));

renders = [];
fireEvent.click(screen.getByText("/page"));
await waitFor(() => screen.getByText("PAGE"));

expect(renders).toMatchObject([
// Re-render of current location with navigation.state = "loading"
{ pathname: "/" },
{
state: "loading",
location: { pathname: "/page" },
},
// Render of new location with navigation.state = "idle"
{ pathname: "/page" },
{ state: "idle" },
]);
});
});
});
}
Expand Down
17 changes: 13 additions & 4 deletions packages/react-router-dom/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
Navigator,
RelativeRoutingType,
RouteObject,
RouterProps,
RouterProviderProps,
To,
unstable_PatchRoutesOnMissFunction,
Expand Down Expand Up @@ -708,6 +709,13 @@ export function RouterProvider({
[router, navigator, basename]
);

let routerFuture = React.useMemo<RouterProps["future"]>(
() => ({
v7_relativeSplatPath: router.future.v7_relativeSplatPath,
}),
[router.future.v7_relativeSplatPath]
);

// The fragment and {null} here are important! We need them to keep React 18's
// useId happy when we are server-rendering since we may have a <script> here
// containing the hydrated server-side staticContext (from StaticRouterProvider).
Expand All @@ -725,12 +733,10 @@ export function RouterProvider({
location={state.location}
navigationType={state.historyAction}
navigator={navigator}
future={{
v7_relativeSplatPath: router.future.v7_relativeSplatPath,
}}
future={routerFuture}
Comment on lines -728 to +736
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memoized above to ensure object stability across re-renders

>
{state.initialized || router.future.v7_partialHydration ? (
<DataRoutes
<MemoizedDataRoutes
Copy link
Contributor Author

@brophdawg11 brophdawg11 Jul 15, 2024

Choose a reason for hiding this comment

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

Has to React.memo this in order to avoid unnecessary re-renders of route components when we updated ViewTransitionContext.

I would have thought I'd have needed to memoize Router as well but it doesn't appear that one needs to be changed...I'm unsure why though?

routes={router.routes}
future={router.future}
state={state}
Expand All @@ -748,6 +754,9 @@ export function RouterProvider({
);
}

// Memoize to avoid re-renders when updating `ViewTransitionContext`
const MemoizedDataRoutes = React.memo(DataRoutes);

function DataRoutes({
routes,
future,
Expand Down
8 changes: 4 additions & 4 deletions scripts/version.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,11 @@ async function run() {
let routerVersion = currentRouterVersion;

// 2. Confirm the next version number
let answer = await prompt(
`Are you sure you want to bump version ${currentVersion} to ${version}? [Yn] `
);
// let answer = await prompt(
// `Are you sure you want to bump version ${currentVersion} to ${version}? [Yn] `
// );

if (answer === false) return 0;
// if (answer === false) return 0;

// We only handle @remix-run/router for experimental since in normal/pre
// releases it's versioned independently from the rest of the packages
Expand Down