From 8259e8d2e945a1d0143320fba49da0b5a6c7a574 Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Fri, 27 Jan 2023 17:22:22 -0800 Subject: [PATCH 01/12] initial work + test outline --- packages/router/__tests__/router-test.ts | 66 ++++++++++++++++++++++++ packages/router/router.ts | 60 +++++++++++++++++++-- 2 files changed, 122 insertions(+), 4 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 5c288e8735..527b66c5eb 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -263,6 +263,7 @@ type SetupOpts = { initialEntries?: InitialEntry[]; initialIndex?: number; hydrationData?: HydrationState; + ref?: TMApiRef; }; function setup({ @@ -271,6 +272,7 @@ function setup({ initialEntries, initialIndex, hydrationData, + ref, }: SetupOpts) { let guid = 0; // Global "active" helpers, keyed by navType:guid:loaderOrAction:routeId. @@ -362,6 +364,9 @@ function setup({ return enhancedRoute; }); } + if (ref) { + ref.enhanceRoutes = enhanceRoutes; + } let history = createMemoryHistory({ initialEntries, initialIndex }); jest.spyOn(history, "push"); @@ -757,9 +762,14 @@ function setup({ }; } +type TMApiRef = { + enhanceRoutes(routes: TestRouteObject[]): AgnosticRouteObject[]; +}; + function initializeTmTest(init?: { url?: string; hydrationData?: HydrationState; + ref?: TMApiRef; }) { return setup({ routes: TM_ROUTES, @@ -767,6 +777,7 @@ function initializeTmTest(init?: { loaderData: { root: "ROOT", index: "INDEX" }, }, ...(init?.url ? { initialEntries: [init.url] } : {}), + ref: init?.ref, }); } //#endregion @@ -13544,4 +13555,59 @@ describe("a router", () => { }); }); }); + + describe("routes updates", () => { + it("should retain existing routes until revalidation completes", async () => { + let ref = {} as TMApiRef; + let t = initializeTmTest({ ref }); + let ogRoutes = t.router.routes; + let A = await t.navigate("/foo"); + await A.loaders.foo.resolve(null); + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT", + foo: null, + }); + + let newRoutes = ref.enhanceRoutes([ + { + path: "", + id: "root", + hasErrorBoundary: true, + loader: true, + children: [ + { + path: "/", + id: "index", + loader: true, + action: true, + }, + { + path: "/foo", + id: "foo", + loader: false, + action: true, + }, + ], + }, + ]); + t.router.setNewRoutes(newRoutes); + + expect(t.router.state.revalidation).toBe("loading"); + expect(t.router.routes).toBe(ogRoutes); + + // Get a new revalidation helper that should use the updated routes + let R = await t.revalidate(); + // Should still be og roues on new revalidation as one started by update + // has not yet completed + expect(t.router.routes).toBe(ogRoutes); + // Resolve any loaders that should have ran + await R.loaders.root.resolve("ROOT*"); + // Don't resolve "foo" because it was removed + // Revalidation should be complete + expect(t.router.state.revalidation).toBe("idle"); + // Routes should be updated + expect(t.router.routes).not.toBe(ogRoutes); + expect(t.router.routes).toBe(newRoutes); + }); + }); }); diff --git a/packages/router/router.ts b/packages/router/router.ts index 17fc05f191..826b89bc85 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -211,6 +211,26 @@ export interface Router { */ deleteBlocker(key: string): void; + /** + * @internal + * PRIVATE - DO NOT USE + */ + addRoute(route: AgnosticDataRouteObject, parentId?: string): void; + /** + * @internal + * PRIVATE - DO NOT USE + */ + updateRoute(id: string, route: AgnosticDataRouteObject): void; + /** + * @internal + * PRIVATE - DO NOT USE + */ + deleteRoute(id: string): void; + /** + * TODO: DELETE THIS FOR ABOVE GRANULAR METHODS + */ + setNewRoutes(routes: AgnosticRouteObject[]): void; + /** * @internal * PRIVATE - DO NOT USE @@ -644,6 +664,7 @@ export function createRouter(init: RouterInit): Router { ); let dataRoutes = convertRoutesToDataRoutes(init.routes); + let inFlightDataRoutes: AgnosticDataRouteObject[] | undefined; // Cleanup function for history let unlistenHistory: (() => void) | null = null; // Externally-provided functions to call on all state changes @@ -921,6 +942,11 @@ export function createRouter(init: RouterInit): Router { isMutationMethod(state.navigation.formMethod) && location.state?._isRedirect !== true); + if (inFlightDataRoutes) { + dataRoutes = inFlightDataRoutes; + inFlightDataRoutes = undefined; + } + updateState({ ...newState, // matches, errors, fetchers go through as-is actionData, @@ -1108,14 +1134,15 @@ export function createRouter(init: RouterInit): Router { saveScrollPosition(state.location, state.matches); pendingPreventScrollReset = (opts && opts.preventScrollReset) === true; + let routesToUse = inFlightDataRoutes || dataRoutes; let loadingNavigation = opts && opts.overrideNavigation; - let matches = matchRoutes(dataRoutes, location, init.basename); + let matches = matchRoutes(routesToUse, location, init.basename); // Short circuit with a 404 on the root error boundary if we match nothing if (!matches) { let error = getInternalRouterError(404, { pathname: location.pathname }); let { matches: notFoundMatches, route } = - getShortCircuitMatches(dataRoutes); + getShortCircuitMatches(routesToUse); // Cancel all pending deferred on 404s since we don't keep any routes cancelActiveDeferreds(); completeNavigation(location, { @@ -1506,7 +1533,8 @@ export function createRouter(init: RouterInit): Router { if (fetchControllers.has(key)) abortFetcher(key); - let matches = matchRoutes(dataRoutes, href, init.basename); + let routesToUse = inFlightDataRoutes || dataRoutes; + let matches = matchRoutes(routesToUse, href, init.basename); if (!matches) { setFetcherError( key, @@ -1629,9 +1657,10 @@ export function createRouter(init: RouterInit): Router { nextLocation, abortController.signal ); + let routesToUse = inFlightDataRoutes || dataRoutes; let matches = state.navigation.state !== "idle" - ? matchRoutes(dataRoutes, state.navigation.location, init.basename) + ? matchRoutes(routesToUse, state.navigation.location, init.basename) : state.matches; invariant(matches, "Didn't find any matches after fetcher action"); @@ -2266,6 +2295,23 @@ export function createRouter(init: RouterInit): Router { return null; } + function addRoute(newRoute: AgnosticDataRouteObject, parentId?: string) { + //TODO: implement me + } + + function updateRoute(id: string, newRoute: AgnosticDataRouteObject) { + //TODO: implement me + } + + function deleteRoute(id: string) { + //TODO: implement me + } + + function setNewRoutes(newRoutes: AgnosticDataRouteObject[]) { + inFlightDataRoutes = newRoutes; + revalidate(); + } + router = { get basename() { return init.basename; @@ -2293,6 +2339,12 @@ export function createRouter(init: RouterInit): Router { deleteBlocker, _internalFetchControllers: fetchControllers, _internalActiveDeferreds: activeDeferreds, + addRoute, + updateRoute, + deleteRoute, + // TODO: Remove setRoutes, it's temporary to avoid dealing with + // updating the tree while validating the update algorithm. + setNewRoutes, }; return router; From 985e2ce823d3a3f92c32d275156e894af1af231c Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Tue, 31 Jan 2023 16:28:50 -0500 Subject: [PATCH 02/12] wip: addRoute,updateRoute,deleteRoute --- packages/router/router.ts | 74 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 3 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 826b89bc85..09424a0d45 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -654,6 +654,18 @@ const isServer = !isBrowser; //#region createRouter //////////////////////////////////////////////////////////////////////////////// +let cloneDataRoutes = ( + routes: AgnosticDataRouteObject[] +): AgnosticDataRouteObject[] => { + return routes.map((route) => { + if (route.children === undefined) return route; + return { + ...route, + children: cloneDataRoutes(route.children), + }; + }); +}; + /** * Create a router and listen to history POP navigations */ @@ -2295,16 +2307,72 @@ export function createRouter(init: RouterInit): Router { return null; } + // TODO: visitNode/visitParent helper? function addRoute(newRoute: AgnosticDataRouteObject, parentId?: string) { - //TODO: implement me + if (inFlightDataRoutes === undefined) { + inFlightDataRoutes = cloneDataRoutes(dataRoutes); + } + + let root: AgnosticDataRouteObject = { + id: Symbol("root").toString(), + children: inFlightDataRoutes, + }; + let recurse = (node: AgnosticDataRouteObject) => { + if (node.id === parentId) { + if (node.children === undefined) { + node.children = []; + } + node.children.push(newRoute); + return; + } + node.children?.forEach(recurse); + }; + recurse(root); } function updateRoute(id: string, newRoute: AgnosticDataRouteObject) { - //TODO: implement me + if (inFlightDataRoutes === undefined) { + inFlightDataRoutes = cloneDataRoutes(dataRoutes); + } + + let root: AgnosticDataRouteObject = { + id: Symbol("root").toString(), + children: inFlightDataRoutes, + }; + let recurse = (node: AgnosticDataRouteObject) => { + if (node.children === undefined) return; + let index = node.children.findIndex((child) => child.id === id); + if (index >= 0) { + node.children[index] = newRoute; + return; + } + node.children.forEach(recurse); + }; + recurse(root); } function deleteRoute(id: string) { - //TODO: implement me + if (inFlightDataRoutes === undefined) { + inFlightDataRoutes = cloneDataRoutes(dataRoutes); + } + + let root: AgnosticDataRouteObject = { + id: Symbol("root").toString(), + children: inFlightDataRoutes, + }; + let recurse = (node: AgnosticDataRouteObject) => { + if (node.children === undefined) return; + let index = node.children.findIndex((child) => child.id === id); + if (index >= 0) { + node.children.splice(index, 1); + if (node.children.length === 0) { + node.children = undefined; + } + return; + } + node.children.forEach(recurse); + }; + recurse(root); } function setNewRoutes(newRoutes: AgnosticDataRouteObject[]) { From 7813baf3700de387bce43cd79ee4d265fd70e125 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Thu, 9 Feb 2023 14:16:20 -0500 Subject: [PATCH 03/12] refactor(hmr): internal api for updating routes and revalidating --- packages/router/router.ts | 108 +++----------------------------------- 1 file changed, 8 insertions(+), 100 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 09424a0d45..8ee0f8033f 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -214,22 +214,11 @@ export interface Router { /** * @internal * PRIVATE - DO NOT USE + * + * HMR needs to pass in-flight route updates to React Router + * TODO: Replace this with granular route update APIs (addRoute, updateRoute, deleteRoute) */ - addRoute(route: AgnosticDataRouteObject, parentId?: string): void; - /** - * @internal - * PRIVATE - DO NOT USE - */ - updateRoute(id: string, route: AgnosticDataRouteObject): void; - /** - * @internal - * PRIVATE - DO NOT USE - */ - deleteRoute(id: string): void; - /** - * TODO: DELETE THIS FOR ABOVE GRANULAR METHODS - */ - setNewRoutes(routes: AgnosticRouteObject[]): void; + _internalSetRoutesAndRevalidate(routes: AgnosticRouteObject[]): void; /** * @internal @@ -654,18 +643,6 @@ const isServer = !isBrowser; //#region createRouter //////////////////////////////////////////////////////////////////////////////// -let cloneDataRoutes = ( - routes: AgnosticDataRouteObject[] -): AgnosticDataRouteObject[] => { - return routes.map((route) => { - if (route.children === undefined) return route; - return { - ...route, - children: cloneDataRoutes(route.children), - }; - }); -}; - /** * Create a router and listen to history POP navigations */ @@ -2307,75 +2284,9 @@ export function createRouter(init: RouterInit): Router { return null; } - // TODO: visitNode/visitParent helper? - function addRoute(newRoute: AgnosticDataRouteObject, parentId?: string) { - if (inFlightDataRoutes === undefined) { - inFlightDataRoutes = cloneDataRoutes(dataRoutes); - } - - let root: AgnosticDataRouteObject = { - id: Symbol("root").toString(), - children: inFlightDataRoutes, - }; - let recurse = (node: AgnosticDataRouteObject) => { - if (node.id === parentId) { - if (node.children === undefined) { - node.children = []; - } - node.children.push(newRoute); - return; - } - node.children?.forEach(recurse); - }; - recurse(root); - } - - function updateRoute(id: string, newRoute: AgnosticDataRouteObject) { - if (inFlightDataRoutes === undefined) { - inFlightDataRoutes = cloneDataRoutes(dataRoutes); - } - - let root: AgnosticDataRouteObject = { - id: Symbol("root").toString(), - children: inFlightDataRoutes, - }; - let recurse = (node: AgnosticDataRouteObject) => { - if (node.children === undefined) return; - let index = node.children.findIndex((child) => child.id === id); - if (index >= 0) { - node.children[index] = newRoute; - return; - } - node.children.forEach(recurse); - }; - recurse(root); - } - - function deleteRoute(id: string) { - if (inFlightDataRoutes === undefined) { - inFlightDataRoutes = cloneDataRoutes(dataRoutes); - } - - let root: AgnosticDataRouteObject = { - id: Symbol("root").toString(), - children: inFlightDataRoutes, - }; - let recurse = (node: AgnosticDataRouteObject) => { - if (node.children === undefined) return; - let index = node.children.findIndex((child) => child.id === id); - if (index >= 0) { - node.children.splice(index, 1); - if (node.children.length === 0) { - node.children = undefined; - } - return; - } - node.children.forEach(recurse); - }; - recurse(root); - } - - function setNewRoutes(newRoutes: AgnosticDataRouteObject[]) { + function _internalSetRoutesAndRevalidate( + newRoutes: AgnosticDataRouteObject[] + ) { inFlightDataRoutes = newRoutes; revalidate(); } @@ -2407,12 +2318,9 @@ export function createRouter(init: RouterInit): Router { deleteBlocker, _internalFetchControllers: fetchControllers, _internalActiveDeferreds: activeDeferreds, - addRoute, - updateRoute, - deleteRoute, // TODO: Remove setRoutes, it's temporary to avoid dealing with // updating the tree while validating the update algorithm. - setNewRoutes, + _internalSetRoutesAndRevalidate, }; return router; From 9e248aaae043c95ffcbc6a31586a16d20a810e41 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Thu, 9 Feb 2023 14:29:15 -0500 Subject: [PATCH 04/12] refactor: _internalSetRoutes --- packages/react-router-dom/server.tsx | 3 +++ packages/router/router.ts | 9 +++------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index 6e9c6cd365..f8aad3bd49 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -307,6 +307,9 @@ export function createStaticRouter( }, _internalFetchControllers: new Map(), _internalActiveDeferreds: new Map(), + _internalSetRoutes() { + throw msg("_internalSetRoutesAndRevalidate"); + }, }; } diff --git a/packages/router/router.ts b/packages/router/router.ts index 8ee0f8033f..4e844a7247 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -218,7 +218,7 @@ export interface Router { * HMR needs to pass in-flight route updates to React Router * TODO: Replace this with granular route update APIs (addRoute, updateRoute, deleteRoute) */ - _internalSetRoutesAndRevalidate(routes: AgnosticRouteObject[]): void; + _internalSetRoutes(routes: AgnosticRouteObject[]): void; /** * @internal @@ -2284,11 +2284,8 @@ export function createRouter(init: RouterInit): Router { return null; } - function _internalSetRoutesAndRevalidate( - newRoutes: AgnosticDataRouteObject[] - ) { + function _internalSetRoutes(newRoutes: AgnosticDataRouteObject[]) { inFlightDataRoutes = newRoutes; - revalidate(); } router = { @@ -2320,7 +2317,7 @@ export function createRouter(init: RouterInit): Router { _internalActiveDeferreds: activeDeferreds, // TODO: Remove setRoutes, it's temporary to avoid dealing with // updating the tree while validating the update algorithm. - _internalSetRoutesAndRevalidate, + _internalSetRoutes, }; return router; From 1337089d94fc28a07c98427ff3178242acb920f1 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Fri, 10 Feb 2023 10:58:37 -0500 Subject: [PATCH 05/12] test: update method name after rename --- packages/react-router-dom/server.tsx | 2 +- packages/router/__tests__/router-test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index f8aad3bd49..cdd66b429f 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -308,7 +308,7 @@ export function createStaticRouter( _internalFetchControllers: new Map(), _internalActiveDeferreds: new Map(), _internalSetRoutes() { - throw msg("_internalSetRoutesAndRevalidate"); + throw msg("_internalSetRoutes"); }, }; } diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 527b66c5eb..8de0d44786 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -13590,7 +13590,7 @@ describe("a router", () => { ], }, ]); - t.router.setNewRoutes(newRoutes); + t.router._internalSetRoutes(newRoutes); expect(t.router.state.revalidation).toBe("loading"); expect(t.router.routes).toBe(ogRoutes); From 71ae9c5bb61f7047e519b4cbc36da9194d83361e Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Wed, 15 Feb 2023 14:44:55 -0800 Subject: [PATCH 06/12] revalidate in tests --- packages/router/__tests__/router-test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 8de0d44786..41cabc08ac 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -13591,6 +13591,7 @@ describe("a router", () => { }, ]); t.router._internalSetRoutes(newRoutes); + t.router.revalidate(); expect(t.router.state.revalidation).toBe("loading"); expect(t.router.routes).toBe(ogRoutes); From 5656983437455e79cbe6f662a1a74509ec071e6e Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Thu, 16 Feb 2023 14:51:45 -0800 Subject: [PATCH 07/12] chore: remove data for routes with no loader test: cover navigation interruption --- packages/router/__tests__/router-test.ts | 156 +++++++++++++++++++---- packages/router/router.ts | 2 +- 2 files changed, 134 insertions(+), 24 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 41cabc08ac..1175cbfe24 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -263,7 +263,6 @@ type SetupOpts = { initialEntries?: InitialEntry[]; initialIndex?: number; hydrationData?: HydrationState; - ref?: TMApiRef; }; function setup({ @@ -272,7 +271,6 @@ function setup({ initialEntries, initialIndex, hydrationData, - ref, }: SetupOpts) { let guid = 0; // Global "active" helpers, keyed by navType:guid:loaderOrAction:routeId. @@ -296,11 +294,12 @@ function setup({ // active navigation loader/action function enhanceRoutes(_routes: TestRouteObject[]) { return _routes.map((r) => { - let enhancedRoute: AgnosticRouteObject = { + let enhancedRoute: AgnosticDataRouteObject = { ...r, loader: undefined, action: undefined, children: undefined, + id: r.id || `route-${guid++}`, }; if (r.loader) { enhancedRoute.loader = (args) => { @@ -364,9 +363,6 @@ function setup({ return enhancedRoute; }); } - if (ref) { - ref.enhanceRoutes = enhanceRoutes; - } let history = createMemoryHistory({ initialEntries, initialIndex }); jest.spyOn(history, "push"); @@ -473,6 +469,12 @@ function setup({ ); } + let inFlightRoutes: AgnosticDataRouteObject[] | undefined; + function _internalSetRoutes(routes: AgnosticDataRouteObject[]) { + inFlightRoutes = routes; + currentRouter?._internalSetRoutes(routes); + } + function getNavigationHelpers( href: string, navigationId: number @@ -481,7 +483,7 @@ function setup({ currentRouter?.routes, "No currentRouter.routes available in getNavigationHelpers" ); - let matches = matchRoutes(currentRouter.routes, href); + let matches = matchRoutes(inFlightRoutes || currentRouter.routes, href); // Generate helpers for all route matches that contain loaders let loaderHelpers = getHelpers( @@ -520,7 +522,7 @@ function setup({ currentRouter?.routes, "No currentRouter.routes available in getFetcherHelpers" ); - let matches = matchRoutes(currentRouter.routes, href); + let matches = matchRoutes(inFlightRoutes || currentRouter.routes, href); invariant(currentRouter, "No currentRouter available"); let search = parsePath(href).search || ""; let hasNakedIndexQuery = new URLSearchParams(search) @@ -553,7 +555,7 @@ function setup({ if (opts?.formMethod === "post") { if (currentRouter.state.navigation?.location) { let matches = matchRoutes( - currentRouter.routes, + inFlightRoutes || currentRouter.routes, currentRouter.state.navigation.location ); invariant(matches, "No matches found for fetcher"); @@ -759,17 +761,14 @@ function setup({ fetch, revalidate, shimHelper, + enhanceRoutes, + _internalSetRoutes, }; } -type TMApiRef = { - enhanceRoutes(routes: TestRouteObject[]): AgnosticRouteObject[]; -}; - function initializeTmTest(init?: { url?: string; hydrationData?: HydrationState; - ref?: TMApiRef; }) { return setup({ routes: TM_ROUTES, @@ -777,7 +776,6 @@ function initializeTmTest(init?: { loaderData: { root: "ROOT", index: "INDEX" }, }, ...(init?.url ? { initialEntries: [init.url] } : {}), - ref: init?.ref, }); } //#endregion @@ -865,6 +863,12 @@ const TM_ROUTES: TestRouteObject[] = [ loader: true, action: true, }, + { + path: "/no-loader", + id: "no-loader", + loader: false, + action: true, + }, ], }, ]; @@ -13557,18 +13561,17 @@ describe("a router", () => { }); describe("routes updates", () => { - it("should retain existing routes until revalidation completes", async () => { - let ref = {} as TMApiRef; - let t = initializeTmTest({ ref }); + it("should retain existing routes until revalidation completes on loader removal", async () => { + let t = initializeTmTest(); let ogRoutes = t.router.routes; let A = await t.navigate("/foo"); - await A.loaders.foo.resolve(null); + await A.loaders.foo.resolve("foo"); expect(t.router.state.loaderData).toMatchObject({ root: "ROOT", - foo: null, + foo: "foo", }); - let newRoutes = ref.enhanceRoutes([ + let newRoutes = t.enhanceRoutes([ { path: "", id: "root", @@ -13590,14 +13593,118 @@ describe("a router", () => { ], }, ]); - t.router._internalSetRoutes(newRoutes); - t.router.revalidate(); + t._internalSetRoutes(newRoutes); + // Get a new revalidation helper that should use the updated routes + let R = await t.revalidate(); expect(t.router.state.revalidation).toBe("loading"); expect(t.router.routes).toBe(ogRoutes); + // Should still be og roues on new revalidation as one started by update + // has not yet completed + expect(t.router.routes).toBe(ogRoutes); + // Resolve any loaders that should have ran + await R.loaders.root.resolve("ROOT*"); + // Don't resolve "foo" because it was removed + // Revalidation should be complete + expect(t.router.state.revalidation).toBe("idle"); + // Routes should be updated + expect(t.router.routes).not.toBe(ogRoutes); + expect(t.router.routes).toBe(newRoutes); + // Loader data should be updated + expect(t.router.state.loaderData.root).toBe("ROOT*"); + expect(t.router.state.loaderData.foo).toBe(undefined); + }); + + it("should retain existing routes until revalidation completes on loader addition", async () => { + let t = initializeTmTest(); + let ogRoutes = t.router.routes; + await t.navigate("/no-loader"); + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT", + }); + + let newRoutes = t.enhanceRoutes([ + { + path: "", + id: "root", + hasErrorBoundary: true, + loader: true, + children: [ + { + path: "/no-loader", + id: "no-loader", + loader: true, + action: true, + }, + ], + }, + ]); + t._internalSetRoutes(newRoutes); // Get a new revalidation helper that should use the updated routes let R = await t.revalidate(); + + expect(t.router.state.revalidation).toBe("loading"); + expect(t.router.routes).toBe(ogRoutes); + + // Should still be og roues on new revalidation as one started by update + // has not yet completed + expect(t.router.routes).toBe(ogRoutes); + // Resolve any loaders that should have ran + await R.loaders.root.resolve("ROOT*"); + await R.loaders["no-loader"].resolve("NO_LOADER*"); + // Don't resolve "foo" because it was removed + // Revalidation should be complete + expect(t.router.state.revalidation).toBe("idle"); + // Routes should be updated + expect(t.router.routes).not.toBe(ogRoutes); + expect(t.router.routes).toBe(newRoutes); + // Loader data should be updated + expect(t.router.state.loaderData.root).toBe("ROOT*"); + expect(t.router.state.loaderData["no-loader"]).toBe("NO_LOADER*"); + }); + + it("should retain existing routes until interrupting navigation completes", async () => { + let t = initializeTmTest(); + let ogRoutes = t.router.routes; + let A = await t.navigate("/foo"); + await A.loaders.foo.resolve("foo"); + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT", + foo: "foo", + }); + + let newRoutes = t.enhanceRoutes([ + { + path: "", + id: "root", + hasErrorBoundary: true, + loader: true, + children: [ + { + path: "/", + id: "index", + loader: false, + action: true, + }, + { + path: "/foo", + id: "foo", + loader: false, + action: true, + }, + ], + }, + ]); + t._internalSetRoutes(newRoutes); + // Get a new revalidation helper that should use the updated routes + await t.revalidate(); + + let R = await t.navigate("/?revalidate"); + + expect(t.router.state.revalidation).toBe("loading"); + expect(t.router.routes).toBe(ogRoutes); + // Should still be og roues on new revalidation as one started by update // has not yet completed expect(t.router.routes).toBe(ogRoutes); @@ -13609,6 +13716,9 @@ describe("a router", () => { // Routes should be updated expect(t.router.routes).not.toBe(ogRoutes); expect(t.router.routes).toBe(newRoutes); + // Loader data should be updated + expect(t.router.state.loaderData.root).toBe("ROOT*"); + expect(t.router.state.loaderData.foo).toBe(undefined); }); }); }); diff --git a/packages/router/router.ts b/packages/router/router.ts index 4e844a7247..ec6e74b61b 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -3436,7 +3436,7 @@ function mergeLoaderData( // incoming object with an undefined value, which is how we unset a prior // loaderData if we encounter a loader error } - } else if (loaderData[id] !== undefined) { + } else if (loaderData[id] !== undefined && match.route.loader) { mergedLoaderData[id] = loaderData[id]; } From 2746f545d9fccc52490829a5e74bb73f458aec03 Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Thu, 16 Feb 2023 14:53:11 -0800 Subject: [PATCH 08/12] add changeset --- .changeset/quick-yaks-join.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/quick-yaks-join.md diff --git a/.changeset/quick-yaks-join.md b/.changeset/quick-yaks-join.md new file mode 100644 index 0000000000..4147237861 --- /dev/null +++ b/.changeset/quick-yaks-join.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Add internal API for custom HMR implementations From 492628ff322fad60df07c1ba3fb9a7cc01ef3434 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 17 Feb 2023 10:35:18 -0500 Subject: [PATCH 09/12] Add a couple more tests --- packages/router/__tests__/router-test.ts | 193 +++++++++++++++++++---- packages/router/router.ts | 2 + 2 files changed, 165 insertions(+), 30 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 1175cbfe24..d27a352a2f 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -865,7 +865,7 @@ const TM_ROUTES: TestRouteObject[] = [ }, { path: "/no-loader", - id: "no-loader", + id: "noLoader", loader: false, action: true, }, @@ -13594,26 +13594,27 @@ describe("a router", () => { }, ]); t._internalSetRoutes(newRoutes); + // Get a new revalidation helper that should use the updated routes let R = await t.revalidate(); - expect(t.router.state.revalidation).toBe("loading"); - expect(t.router.routes).toBe(ogRoutes); - // Should still be og roues on new revalidation as one started by update - // has not yet completed + // Should still expose be the og routes until revalidation completes expect(t.router.routes).toBe(ogRoutes); - // Resolve any loaders that should have ran + + // Resolve any loaders that should have ran (foo's loader has been removed) await R.loaders.root.resolve("ROOT*"); - // Don't resolve "foo" because it was removed - // Revalidation should be complete expect(t.router.state.revalidation).toBe("idle"); + // Routes should be updated expect(t.router.routes).not.toBe(ogRoutes); expect(t.router.routes).toBe(newRoutes); - // Loader data should be updated - expect(t.router.state.loaderData.root).toBe("ROOT*"); - expect(t.router.state.loaderData.foo).toBe(undefined); + + // Loader data should be updated and foo removed + expect(t.router.state.loaderData).toEqual({ + root: "ROOT*", + }); + expect(t.router.state.errors).toEqual(null); }); it("should retain existing routes until revalidation completes on loader addition", async () => { @@ -13633,7 +13634,7 @@ describe("a router", () => { children: [ { path: "/no-loader", - id: "no-loader", + id: "noLoader", loader: true, action: true, }, @@ -13643,25 +13644,27 @@ describe("a router", () => { t._internalSetRoutes(newRoutes); // Get a new revalidation helper that should use the updated routes let R = await t.revalidate(); - expect(t.router.state.revalidation).toBe("loading"); expect(t.router.routes).toBe(ogRoutes); - // Should still be og roues on new revalidation as one started by update - // has not yet completed + // Should still expose be the og routes until revalidation completes expect(t.router.routes).toBe(ogRoutes); + // Resolve any loaders that should have ran await R.loaders.root.resolve("ROOT*"); - await R.loaders["no-loader"].resolve("NO_LOADER*"); - // Don't resolve "foo" because it was removed - // Revalidation should be complete + await R.loaders.noLoader.resolve("NO_LOADER*"); expect(t.router.state.revalidation).toBe("idle"); + // Routes should be updated expect(t.router.routes).not.toBe(ogRoutes); expect(t.router.routes).toBe(newRoutes); + // Loader data should be updated - expect(t.router.state.loaderData.root).toBe("ROOT*"); - expect(t.router.state.loaderData["no-loader"]).toBe("NO_LOADER*"); + expect(t.router.state.loaderData).toEqual({ + root: "ROOT*", + noLoader: "NO_LOADER*", + }); + expect(t.router.state.errors).toEqual(null); }); it("should retain existing routes until interrupting navigation completes", async () => { @@ -13697,28 +13700,158 @@ describe("a router", () => { }, ]); t._internalSetRoutes(newRoutes); - // Get a new revalidation helper that should use the updated routes - await t.revalidate(); - let R = await t.navigate("/?revalidate"); + // Revalidate and interrupt with a navigation + let R = await t.revalidate(); + let N = await t.navigate("/?revalidate"); + + expect(t.router.state.navigation.state).toBe("loading"); + expect(t.router.state.revalidation).toBe("loading"); + + // Should still expose be the og routes until navigation completes + expect(t.router.routes).toBe(ogRoutes); + + // Revalidation cancelled so this shouldn't make it through + await R.loaders.root.resolve("ROOT STALE"); + + // Resolve any loaders that should have ran (foo's loader has been removed) + await N.loaders.root.resolve("ROOT*"); + expect(t.router.state.navigation.state).toBe("idle"); + expect(t.router.state.revalidation).toBe("idle"); + + // Routes should be updated + expect(t.router.routes).not.toBe(ogRoutes); + expect(t.router.routes).toBe(newRoutes); + + // Loader data should be updated + expect(t.router.state.loaderData).toEqual({ + root: "ROOT*", + }); + expect(t.router.state.errors).toEqual(null); + }); + + it("should retain existing routes until interrupted navigation completes", async () => { + let t = initializeTmTest(); + let ogRoutes = t.router.routes; + + let N = await t.navigate("/foo"); + + let newRoutes = t.enhanceRoutes([ + { + path: "", + id: "root", + hasErrorBoundary: true, + loader: true, + children: [ + { + path: "/", + id: "index", + loader: false, + action: true, + }, + { + path: "/foo", + id: "foo", + loader: false, + action: true, + }, + ], + }, + ]); + t._internalSetRoutes(newRoutes); + + // Interrupt /foo navigation with a revalidation + let R = await t.revalidate(); + expect(t.router.state.navigation.state).toBe("loading"); expect(t.router.state.revalidation).toBe("loading"); + + // Should still expose be the og routes until navigation completes expect(t.router.routes).toBe(ogRoutes); - // Should still be og roues on new revalidation as one started by update - // has not yet completed + // NAvigation interrupted so this shouldn't make it through + await N.loaders.root.resolve("ROOT STALE"); + + // Resolve any loaders that should have ran (foo's loader has been removed) + await R.loaders.root.resolve("ROOT*"); + expect(t.router.state.navigation.state).toBe("idle"); + expect(t.router.state.revalidation).toBe("idle"); + + // Routes should be updated + expect(t.router.routes).not.toBe(ogRoutes); + expect(t.router.routes).toBe(newRoutes); + + // Loader data should be updated + expect(t.router.state.loaderData).toEqual({ + root: "ROOT*", + }); + expect(t.router.state.errors).toEqual(null); + }); + + it("should retain existing routes until revalidation completes on loader removal (fetch)", async () => { + let t = initializeTmTest(); + let ogRoutes = t.router.routes; + + let key = "key"; + let F1 = await t.fetch("/foo", key, "root"); + await F1.loaders.foo.resolve("FOO"); + expect(t.router.state.fetchers.get("key")?.data).toBe("FOO"); + + let newRoutes = t.enhanceRoutes([ + { + path: "", + id: "root", + hasErrorBoundary: true, + loader: true, + children: [ + { + path: "/", + id: "index", + loader: false, + action: true, + }, + { + path: "/foo", + id: "foo", + loader: false, + action: true, + }, + ], + }, + ]); + t._internalSetRoutes(newRoutes); + + // Interrupt /foo navigation with a revalidation + let R = await t.revalidate(); + + expect(t.router.state.revalidation).toBe("loading"); + + // Should still expose be the og routes until navigation completes expect(t.router.routes).toBe(ogRoutes); - // Resolve any loaders that should have ran + + // Resolve any loaders that should have ran (foo's loader has been removed) await R.loaders.root.resolve("ROOT*"); - // Don't resolve "foo" because it was removed - // Revalidation should be complete expect(t.router.state.revalidation).toBe("idle"); + // Routes should be updated expect(t.router.routes).not.toBe(ogRoutes); expect(t.router.routes).toBe(newRoutes); + // Loader data should be updated - expect(t.router.state.loaderData.root).toBe("ROOT*"); - expect(t.router.state.loaderData.foo).toBe(undefined); + expect(t.router.state.loaderData).toEqual({ + root: "ROOT*", + }); + // Fetcher should have been revalidated + expect(t.router.state.fetchers.get("key")?.data).toBe(undefined); + + // FIXME: Figure out the behavior here. If we have a previous fetcher.load + // and the loader is removed - how should we handle that. Let it throw into + // the error boundary? Right now we try to call the stale loader cached + // on the FetchLoadMatch so we may need to walk through those and null + // them out? + expect(t.router.state.errors).toEqual({ + root: new Error("No helpers found for: navigation:2:loader:foo"), + }); }); }); }); diff --git a/packages/router/router.ts b/packages/router/router.ts index ec6e74b61b..c5d9f9b91a 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -3437,6 +3437,8 @@ function mergeLoaderData( // loaderData if we encounter a loader error } } else if (loaderData[id] !== undefined && match.route.loader) { + // Preserve existing keys not included in newLoaderData and where a loader + // wasn't removed by HMR mergedLoaderData[id] = loaderData[id]; } From 58bc573c3a635603425215f0bb7adc69c564c88f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 17 Feb 2023 10:51:14 -0500 Subject: [PATCH 10/12] Fix invalid useRouteLoaderData test --- packages/react-router/__tests__/data-memory-router-test.tsx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/react-router/__tests__/data-memory-router-test.tsx b/packages/react-router/__tests__/data-memory-router-test.tsx index 6ab973216d..b641de6df1 100644 --- a/packages/react-router/__tests__/data-memory-router-test.tsx +++ b/packages/react-router/__tests__/data-memory-router-test.tsx @@ -821,7 +821,6 @@ describe("", () => { initialEntries={["/foo"]} hydrationData={{ loaderData: { - layout: null, foo: "FOO", }, }} @@ -861,7 +860,7 @@ describe("", () => { } expect(spy).toHaveBeenCalledWith({ - layout: null, + layout: undefined, foo: "FOO", bar: undefined, child: undefined, @@ -896,7 +895,7 @@ describe("", () => { " `); expect(spy).toHaveBeenCalledWith({ - layout: null, + layout: undefined, foo: undefined, bar: undefined, child: "CHILD", From 6597af60845311b4992ee2ca584ba61dcf2bfa5d Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 17 Feb 2023 15:56:52 -0500 Subject: [PATCH 11/12] Handle HMR use cases for revalidating fetchers --- packages/router/__tests__/router-test.ts | 179 ++++++++++++++++++----- packages/router/router.ts | 139 ++++++++++++------ 2 files changed, 237 insertions(+), 81 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index d27a352a2f..05181d86d2 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -13789,68 +13789,181 @@ describe("a router", () => { }); it("should retain existing routes until revalidation completes on loader removal (fetch)", async () => { - let t = initializeTmTest(); - let ogRoutes = t.router.routes; + let rootDfd = createDeferred(); + let fooDfd = createDeferred(); + let ogRoutes: AgnosticDataRouteObject[] = [ + { + path: "/", + id: "root", + hasErrorBoundary: true, + loader: () => rootDfd.promise, + children: [ + { + index: true, + id: "index", + }, + { + path: "foo", + id: "foo", + loader: () => fooDfd.promise, + children: undefined, + }, + ], + }, + ]; + currentRouter = createRouter({ + routes: ogRoutes, + history: createMemoryHistory(), + hydrationData: { + loaderData: { + root: "ROOT INITIAL", + }, + }, + }); + currentRouter.initialize(); let key = "key"; - let F1 = await t.fetch("/foo", key, "root"); - await F1.loaders.foo.resolve("FOO"); - expect(t.router.state.fetchers.get("key")?.data).toBe("FOO"); + currentRouter.fetch(key, "root", "/foo"); + await fooDfd.resolve("FOO"); + expect(currentRouter.state.fetchers.get("key")?.data).toBe("FOO"); - let newRoutes = t.enhanceRoutes([ + let rootDfd2 = createDeferred(); + let newRoutes: AgnosticDataRouteObject[] = [ { - path: "", + path: "/", id: "root", hasErrorBoundary: true, - loader: true, + loader: () => rootDfd2.promise, children: [ { - path: "/", + index: true, id: "index", - loader: false, - action: true, }, { - path: "/foo", + path: "foo", id: "foo", - loader: false, - action: true, + children: undefined, }, ], }, - ]); - t._internalSetRoutes(newRoutes); + ]; + + currentRouter._internalSetRoutes(newRoutes); // Interrupt /foo navigation with a revalidation - let R = await t.revalidate(); + currentRouter.revalidate(); - expect(t.router.state.revalidation).toBe("loading"); + expect(currentRouter.state.revalidation).toBe("loading"); // Should still expose be the og routes until navigation completes - expect(t.router.routes).toBe(ogRoutes); + expect(currentRouter.routes).toEqual(ogRoutes); // Resolve any loaders that should have ran (foo's loader has been removed) - await R.loaders.root.resolve("ROOT*"); - expect(t.router.state.revalidation).toBe("idle"); + await rootDfd2.resolve("ROOT*"); + expect(currentRouter.state.revalidation).toBe("idle"); // Routes should be updated - expect(t.router.routes).not.toBe(ogRoutes); - expect(t.router.routes).toBe(newRoutes); + expect(currentRouter.routes).not.toEqual(ogRoutes); + expect(currentRouter.routes).toBe(newRoutes); // Loader data should be updated - expect(t.router.state.loaderData).toEqual({ + expect(currentRouter.state.loaderData).toEqual({ root: "ROOT*", }); - // Fetcher should have been revalidated - expect(t.router.state.fetchers.get("key")?.data).toBe(undefined); + // Fetcher should have been revalidated but thrown an errow since the + // loader was removed + expect(currentRouter.state.fetchers.get("key")?.data).toBe(undefined); + expect(currentRouter.state.errors).toEqual({ + root: new Error('Could not find the loader to run on the "foo" route'), + }); + }); - // FIXME: Figure out the behavior here. If we have a previous fetcher.load - // and the loader is removed - how should we handle that. Let it throw into - // the error boundary? Right now we try to call the stale loader cached - // on the FetchLoadMatch so we may need to walk through those and null - // them out? - expect(t.router.state.errors).toEqual({ - root: new Error("No helpers found for: navigation:2:loader:foo"), + it("should retain existing routes until revalidation completes on route removal (fetch)", async () => { + let rootDfd = createDeferred(); + let fooDfd = createDeferred(); + let ogRoutes: AgnosticDataRouteObject[] = [ + { + path: "/", + id: "root", + hasErrorBoundary: true, + loader: () => rootDfd.promise, + children: [ + { + index: true, + id: "index", + }, + { + path: "foo", + id: "foo", + loader: () => fooDfd.promise, + children: undefined, + }, + ], + }, + ]; + currentRouter = createRouter({ + routes: ogRoutes, + history: createMemoryHistory(), + hydrationData: { + loaderData: { + root: "ROOT INITIAL", + }, + }, + }); + currentRouter.initialize(); + + let key = "key"; + currentRouter.fetch(key, "root", "/foo"); + await fooDfd.resolve("FOO"); + expect(currentRouter.state.fetchers.get("key")?.data).toBe("FOO"); + + let rootDfd2 = createDeferred(); + let newRoutes: AgnosticDataRouteObject[] = [ + { + path: "/", + id: "root", + hasErrorBoundary: true, + loader: () => rootDfd2.promise, + children: [ + { + index: true, + id: "index", + }, + ], + }, + ]; + + currentRouter._internalSetRoutes(newRoutes); + + // Interrupt /foo navigation with a revalidation + currentRouter.revalidate(); + + expect(currentRouter.state.revalidation).toBe("loading"); + + // Should still expose be the og routes until navigation completes + expect(currentRouter.routes).toEqual(ogRoutes); + + // Resolve any loaders that should have ran (foo's loader has been removed) + await rootDfd2.resolve("ROOT*"); + expect(currentRouter.state.revalidation).toBe("idle"); + + // Routes should be updated + expect(currentRouter.routes).not.toEqual(ogRoutes); + expect(currentRouter.routes).toBe(newRoutes); + + // Loader data should be updated + expect(currentRouter.state.loaderData).toEqual({ + root: "ROOT*", + }); + // Fetcher should have been revalidated but theown a 404 wince the route was removed + expect(currentRouter.state.fetchers.get("key")?.data).toBe(undefined); + expect(currentRouter.state.errors).toEqual({ + root: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/foo"'), + true + ), }); }); }); diff --git a/packages/router/router.ts b/packages/router/router.ts index c5d9f9b91a..34d10d0c5c 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -565,8 +565,6 @@ interface HandleLoadersResult extends ShortCircuitable { interface FetchLoadMatch { routeId: string; path: string; - match: AgnosticDataRouteMatch; - matches: AgnosticDataRouteMatch[]; } /** @@ -574,6 +572,8 @@ interface FetchLoadMatch { */ interface RevalidatingFetcher extends FetchLoadMatch { key: string; + match: AgnosticDataRouteMatch | null; + matches: AgnosticDataRouteMatch[] | null; } /** @@ -1368,6 +1368,7 @@ export function createRouter(init: RouterInit): Router { } : undefined; + let routesToUse = inFlightDataRoutes || dataRoutes; let [matchesToLoad, revalidatingFetchers] = getMatchesToLoad( init.history, state, @@ -1377,9 +1378,11 @@ export function createRouter(init: RouterInit): Router { isRevalidationRequired, cancelledDeferredRoutes, cancelledFetcherLoads, + fetchLoadMatches, + routesToUse, + init.basename, pendingActionData, - pendingError, - fetchLoadMatches + pendingError ); // Cancel pending deferreds for no-longer-matched routes or routes we're @@ -1545,7 +1548,7 @@ export function createRouter(init: RouterInit): Router { // Store off the match so we can call it's shouldRevalidate on subsequent // revalidations - fetchLoadMatches.set(key, { routeId, path, match, matches }); + fetchLoadMatches.set(key, { routeId, path }); handleFetcherLoader(key, routeId, path, match, matches, submission); } @@ -1674,9 +1677,11 @@ export function createRouter(init: RouterInit): Router { isRevalidationRequired, cancelledDeferredRoutes, cancelledFetcherLoads, + fetchLoadMatches, + routesToUse, + init.basename, { [match.route.id]: actionResult.data }, - undefined, // No need to send through errors since we short circuit above - fetchLoadMatches + undefined // No need to send through errors since we short circuit above ); // Put all revalidating fetchers into the loading state, except for the @@ -2015,15 +2020,23 @@ export function createRouter(init: RouterInit): Router { ...matchesToLoad.map((match) => callLoaderOrAction("loader", request, match, matches, router.basename) ), - ...fetchersToLoad.map((f) => - callLoaderOrAction( - "loader", - createClientSideRequest(init.history, f.path, request.signal), - f.match, - f.matches, - router.basename - ) - ), + ...fetchersToLoad.map((f) => { + if (f.matches && f.match) { + return callLoaderOrAction( + "loader", + createClientSideRequest(init.history, f.path, request.signal), + f.match, + f.matches, + router.basename + ); + } else { + let error: ErrorResult = { + type: ResultType.error, + error: getInternalRouterError(404, { pathname: f.path }), + }; + return error; + } + }), ]); let loaderResults = results.slice(0, matchesToLoad.length); let fetcherResults = results.slice(matchesToLoad.length); @@ -2916,9 +2929,11 @@ function getMatchesToLoad( isRevalidationRequired: boolean, cancelledDeferredRoutes: string[], cancelledFetcherLoads: string[], + fetchLoadMatches: Map, + routesToUse: AgnosticDataRouteObject[], + basename: string | undefined, pendingActionData?: RouteData, - pendingError?: RouteData, - fetchLoadMatches?: Map + pendingError?: RouteData ): [AgnosticDataRouteMatch[], RevalidatingFetcher[]] { let actionResult = pendingError ? Object.values(pendingError)[0] @@ -2976,34 +2991,55 @@ function getMatchesToLoad( // Pick fetcher.loads that need to be revalidated let revalidatingFetchers: RevalidatingFetcher[] = []; - fetchLoadMatches && - fetchLoadMatches.forEach((f, key) => { - if (!matches.some((m) => m.route.id === f.routeId)) { - // This fetcher is not going to be present in the subsequent render so - // there's no need to revalidate it - return; - } else if (cancelledFetcherLoads.includes(key)) { - // This fetcher was cancelled from a prior action submission - force reload - revalidatingFetchers.push({ key, ...f }); - } else { - // Revalidating fetchers are decoupled from the route matches since they - // hit a static href, so they _always_ check shouldRevalidate and the - // default is strictly if a revalidation is explicitly required (action - // submissions, useRevalidator, X-Remix-Revalidate). - let shouldRevalidate = shouldRevalidateLoader(f.match, { - currentUrl, - currentParams: state.matches[state.matches.length - 1].params, - nextUrl, - nextParams: matches[matches.length - 1].params, - ...submission, - actionResult, - defaultShouldRevalidate, - }); - if (shouldRevalidate) { - revalidatingFetchers.push({ key, ...f }); - } - } + fetchLoadMatches.forEach((f, key) => { + // Don't revalidate if fetcher won't be present in the subsequent render + if (!matches.some((m) => m.route.id === f.routeId)) { + return; + } + + let fetcherMatches = matchRoutes(routesToUse, f.path, basename); + + // If the fetcher path no longer matches, push it in with null matches so + // we can trigger a 404 in callLoadersAndMaybeResolveData + if (!fetcherMatches) { + revalidatingFetchers.push({ key, ...f, matches: null, match: null }); + return; + } + + let fetcherMatch = getTargetMatch(fetcherMatches, f.path); + + if (cancelledFetcherLoads.includes(key)) { + revalidatingFetchers.push({ + key, + matches: fetcherMatches, + match: fetcherMatch, + ...f, + }); + return; + } + + // Revalidating fetchers are decoupled from the route matches since they + // hit a static href, so they _always_ check shouldRevalidate and the + // default is strictly if a revalidation is explicitly required (action + // submissions, useRevalidator, X-Remix-Revalidate). + let shouldRevalidate = shouldRevalidateLoader(fetcherMatch, { + currentUrl, + currentParams: state.matches[state.matches.length - 1].params, + nextUrl, + nextParams: matches[matches.length - 1].params, + ...submission, + actionResult, + defaultShouldRevalidate, }); + if (shouldRevalidate) { + revalidatingFetchers.push({ + key, + matches: fetcherMatches, + match: fetcherMatch, + ...f, + }); + } + }); return [navigationMatches, revalidatingFetchers]; } @@ -3386,7 +3422,7 @@ function processLoaderData( // Process fetcher non-redirect errors if (isErrorResult(result)) { - let boundaryMatch = findNearestBoundary(state.matches, match.route.id); + let boundaryMatch = findNearestBoundary(state.matches, match?.route.id); if (!(errors && errors[boundaryMatch.route.id])) { errors = { ...errors, @@ -3612,7 +3648,7 @@ function isMutationMethod(method?: string): method is MutationFormMethod { async function resolveDeferredResults( currentMatches: AgnosticDataRouteMatch[], - matchesToLoad: AgnosticDataRouteMatch[], + matchesToLoad: (AgnosticDataRouteMatch | null)[], results: DataResult[], signal: AbortSignal, isFetcher: boolean, @@ -3621,8 +3657,15 @@ async function resolveDeferredResults( for (let index = 0; index < results.length; index++) { let result = results[index]; let match = matchesToLoad[index]; + // If we don't have a match, then we can have a deferred result to do + // anything with. This is for revalidating fetchers where the route was + // removed during HMR + if (!match) { + continue; + } + let currentMatch = currentMatches.find( - (m) => m.route.id === match.route.id + (m) => m.route.id === match!.route.id ); let isRevalidatingLoader = currentMatch != null && From cf9637ced4fd747ad4dca8bcd4d4443a721e6447 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 17 Feb 2023 16:00:13 -0500 Subject: [PATCH 12/12] Bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index d2cf1bd74a..607528bd9b 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "41.5 kB" + "none": "41.6 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13 kB"