diff --git a/.changeset/stable-use-revalidator.md b/.changeset/stable-use-revalidator.md new file mode 100644 index 0000000000..e94ff6a05b --- /dev/null +++ b/.changeset/stable-use-revalidator.md @@ -0,0 +1,5 @@ +--- +"react-router": patch +--- + +Ensure `useRevalidator` is referentially stable across re-renders if revalidations are not actively occuring diff --git a/packages/react-router-dom/__tests__/concurrent-mode-navigations-test.tsx b/packages/react-router-dom/__tests__/concurrent-mode-navigations-test.tsx index 5f494ffcd0..3cd5ac1ee0 100644 --- a/packages/react-router-dom/__tests__/concurrent-mode-navigations-test.tsx +++ b/packages/react-router-dom/__tests__/concurrent-mode-navigations-test.tsx @@ -13,12 +13,13 @@ import { import { act, fireEvent, - prettyDOM, render, screen, waitFor, } from "@testing-library/react"; import { JSDOM } from "jsdom"; +import createDeferred from "../../router/__tests__/utils/createDeferred"; +import getHtml from "react-router/__tests__/utils/getHtml"; describe("Handles concurrent mode features during navigations", () => { function getComponents() { @@ -378,36 +379,3 @@ function getWindowImpl(initialUrl: string, isHash = false): Window { dom.window.history.replaceState(null, "", (isHash ? "#" : "") + initialUrl); return dom.window as unknown as Window; } - -function getHtml(container: HTMLElement) { - return prettyDOM(container, undefined, { - highlight: false, - }); -} - -async function tick() { - await new Promise((r) => setTimeout(r, 0)); -} - -function createDeferred() { - let resolve: (val?: any) => Promise; - let reject: (error?: Error) => Promise; - let promise = new Promise((res, rej) => { - resolve = async (val: any) => { - res(val); - await tick(); - await promise; - }; - reject = async (error?: Error) => { - rej(error); - await promise.catch(() => tick()); - }; - }); - return { - promise, - //@ts-ignore - resolve, - //@ts-ignore - reject, - }; -} diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx index 9b0e75fa35..83828d986e 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -6,7 +6,6 @@ import { fireEvent, waitFor, screen, - prettyDOM, } from "@testing-library/react"; import "@testing-library/jest-dom"; import type { ErrorResponse, Fetcher } from "@remix-run/router"; @@ -37,6 +36,9 @@ import { createRoutesFromElements, } from "react-router-dom"; +import createDeferred from "../../router/__tests__/utils/createDeferred"; +import getHtml from "../../react-router/__tests__/utils/getHtml"; + testDomRouter("", createBrowserRouter, (url) => getWindowImpl(url, false) ); @@ -5925,48 +5927,9 @@ function testDomRouter( }); } -function createDeferred() { - let resolve: (val?: any) => Promise; - let reject: (error?: Error) => Promise; - let promise = new Promise((res, rej) => { - resolve = async (val: any) => { - res(val); - try { - await promise; - } catch (e) {} - }; - reject = async (error?: Error) => { - rej(error); - try { - await promise; - } catch (e) {} - }; - }); - return { - promise, - //@ts-ignore - resolve, - //@ts-ignore - reject, - }; -} - function getWindowImpl(initialUrl: string, isHash = false): Window { // Need to use our own custom DOM in order to get a working history const dom = new JSDOM(``, { url: "http://localhost/" }); dom.window.history.replaceState(null, "", (isHash ? "#" : "") + initialUrl); return dom.window as unknown as Window; } - -function getHtml(container: HTMLElement) { - return prettyDOM(container, undefined, { - highlight: false, - theme: { - comment: null, - content: null, - prop: null, - tag: null, - value: null, - }, - }); -} diff --git a/packages/react-router-dom/__tests__/scroll-restoration-test.tsx b/packages/react-router-dom/__tests__/scroll-restoration-test.tsx index f30ae3d213..d4309caacc 100644 --- a/packages/react-router-dom/__tests__/scroll-restoration-test.tsx +++ b/packages/react-router-dom/__tests__/scroll-restoration-test.tsx @@ -1,6 +1,6 @@ import { JSDOM } from "jsdom"; import * as React from "react"; -import { render, prettyDOM, fireEvent, screen } from "@testing-library/react"; +import { render, fireEvent, screen } from "@testing-library/react"; import "@testing-library/jest-dom"; import { Link, @@ -10,6 +10,8 @@ import { createBrowserRouter, } from "react-router-dom"; +import getHtml from "../../react-router/__tests__/utils/getHtml"; + describe(`ScrollRestoration`, () => { it("removes the basename from the location provided to getKey", () => { let getKey = jest.fn(() => "mykey"); @@ -70,16 +72,3 @@ function getWindowImpl(initialUrl: string): Window { dom.window.history.replaceState(null, "", initialUrl); return dom.window as unknown as Window; } - -function getHtml(container: HTMLElement) { - return prettyDOM(container, undefined, { - highlight: false, - theme: { - comment: null, - content: null, - prop: null, - tag: null, - value: null, - }, - }); -} diff --git a/packages/react-router/__tests__/data-memory-router-test.tsx b/packages/react-router/__tests__/data-memory-router-test.tsx index 303581da20..89297fd9ee 100644 --- a/packages/react-router/__tests__/data-memory-router-test.tsx +++ b/packages/react-router/__tests__/data-memory-router-test.tsx @@ -1,22 +1,20 @@ -import * as React from "react"; +import type { ErrorResponse } from "@remix-run/router"; +import "@testing-library/jest-dom"; import { - render, fireEvent, - waitFor, - screen, - prettyDOM, queryByText, + render, + screen, + waitFor, } from "@testing-library/react"; -import "@testing-library/jest-dom"; -import type { ErrorResponse, FormMethod } from "@remix-run/router"; -import { joinPaths } from "@remix-run/router"; +import * as React from "react"; import { Await, MemoryRouter, + Outlet, Route, - Routes, RouterProvider, - Outlet, + Routes, createMemoryRouter, createRoutesFromElements, defer, @@ -27,13 +25,15 @@ import { useLoaderData, useLocation, useMatches, - useRouteLoaderData, - useRouteError, useNavigation, - useRevalidator, - UNSAFE_DataRouterContext as DataRouterContext, + useRouteError, + useRouteLoaderData, } from "react-router"; +import createDeferred from "../../router/__tests__/utils/createDeferred"; +import MemoryNavigate from "./utils/MemoryNavigate"; +import getHtml from "./utils/getHtml"; + describe("createMemoryRouter", () => { let consoleWarn: jest.SpyInstance; let consoleError: jest.SpyInstance; @@ -909,107 +909,6 @@ describe("createMemoryRouter", () => { }); }); - it("reloads data using useRevalidator", async () => { - let count = 1; - let router = createMemoryRouter( - createRoutesFromElements( - }> - `count=${++count}`} - element={} - /> - - ), - { - initialEntries: ["/foo"], - hydrationData: { - loaderData: { - "0-0": "count=1", - }, - }, - } - ); - let { container } = render(); - - function Layout() { - let navigation = useNavigation(); - let { revalidate, state } = useRevalidator(); - return ( -
- -

{navigation.state}

-

{state}

- -
- ); - } - - function Foo() { - let data = useLoaderData() as string; - return

{data}

; - } - - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- -

- idle -

-

- idle -

-

- count=1 -

-
-
" - `); - - fireEvent.click(screen.getByText("Revalidate")); - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- -

- idle -

-

- loading -

-

- count=1 -

-
-
" - `); - - await waitFor(() => screen.getByText("count=2")); - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- -

- idle -

-

- idle -

-

- count=2 -

-
-
" - `); - }); - it("renders descendent routes inside a data router", () => { let router = createMemoryRouter( createRoutesFromElements( @@ -2432,120 +2331,6 @@ describe("createMemoryRouter", () => { ); errorSpy.mockRestore(); }); - - it("allows a successful useRevalidator to resolve the error boundary (loader + child boundary)", async () => { - let shouldFail = true; - let router = createMemoryRouter( - createRoutesFromElements( - ( - <> - /child - - - )} - > - { - if (shouldFail) { - shouldFail = false; - throw new Error("Broken"); - } else { - return "Fixed"; - } - }} - Component={() =>

{("Child:" + useLoaderData()) as string}

} - ErrorBoundary={() => { - let { revalidate } = useRevalidator(); - return ( - <> -

{"Error:" + (useRouteError() as Error).message}

- - - ); - }} - /> - - ) - ); - - let { container } = render( -
- -
- ); - - fireEvent.click(screen.getByText("/child")); - await waitFor(() => screen.getByText("Error:Broken")); - expect(getHtml(container)).toMatch("Error:Broken"); - expect(router.state.errors).not.toBe(null); - - fireEvent.click(screen.getByText("Try again")); - await waitFor(() => { - expect(queryByText(container, "Child:Fixed")).toBeInTheDocument(); - }); - expect(getHtml(container)).toMatch("Child:Fixed"); - expect(router.state.errors).toBe(null); - }); - - it("allows a successful useRevalidator to resolve the error boundary (loader + parent boundary)", async () => { - let shouldFail = true; - let router = createMemoryRouter( - createRoutesFromElements( - ( - <> - /child - - - )} - ErrorBoundary={() => { - let { revalidate } = useRevalidator(); - return ( - <> -

{"Error:" + (useRouteError() as Error).message}

- - - ); - }} - > - { - if (shouldFail) { - shouldFail = false; - throw new Error("Broken"); - } else { - return "Fixed"; - } - }} - Component={() =>

{("Child:" + useLoaderData()) as string}

} - /> - - ) - ); - - let { container } = render( -
- -
- ); - - fireEvent.click(screen.getByText("/child")); - await waitFor(() => screen.getByText("Error:Broken")); - expect(getHtml(container)).toMatch("Error:Broken"); - expect(router.state.errors).not.toBe(null); - - fireEvent.click(screen.getByText("Try again")); - await waitFor(() => { - expect(queryByText(container, "Child:Fixed")).toBeInTheDocument(); - }); - expect(getHtml(container)).toMatch("Child:Fixed"); - expect(router.state.errors).toBe(null); - }); }); describe("defer", () => { @@ -3338,75 +3123,3 @@ describe("createMemoryRouter", () => { }); }); }); - -function createDeferred() { - let resolve: (val?: any) => Promise; - let reject: (error?: Error) => Promise; - let promise = new Promise((res, rej) => { - resolve = async (val: any) => { - res(val); - try { - await promise; - } catch (e) {} - }; - reject = async (error?: Error) => { - rej(error); - try { - await promise; - } catch (e) {} - }; - }); - return { - promise, - //@ts-ignore - resolve, - //@ts-ignore - reject, - }; -} - -function getHtml(container: HTMLElement) { - return prettyDOM(container, undefined, { - highlight: false, - }); -} - -function MemoryNavigate({ - to, - formMethod, - formData, - children, -}: { - to: string; - formMethod?: FormMethod; - formData?: FormData; - children: React.ReactNode; -}) { - let dataRouterContext = React.useContext(DataRouterContext); - - let onClickHandler = React.useCallback( - async (event: React.MouseEvent) => { - event.preventDefault(); - if (formMethod && formData) { - dataRouterContext?.router.navigate(to, { formMethod, formData }); - } else { - dataRouterContext?.router.navigate(to); - } - }, - [dataRouterContext, to, formMethod, formData] - ); - - // Only prepend the basename to the rendered href, send the non-prefixed `to` - // value into the router since it will prepend the basename - let basename = dataRouterContext?.basename; - let href = to; - if (basename && basename !== "/") { - href = to === "/" ? basename : joinPaths([basename, to]); - } - - return formData ? ( -
- ) : ( - - ); -} diff --git a/packages/react-router/__tests__/navigate-test.tsx b/packages/react-router/__tests__/navigate-test.tsx index 8a3c03615f..8ec88ede60 100644 --- a/packages/react-router/__tests__/navigate-test.tsx +++ b/packages/react-router/__tests__/navigate-test.tsx @@ -10,7 +10,9 @@ import { createMemoryRouter, useLocation, } from "react-router"; -import { prettyDOM, render, screen, waitFor } from "@testing-library/react"; +import { render, screen, waitFor } from "@testing-library/react"; + +import getHtml from "../../react-router/__tests__/utils/getHtml"; describe("", () => { describe("with an absolute href", () => { @@ -1153,9 +1155,3 @@ describe("concurrent mode", () => { }); }); }); - -function getHtml(container: HTMLElement) { - return prettyDOM(container, undefined, { - highlight: false, - }); -} diff --git a/packages/react-router/__tests__/use-revalidator-test.tsx b/packages/react-router/__tests__/use-revalidator-test.tsx new file mode 100644 index 0000000000..192262d94d --- /dev/null +++ b/packages/react-router/__tests__/use-revalidator-test.tsx @@ -0,0 +1,286 @@ +import "@testing-library/jest-dom"; +import { + fireEvent, + queryByText, + render, + screen, + waitFor, +} from "@testing-library/react"; +import * as React from "react"; +import { + Outlet, + Route, + RouterProvider, + createMemoryRouter, + createRoutesFromElements, + useLoaderData, + useNavigation, + useRevalidator, + useRouteError, +} from "react-router"; +import MemoryNavigate from "./utils/MemoryNavigate"; +import getHtml from "./utils/getHtml"; + +describe("useRevalidator", () => { + it("reloads data using useRevalidator", async () => { + let count = 1; + let router = createMemoryRouter( + createRoutesFromElements( + }> + `count=${++count}`} + element={} + /> + + ), + { + initialEntries: ["/foo"], + hydrationData: { + loaderData: { + "0-0": "count=1", + }, + }, + } + ); + let { container } = render(); + + function Layout() { + let navigation = useNavigation(); + let { revalidate, state } = useRevalidator(); + return ( +
+ +

{navigation.state}

+

{state}

+ +
+ ); + } + + function Foo() { + let data = useLoaderData() as string; + return

{data}

; + } + + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+
+ +

+ idle +

+

+ idle +

+

+ count=1 +

+
+
" + `); + + fireEvent.click(screen.getByText("Revalidate")); + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+
+ +

+ idle +

+

+ loading +

+

+ count=1 +

+
+
" + `); + + await waitFor(() => screen.getByText("count=2")); + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+
+ +

+ idle +

+

+ idle +

+

+ count=2 +

+
+
" + `); + }); + + it("allows a successful useRevalidator to resolve the error boundary (loader + child boundary)", async () => { + let shouldFail = true; + let router = createMemoryRouter( + createRoutesFromElements( + ( + <> + /child + + + )} + > + { + if (shouldFail) { + shouldFail = false; + throw new Error("Broken"); + } else { + return "Fixed"; + } + }} + Component={() =>

{("Child:" + useLoaderData()) as string}

} + ErrorBoundary={() => { + let { revalidate } = useRevalidator(); + return ( + <> +

{"Error:" + (useRouteError() as Error).message}

+ + + ); + }} + /> + + ) + ); + + let { container } = render( +
+ +
+ ); + + fireEvent.click(screen.getByText("/child")); + await waitFor(() => screen.getByText("Error:Broken")); + expect(getHtml(container)).toMatch("Error:Broken"); + expect(router.state.errors).not.toBe(null); + + fireEvent.click(screen.getByText("Try again")); + await waitFor(() => { + expect(queryByText(container, "Child:Fixed")).toBeInTheDocument(); + }); + expect(getHtml(container)).toMatch("Child:Fixed"); + expect(router.state.errors).toBe(null); + }); + + it("allows a successful useRevalidator to resolve the error boundary (loader + parent boundary)", async () => { + let shouldFail = true; + let router = createMemoryRouter( + createRoutesFromElements( + ( + <> + /child + + + )} + ErrorBoundary={() => { + let { revalidate } = useRevalidator(); + return ( + <> +

{"Error:" + (useRouteError() as Error).message}

+ + + ); + }} + > + { + if (shouldFail) { + shouldFail = false; + throw new Error("Broken"); + } else { + return "Fixed"; + } + }} + Component={() =>

{("Child:" + useLoaderData()) as string}

} + /> + + ) + ); + + let { container } = render( +
+ +
+ ); + + fireEvent.click(screen.getByText("/child")); + await waitFor(() => screen.getByText("Error:Broken")); + expect(getHtml(container)).toMatch("Error:Broken"); + expect(router.state.errors).not.toBe(null); + + fireEvent.click(screen.getByText("Try again")); + await waitFor(() => { + expect(queryByText(container, "Child:Fixed")).toBeInTheDocument(); + }); + expect(getHtml(container)).toMatch("Child:Fixed"); + expect(router.state.errors).toBe(null); + }); + + it("is stable across location changes", async () => { + let count = 0; + let router = createMemoryRouter([ + { + path: "/", + Component() { + let revalidator = useRevalidator(); + + React.useEffect(() => { + count++; + }, [revalidator]); + return ( +
+ Link to Home{" "} + Link to Foo + +
+ ); + }, + children: [ + { + index: true, + Component() { + return

Home Page

; + }, + }, + { + path: "foo", + Component() { + return

Foo Page

; + }, + }, + ], + }, + ]); + + render(); + + fireEvent.click(screen.getByText("Link to Foo")); + await waitFor(() => screen.getByText("Foo Page")); + + fireEvent.click(screen.getByText("Link to Home")); + await waitFor(() => screen.getByText("Home Page")); + + expect(count).toBe(1); + }); +}); diff --git a/packages/react-router/__tests__/utils/MemoryNavigate.tsx b/packages/react-router/__tests__/utils/MemoryNavigate.tsx new file mode 100644 index 0000000000..9f418cd619 --- /dev/null +++ b/packages/react-router/__tests__/utils/MemoryNavigate.tsx @@ -0,0 +1,44 @@ +import type { FormMethod } from "@remix-run/router"; +import { joinPaths } from "@remix-run/router"; +import * as React from "react"; +import { UNSAFE_DataRouterContext } from "react-router"; + +export default function MemoryNavigate({ + to, + formMethod, + formData, + children, +}: { + to: string; + formMethod?: FormMethod; + formData?: FormData; + children: React.ReactNode; +}) { + let dataRouterContext = React.useContext(UNSAFE_DataRouterContext); + + let onClickHandler = React.useCallback( + async (event: React.MouseEvent) => { + event.preventDefault(); + if (formMethod && formData) { + dataRouterContext?.router.navigate(to, { formMethod, formData }); + } else { + dataRouterContext?.router.navigate(to); + } + }, + [dataRouterContext, to, formMethod, formData] + ); + + // Only prepend the basename to the rendered href, send the non-prefixed `to` + // value into the router since it will prepend the basename + let basename = dataRouterContext?.basename; + let href = to; + if (basename && basename !== "/") { + href = to === "/" ? basename : joinPaths([basename, to]); + } + + return formData ? ( + + ) : ( + + ); +} diff --git a/packages/react-router/__tests__/utils/getHtml.ts b/packages/react-router/__tests__/utils/getHtml.ts new file mode 100644 index 0000000000..15c90b1163 --- /dev/null +++ b/packages/react-router/__tests__/utils/getHtml.ts @@ -0,0 +1,7 @@ +import { prettyDOM } from "@testing-library/react"; + +export default function getHtml(container: HTMLElement) { + return prettyDOM(container, undefined, { + highlight: false, + }); +} diff --git a/packages/react-router/__tests__/utils/tick.ts b/packages/react-router/__tests__/utils/tick.ts new file mode 100644 index 0000000000..13b1ec0e74 --- /dev/null +++ b/packages/react-router/__tests__/utils/tick.ts @@ -0,0 +1,3 @@ +export default async function tick() { + await new Promise((r) => setTimeout(r, 0)); +} diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 44a25ccc10..1f38f931ce 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -821,10 +821,13 @@ export function useNavigation() { export function useRevalidator() { let dataRouterContext = useDataRouterContext(DataRouterHook.UseRevalidator); let state = useDataRouterState(DataRouterStateHook.UseRevalidator); - return { - revalidate: dataRouterContext.router.revalidate, - state: state.revalidation, - }; + return React.useMemo( + () => ({ + revalidate: dataRouterContext.router.revalidate, + state: state.revalidation, + }), + [dataRouterContext.router.revalidate, state.revalidation] + ); } /** diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 4bdd1c4c7f..f602717bf6 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -129,29 +129,6 @@ function invariant(value: any, message?: string) { } } -function createDeferred() { - let resolve: (val?: any) => Promise; - let reject: (error?: Error) => Promise; - let promise = new Promise((res, rej) => { - resolve = async (val: any) => { - res(val); - await tick(); - await promise; - }; - reject = async (error?: Error) => { - rej(error); - await promise.catch(() => tick()); - }; - }); - return { - promise, - //@ts-ignore - resolve, - //@ts-ignore - reject, - }; -} - function createFormData(obj: Record): FormData { let formData = new FormData(); Object.entries(obj).forEach((e) => formData.append(e[0], e[1])); @@ -4972,8 +4949,7 @@ describe("a router", () => { await t.navigate("/tasks", { // @ts-expect-error formMethod: "head", - // @ts-expect-error - formData: formData, + formData, }); expect(t.router.state.navigation.state).toBe("idle"); expect(t.router.state.location).toMatchObject({ @@ -5013,7 +4989,6 @@ describe("a router", () => { await t.navigate("/tasks", { // @ts-expect-error formMethod: "options", - // @ts-expect-error formData: formData, }); expect(t.router.state.navigation.state).toBe("idle"); @@ -17517,3 +17492,28 @@ describe("a router", () => { }); }); }); + +// We use a slightly modified version of createDeferred here that incoudes the +// tick() calls to let the router finish updating +function createDeferred() { + let resolve: (val?: any) => Promise; + let reject: (error?: Error) => Promise; + let promise = new Promise((res, rej) => { + resolve = async (val: any) => { + res(val); + await tick(); + await promise; + }; + reject = async (error?: Error) => { + rej(error); + await promise.catch(() => tick()); + }; + }); + return { + promise, + //@ts-ignore + resolve, + //@ts-ignore + reject, + }; +} diff --git a/packages/router/__tests__/utils/createDeferred.ts b/packages/router/__tests__/utils/createDeferred.ts new file mode 100644 index 0000000000..39341bab25 --- /dev/null +++ b/packages/router/__tests__/utils/createDeferred.ts @@ -0,0 +1,25 @@ +export default function createDeferred() { + let resolve: (val?: any) => Promise; + let reject: (error?: Error) => Promise; + let promise = new Promise((res, rej) => { + resolve = async (val: any) => { + res(val); + try { + await promise; + } catch (e) {} + }; + reject = async (error?: Error) => { + rej(error); + try { + await promise; + } catch (e) {} + }; + }); + return { + promise, + //@ts-ignore + resolve, + //@ts-ignore + reject, + }; +}