diff --git a/.changeset/fetcher-cleanup.md b/.changeset/fetcher-cleanup.md index 029329149e..ad34b7a2f8 100644 --- a/.changeset/fetcher-cleanup.md +++ b/.changeset/fetcher-cleanup.md @@ -2,10 +2,9 @@ "@remix-run/router": minor --- -Add a new `future.v7_fetcherPersist` flag to the `@remix-run/router` to change the persistence behavior of fetchers when `router.deleteFetcher` is called. Instead of being immediately cleaned up, fetchers will persist until they return to an `idle` state([RFC](https://github.com/remix-run/remix/discussions/7698)) +Add a new `future.v7_fetcherPersist` flag to the `@remix-run/router` to change the persistence behavior of fetchers when `router.deleteFetcher` is called. Instead of being immediately cleaned up, fetchers will persist until they return to an `idle` state ([RFC](https://github.com/remix-run/remix/discussions/7698)) - This is sort of a long-standing bug fix as the `useFetchers()` API was always supposed to only reflect **in-flight** fetcher information for pending/optimistic UI -- it was not intended to reflect fetcher data or hang onto fetchers after they returned to an `idle` state -- With `v7_fetcherPersist`, the `router` only knows about in-flight fetchers - they do not exist in `state.fetchers` until a `fetch()` call is made, and they are removed as soon as it returns to `idle` (and the data is handed off to the React layer) - Keep an eye out for the following specific behavioral changes when opting into this flag and check your app for compatibility: - Fetchers that complete _while still mounted_ will no longer appear in `useFetchers()`. They served effectively no purpose in there since you can access the data via `useFetcher().data`). - Fetchers that previously unmounted _while in-flight_ will not be immediately aborted and will instead be cleaned up once they return to an `idle` state. They will remain exposed via `useFetchers` while in-flight so you can still access pending/optimistic data after unmount. diff --git a/.changeset/fetcher-ref-count.md b/.changeset/fetcher-ref-count.md new file mode 100644 index 0000000000..1c18e45fbd --- /dev/null +++ b/.changeset/fetcher-ref-count.md @@ -0,0 +1,8 @@ +--- +"@remix-run/router": minor +--- + +When `v7_fetcherPersist` is enabled, the router now performs ref-counting on fetcher keys via `getFetcher`/`deleteFetcher` so it knows when a given fetcher is totally unmounted from the UI + +- Once a fetcher has been totally unmounted, we can ignore post-processing of a persisted fetcher result such as a redirect or an error +- The router will also pass a new `deletedFetchers` array to the subscriber callbacks so that the UI layer can remove associated fetcher data diff --git a/.changeset/fix-router-future-prop.md b/.changeset/fix-router-future-prop.md index cb564b5a69..d47c500fde 100644 --- a/.changeset/fix-router-future-prop.md +++ b/.changeset/fix-router-future-prop.md @@ -3,4 +3,4 @@ "react-router-dom": patch --- -Fix the `future`prop on `BrowserRouter`, `HashRouter` and `MemoryRouter` so that it accepts a `Partial` instead of requiring all flags to be included. +Fix the `future` prop on `BrowserRouter`, `HashRouter` and `MemoryRouter` so that it accepts a `Partial` instead of requiring all flags to be included. diff --git a/package.json b/package.json index 8e48624414..1b12bdb1f8 100644 --- a/package.json +++ b/package.json @@ -110,7 +110,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "48.7 kB" + "none": "49.0 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13.9 kB" @@ -119,10 +119,10 @@ "none": "16.3 kB" }, "packages/react-router-dom/dist/react-router-dom.production.min.js": { - "none": "16.5 kB" + "none": "16.1 kB" }, "packages/react-router-dom/dist/umd/react-router-dom.production.min.js": { - "none": "22.7 kB" + "none": "22.3 kB" } } } 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 6c43240fcc..c6ffbc6354 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -5533,7 +5533,81 @@ function testDomRouter( { index: true, Component() { - let fetcher = useFetcher({ persist: true }); + let fetcher = useFetcher(); + return ( + + ); + }, + }, + { + path: "page", + Component() { + let data = useLoaderData() as { count: number }; + return

{`Page (${data.count})`}

; + }, + async loader() { + await new Promise((r) => setTimeout(r, 10)); + return { count: ++count }; + }, + }, + ], + }, + { + path: "/fetch", + action: () => dfd.promise, + }, + ], + { window: getWindow("/"), future: { v7_fetcherPersist: true } } + ); + let { container } = render(); + + expect(getHtml(container)).toMatch("Num fetchers: 0"); + + fireEvent.click(screen.getByText("Submit (idle)")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + expect(getHtml(container)).toMatch("Submit (submitting)"); + + fireEvent.click(screen.getByText("Go to /page")); + await waitFor(() => screen.getByText("Page (1)")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + + // Resolve after the navigation and revalidation + dfd.resolve("FETCH"); + await waitFor(() => screen.getByText("Num fetchers: 0")); + expect(getHtml(container)).toMatch("Page (1)"); + }); + + it("submitting fetchers w/revalidations are cleaned up on completion (remounted)", async () => { + let count = 0; + let dfd = createDeferred(); + let router = createTestRouter( + [ + { + path: "/", + Component() { + let fetchers = useFetchers(); + return ( + <> + 
{`Num fetchers: ${fetchers.length}`}
+ Go to /page + + + ); + }, + children: [ + { + index: true, + Component() { + let fetcher = useFetcher({ key: "me" }); return ( + ); + }, + }, + { + path: "page", + Component() { + return

Page

; + }, + }, + ], + }, + { + path: "/fetch", + loader: () => dfd.promise, + }, + ], + { window: getWindow("/"), future: { v7_fetcherPersist: true } } + ); + let { container } = render(); + + expect(getHtml(container)).toMatch("Num fetchers: 0"); + + fireEvent.click(screen.getByText("Load (idle)")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + expect(getHtml(container)).toMatch("Load (loading)"); + + fireEvent.click(screen.getByText("Go to /page")); + await waitFor(() => screen.getByText("Page")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + expect(getHtml(container)).toMatch("Page"); + + // Reject after the navigation - no-op because the fetcher is no longer mounted + dfd.reject(new Error("FETCH ERROR")); + await waitFor(() => screen.getByText("Num fetchers: 0")); + expect(getHtml(container)).toMatch("Page"); + expect(getHtml(container)).not.toMatch( + "Unexpected Application Error!" + ); + expect(getHtml(container)).not.toMatch("FETCH ERROR"); + }); + + it("unmounted/remounted fetcher.load errors should bubble up to the UI", async () => { + let dfd = createDeferred(); + let router = createTestRouter( + [ + { + path: "/", + Component() { + let fetchers = useFetchers(); + return ( + <> + 
{`Num fetchers: ${fetchers.length}`}
+ Go to /page + + + ); + }, + children: [ + { + index: true, + Component() { + let fetcher = useFetcher({ key: "me" }); + return ( + <> +

Index

+ + + ); + }, + }, + { + path: "page", + Component() { + let fetcher = useFetcher({ key: "me" }); + return ( + <> +

Page

+ 
{fetcher.data}
+ + ); + }, + }, + ], + }, + { + path: "/fetch", + loader: () => dfd.promise, + }, + ], + { window: getWindow("/"), future: { v7_fetcherPersist: true } } + ); + let { container } = render(); + + await waitFor(() => screen.getByText("Index")); + expect(getHtml(container)).toMatch("Num fetchers: 0"); + + fireEvent.click(screen.getByText("Load (idle)")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + expect(getHtml(container)).toMatch("Load (loading)"); + + fireEvent.click(screen.getByText("Go to /page")); + await waitFor(() => screen.getByText("Page")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + expect(getHtml(container)).toMatch("Page"); + + // Reject after the navigation - should trigger the error boundary + // because the fetcher is still mounted in the new location + dfd.reject(new Error("FETCH ERROR")); + await waitFor(() => screen.getByText("FETCH ERROR")); + expect(getHtml(container)).toMatch("Unexpected Application Error!"); + }); + + it("unmounted fetcher.submit errors should not bubble up to the UI", async () => { + let dfd = createDeferred(); + let router = createTestRouter( + [ + { + path: "/", + Component() { + let fetchers = useFetchers(); + return ( + <> + 
{`Num fetchers: ${fetchers.length}`}
+ Go to /page + + + ); + }, + children: [ + { + index: true, + Component() { + let fetcher = useFetcher(); + return ( + + ); + }, + }, + { + path: "page", + Component() { + return

Page

; + }, + }, + ], + }, + { + path: "/fetch", + action: () => dfd.promise, + }, + ], + { window: getWindow("/"), future: { v7_fetcherPersist: true } } + ); + let { container } = render(); + + expect(getHtml(container)).toMatch("Num fetchers: 0"); + + fireEvent.click(screen.getByText("Submit (idle)")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + expect(getHtml(container)).toMatch("Submit (submitting)"); + + fireEvent.click(screen.getByText("Go to /page")); + await waitFor(() => screen.getByText("Page")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + expect(getHtml(container)).toMatch("Page"); + + // Reject after the navigation - no-op because the fetcher is no longer mounted + dfd.reject(new Error("FETCH ERROR")); + await waitFor(() => screen.getByText("Num fetchers: 0")); + expect(getHtml(container)).toMatch("Page"); + expect(getHtml(container)).not.toMatch( + "Unexpected Application Error!" + ); + expect(getHtml(container)).not.toMatch("FETCH ERROR"); + }); + + it("unmounted/remounted fetcher.submit errors should bubble up to the UI", async () => { + let dfd = createDeferred(); + let router = createTestRouter( + [ + { + path: "/", + Component() { + let fetchers = useFetchers(); + return ( + <> + 
{`Num fetchers: ${fetchers.length}`}
+ Go to /page + + + ); + }, + children: [ + { + index: true, + Component() { + let fetcher = useFetcher({ key: "me" }); + return ( + <> +

Index

+ + + ); + }, + }, + { + path: "page", + Component() { + let fetcher = useFetcher({ key: "me" }); + return ( + <> +

Page

+ 
{fetcher.data}
+ + ); + }, + }, + ], + }, + { + path: "/fetch", + action: () => dfd.promise, + }, + ], + { window: getWindow("/"), future: { v7_fetcherPersist: true } } + ); + let { container } = render(); + + await waitFor(() => screen.getByText("Index")); + expect(getHtml(container)).toMatch("Num fetchers: 0"); + + fireEvent.click(screen.getByText("Submit (idle)")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + expect(getHtml(container)).toMatch("Submit (submitting)"); + + fireEvent.click(screen.getByText("Go to /page")); + await waitFor(() => screen.getByText("Page")); + expect(getHtml(container)).toMatch("Num fetchers: 1"); + expect(getHtml(container)).toMatch("Page"); + + // Reject after the navigation - should trigger the error boundary + // because the fetcher is still mounted in the new location + dfd.reject(new Error("FETCH ERROR")); + await waitFor(() => screen.getByText("FETCH ERROR")); + expect(getHtml(container)).toMatch("Unexpected Application Error!"); + }); }); }); diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 7e73c66d6b..9735b2bda7 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -351,13 +351,9 @@ if (__DEV__) { export { ViewTransitionContext as UNSAFE_ViewTransitionContext }; // TODO: (v7) Change the useFetcher data from `any` to `unknown` -type FetchersContextObject = { - fetcherData: Map; - register: (key: string) => void; - unregister: (key: string) => void; -}; +type FetchersContextObject = Map; -const FetchersContext = React.createContext(null); +const FetchersContext = React.createContext(new Map()); if (__DEV__) { FetchersContext.displayName = "Fetchers"; } @@ -442,7 +438,6 @@ export function RouterProvider({ router, future, }: RouterProviderProps): React.ReactElement { - let { fetcherContext, fetcherData } = useFetcherDataLayer(); let [state, setStateImpl] = React.useState(router.state); let [pendingState, setPendingState] = React.useState(); let [vtContext, setVtContext] = React.useState({ @@ -455,6 +450,7 @@ export function RouterProvider({ currentLocation: Location; nextLocation: Location; }>(); + let fetcherData = React.useRef>(new Map()); let { v7_startTransition } = future || {}; let optInStartTransition = React.useCallback( @@ -471,8 +467,9 @@ export function RouterProvider({ let setState = React.useCallback( ( newState: RouterState, - { unstable_viewTransitionOpts: viewTransitionOpts } + { deletedFetchers, unstable_viewTransitionOpts: viewTransitionOpts } ) => { + deletedFetchers.forEach((key) => fetcherData.current.delete(key)); newState.fetchers.forEach((fetcher, key) => { if (fetcher.data !== undefined) { fetcherData.current.set(key, fetcher.data); @@ -609,7 +606,7 @@ export function RouterProvider({ <> - + >(new Map()); - let fetcherData = React.useRef>(new Map()); - - let registerFetcher = React.useCallback( - (key: string) => { - let count = fetcherRefs.current.get(key); - if (count == null) { - fetcherRefs.current.set(key, 1); - } else { - fetcherRefs.current.set(key, count + 1); - } - }, - [fetcherRefs] - ); - - let unregisterFetcher = React.useCallback( - (key: string) => { - let count = fetcherRefs.current.get(key); - if (count == null || count <= 1) { - fetcherRefs.current.delete(key); - fetcherData.current.delete(key); - } else { - fetcherRefs.current.set(key, count - 1); - } - }, - [fetcherData, fetcherRefs] - ); - - let fetcherContext = React.useMemo( - () => ({ - fetcherData: fetcherData.current, - register: registerFetcher, - unregister: unregisterFetcher, - }), - [fetcherData, registerFetcher, unregisterFetcher] - ); - - return { fetcherContext, fetcherData }; -} - // External hooks /** @@ -1570,14 +1526,11 @@ export function useFetcher({ }: { key?: string } = {}): FetcherWithComponents { let { router } = useDataRouterContext(DataRouterHook.UseFetcher); let state = useDataRouterState(DataRouterStateHook.UseFetcher); - let fetchersContext = React.useContext(FetchersContext); + let fetcherData = React.useContext(FetchersContext); let route = React.useContext(RouteContext); let routeId = route.matches[route.matches.length - 1]?.route.id; - invariant( - fetchersContext, - `useFetcher must be used inside a FetchersContext` - ); + invariant(fetcherData, `useFetcher must be used inside a FetchersContext`); invariant(route, `useFetcher must be used inside a RouteContext`); invariant( routeId != null, @@ -1591,18 +1544,15 @@ export function useFetcher({ } // Registration/cleanup - let { fetcherData, register, unregister } = fetchersContext; React.useEffect(() => { - register(fetcherKey); + router.getFetcher(fetcherKey); return () => { - // Unregister from ref counting for the data layer - unregister(fetcherKey); // Tell the router we've unmounted - if v7_fetcherPersist is enabled this // will not delete immediately but instead queue up a delete after the // fetcher returns to an `idle` state router.deleteFetcher(fetcherKey); }; - }, [router, fetcherKey, register, unregister]); + }, [router, fetcherKey]); // Fetcher additions let load = React.useCallback( diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index b8fd113bd2..99fc1426dc 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -111,6 +111,8 @@ export function StaticRouterProvider({ basename: context.basename || "/", }; + let fetchersContext = new Map(); + let hydrateScript = ""; if (hydrate !== false) { @@ -133,13 +135,7 @@ export function StaticRouterProvider({ <> - (), - register: () => {}, - unregister: () => {}, - }} - > + { navigation: IDLE_NAVIGATION, location: expect.objectContaining({ pathname: "/a" }), }), - { unstable_viewTransitionOpts: undefined } + expect.objectContaining({ unstable_viewTransitionOpts: undefined }) ); // PUSH /a -> /b - w/ transition @@ -29,12 +29,12 @@ describe("view transitions", () => { navigation: IDLE_NAVIGATION, location: expect.objectContaining({ pathname: "/b" }), }), - { + expect.objectContaining({ unstable_viewTransitionOpts: { currentLocation: expect.objectContaining({ pathname: "/a" }), nextLocation: expect.objectContaining({ pathname: "/b" }), }, - } + }) ); // POP /b -> /a - w/ transition (cached from above) @@ -44,13 +44,13 @@ describe("view transitions", () => { navigation: IDLE_NAVIGATION, location: expect.objectContaining({ pathname: "/a" }), }), - { + expect.objectContaining({ unstable_viewTransitionOpts: { // Args reversed on POP so same hooks apply currentLocation: expect.objectContaining({ pathname: "/a" }), nextLocation: expect.objectContaining({ pathname: "/b" }), }, - } + }) ); // POP /a -> / - No transition @@ -60,7 +60,7 @@ describe("view transitions", () => { navigation: IDLE_NAVIGATION, location: expect.objectContaining({ pathname: "/" }), }), - { unstable_viewTransitionOpts: undefined } + expect.objectContaining({ unstable_viewTransitionOpts: undefined }) ); unsubscribe(); diff --git a/packages/router/router.ts b/packages/router/router.ts index 971646b6f8..47977e0a29 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -409,6 +409,7 @@ export interface RouterSubscriber { ( state: RouterState, opts: { + deletedFetchers: string[]; unstable_viewTransitionOpts?: ViewTransitionOpts; } ): void; @@ -887,6 +888,9 @@ export function createRouter(init: RouterInit): Router { // Most recent href/match for fetcher.load calls for fetchers let fetchLoadMatches = new Map(); + // Ref-count mounted fetchers so we know when it's ok to clean them up + let activeFetchers = new Map(); + // Fetchers that have requested a delete when using v7_fetcherPersist, // they'll be officially removed after they return to idle let deletedFetchers = new Set(); @@ -1020,27 +1024,39 @@ export function createRouter(init: RouterInit): Router { ...state, ...newState, }; - subscribers.forEach((subscriber) => - subscriber(state, { unstable_viewTransitionOpts: viewTransitionOpts }) - ); - // Remove idle fetchers from state since we only care about in-flight fetchers. + // Prep fetcher cleanup so we can tell the UI which fetcher data entries + // can be removed + let completedFetchers: string[] = []; + let deletedFetchersKeys: string[] = []; + if (future.v7_fetcherPersist) { state.fetchers.forEach((fetcher, key) => { if (fetcher.state === "idle") { if (deletedFetchers.has(key)) { - // If the fetcher has unmounted and called router.deleteFetcher(), - // we can totally delete the fetcher - deleteFetcher(key); + // Unmounted from the UI and can be totally removed + deletedFetchersKeys.push(key); } else { - // Otherwise, it must still be mounted in the UI so we just remove - // it from state now that we've handed off the data to the React - // layer. Things such as fetchLoadMatches remain for revalidation. - state.fetchers.delete(key); + // Returned to idle but still mounted in the UI, so semi-remains for + // revalidations and such + completedFetchers.push(key); } } }); } + + subscribers.forEach((subscriber) => + subscriber(state, { + deletedFetchers: deletedFetchersKeys, + unstable_viewTransitionOpts: viewTransitionOpts, + }) + ); + + // Remove idle fetchers from state since we only care about in-flight fetchers. + if (future.v7_fetcherPersist) { + completedFetchers.forEach((key) => state.fetchers.delete(key)); + deletedFetchersKeys.forEach((key) => deleteFetcher(key)); + } } // Complete a navigation returning the state.navigation back to the IDLE_NAVIGATION @@ -1749,6 +1765,14 @@ export function createRouter(init: RouterInit): Router { } function getFetcher(key: string): Fetcher { + if (future.v7_fetcherPersist) { + activeFetchers.set(key, (activeFetchers.get(key) || 0) + 1); + // If this fetcher was previously marked for deletion, unmark it since we + // have a new instance + if (deletedFetchers.has(key)) { + deletedFetchers.delete(key); + } + } return state.fetchers.get(key) || IDLE_FETCHER; } @@ -1868,7 +1892,7 @@ export function createRouter(init: RouterInit): Router { ); if (fetchRequest.signal.aborted) { - // We can delete this so long as we weren't aborted by ou our own fetcher + // We can delete this so long as we weren't aborted by our own fetcher // re-submit which would have put _new_ controller is in fetchControllers if (fetchControllers.get(key) === abortController) { fetchControllers.delete(key); @@ -1876,6 +1900,12 @@ export function createRouter(init: RouterInit): Router { return; } + if (deletedFetchers.has(key)) { + state.fetchers.set(key, getDoneFetcher(undefined)); + updateState({ fetchers: new Map(state.fetchers) }); + return; + } + if (isRedirectResult(actionResult)) { fetchControllers.delete(key); if (pendingNavigationLoadId > originatingLoadId) { @@ -2127,6 +2157,12 @@ export function createRouter(init: RouterInit): Router { return; } + if (deletedFetchers.has(key)) { + state.fetchers.set(key, getDoneFetcher(undefined)); + updateState({ fetchers: new Map(state.fetchers) }); + return; + } + // If the loader threw a redirect Response, start a new REPLACE navigation if (isRedirectResult(result)) { if (pendingNavigationLoadId > originatingLoadId) { @@ -2145,17 +2181,7 @@ export function createRouter(init: RouterInit): Router { // Process any non-redirect errors thrown if (isErrorResult(result)) { - let boundaryMatch = findNearestBoundary(state.matches, routeId); - state.fetchers.delete(key); - // TODO: In remix, this would reset to IDLE_NAVIGATION if it was a catch - - // do we need to behave any differently with our non-redirect errors? - // What if it was a non-redirect Response? - updateState({ - fetchers: new Map(state.fetchers), - errors: { - [boundaryMatch.route.id]: result.error, - }, - }); + setFetcherError(key, routeId, result.error); return; } @@ -2404,7 +2430,13 @@ export function createRouter(init: RouterInit): Router { function deleteFetcherAndUpdateState(key: string): void { if (future.v7_fetcherPersist) { - deletedFetchers.add(key); + let count = (activeFetchers.get(key) || 0) - 1; + if (count <= 0) { + activeFetchers.delete(key); + deletedFetchers.add(key); + } else { + activeFetchers.set(key, count); + } } else { deleteFetcher(key); }