From 3e947b0e41528babcf02b8adfb4f113e7e051976 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 20 Jan 2023 15:25:06 -0500 Subject: [PATCH 01/30] Test harness updates --- packages/router/__tests__/router-test.ts | 60 ++++++++++++++++++------ 1 file changed, 46 insertions(+), 14 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index e9b19cfc88..18719e24ea 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -24,6 +24,7 @@ import { matchRoutes, redirect, parsePath, + UNSAFE_convertRoutesToDataRoutes, } from "../index"; // Private API @@ -262,7 +263,7 @@ type SetupOpts = { basename?: string; initialEntries?: InitialEntry[]; initialIndex?: number; - hydrationData?: HydrationState; + hydrationData?: HydrationState | null; }; function setup({ @@ -363,13 +364,39 @@ function setup({ }); } + let testRoutes = enhanceRoutes(routes); let history = createMemoryHistory({ initialEntries, initialIndex }); jest.spyOn(history, "push"); jest.spyOn(history, "replace"); + + // If the test didn't provide hydrationData for it's initial location - be a + // friendly test harness and tick something in there to avoid async fetches + // kicking off in our test. Tests can opt out to test automatic initialization + // by providing null + if (typeof hydrationData === "undefined") { + let dataRoutes = UNSAFE_convertRoutesToDataRoutes(testRoutes); + let matches = matchRoutes( + dataRoutes, + initialEntries?.[initialIndex || 0] || "/" + ); + hydrationData = { + loaderData: matches + ?.filter((m) => m.route.loader) + .reduce( + (acc, match) => + Object.assign(acc, { + [match.route.id]: + match.route.id.toUpperCase() + " INITIAL LOADER DATA", + }), + {} + ), + }; + } + currentRouter = createRouter({ basename, history, - routes: enhanceRoutes(routes), + routes: testRoutes, hydrationData, }).initialize(); @@ -761,11 +788,14 @@ function initializeTmTest(init?: { url?: string; hydrationData?: HydrationState; }) { + let hydrationData: HydrationState | undefined = init?.hydrationData + ? init.hydrationData + : init?.url != null + ? undefined + : { loaderData: { root: "ROOT", index: "INDEX" } }; return setup({ routes: TM_ROUTES, - hydrationData: init?.hydrationData || { - loaderData: { root: "ROOT", index: "INDEX" }, - }, + hydrationData, ...(init?.url ? { initialEntries: [init.url] } : {}), }); } @@ -1451,6 +1481,7 @@ describe("a router", () => { }); let B = await A.loaders.bar.redirect("/baz"); + expect(t.router.state.errors).toBe(null); expect(t.router.state.navigation.state).toBe("loading"); expect(t.router.state.navigation.location?.pathname).toBe("/baz"); expect(t.router.state.loaderData).toMatchObject({ @@ -2576,10 +2607,9 @@ describe("a router", () => { ], }); let nav = await t.navigate("/child"); - await nav.loaders.parent.resolve("PARENT"); await nav.loaders.child.resolve("CHILD"); expect(t.router.state.loaderData).toEqual({ - parent: "PARENT", + parent: "PARENT INITIAL LOADER DATA", child: "CHILD", }); expect(t.router.state.errors).toEqual(null); @@ -3358,7 +3388,9 @@ describe("a router", () => { expect(t.router.state.actionData).toEqual({ index: { error: "invalid" }, }); - expect(t.router.state.loaderData).toEqual({}); + expect(t.router.state.loaderData).toEqual({ + index: "INDEX INITIAL LOADER DATA", + }); await C.loaders.index.resolve("NEW"); @@ -3820,11 +3852,10 @@ describe("a router", () => { ], }); let nav = await t.navigate("/child"); - await nav.loaders.parent.resolve("PARENT"); await nav.loaders.child.resolve("CHILD"); expect(t.router.state.actionData).toEqual(null); expect(t.router.state.loaderData).toEqual({ - parent: "PARENT", + parent: "PARENT INITIAL LOADER DATA", child: "CHILD", }); expect(t.router.state.errors).toEqual(null); @@ -3836,7 +3867,7 @@ describe("a router", () => { await nav2.actions.child.reject(new Error("Kaboom!")); expect(t.router.state.actionData).toEqual(null); expect(t.router.state.loaderData).toEqual({ - parent: "PARENT", + parent: "PARENT INITIAL LOADER DATA", }); expect(t.router.state.errors).toEqual({ parent: new Error("Kaboom!"), @@ -6476,6 +6507,7 @@ describe("a router", () => { let t = setup({ routes: SCROLL_ROUTES, initialEntries: ["/no-loader"], + hydrationData: null, }); expect(t.router.state.restoreScrollPosition).toBe(null); @@ -8505,7 +8537,7 @@ describe("a router", () => { await A.loaders.root.resolve("A ROOT LOADER"); await A.loaders.foo.resolve("A LOADER"); - expect(t.router.state.loaderData.foo).toBeUndefined(); + expect(t.router.state.loaderData.foo).toBe("FOO INITIAL LOADER DATA"); let C = await t.fetch("/foo", key, { formMethod: "post", @@ -8521,7 +8553,7 @@ describe("a router", () => { await B.loaders.root.resolve("B ROOT LOADER"); await B.loaders.foo.resolve("B LOADER"); - expect(t.router.state.loaderData.foo).toBeUndefined(); + expect(t.router.state.loaderData.foo).toBe("FOO INITIAL LOADER DATA"); await C.loaders.root.resolve("C ROOT LOADER"); await C.loaders.foo.resolve("C LOADER"); @@ -8561,7 +8593,7 @@ describe("a router", () => { await Ak1.loaders.root.resolve("A ROOT LOADER"); await Ak1.loaders.foo.resolve("A LOADER"); - expect(t.router.state.loaderData.foo).toBeUndefined(); + expect(t.router.state.loaderData.foo).toBe("FOO INITIAL LOADER DATA"); await Bk2.loaders.root.resolve("B ROOT LOADER"); await Bk2.loaders.foo.resolve("B LOADER"); From f184d4827836fdf8e87d8f95bbb257672901be6c Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 20 Jan 2023 17:25:59 -0500 Subject: [PATCH 02/30] Initial WIP implementation of beforeRequest --- packages/router/__tests__/router-test.ts | 461 +++++++++++++++++++++++ packages/router/router.ts | 204 +++++++++- packages/router/utils.ts | 27 ++ 3 files changed, 683 insertions(+), 9 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 18719e24ea..e0617cc12f 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -11480,6 +11480,467 @@ describe("a router", () => { }); }); + describe("beforeRequest", () => { + it("runs beforeRequest sequentially before loaders", async () => { + let calls: string[] = []; + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + async beforeRequest() { + calls.push("beforeRequest start - root"); + await tick(); + calls.push("beforeRequest end - root"); + }, + async loader() { + calls.push("loader start - root"); + await tick(); + calls.push("loader end - root"); + return null; + }, + children: [ + { + id: "child", + path: "child", + async beforeRequest() { + calls.push("beforeRequest start - child"); + await tick(); + calls.push("beforeRequest end - child"); + }, + async loader() { + calls.push("loader start - child"); + await tick(); + calls.push("loader end - child"); + return null; + }, + children: [ + { + id: "grandchild", + path: "grandchild", + async beforeRequest() { + calls.push("beforeRequest start - grandchild"); + await tick(); + calls.push("beforeRequest end - grandchild"); + }, + async loader() { + calls.push("loader start - grandchild"); + await tick(); + calls.push("loader end - grandchild"); + return null; + }, + }, + ], + }, + ], + }, + ], + history: createMemoryHistory({ initialEntries: ["/"] }), + hydrationData: { loaderData: { root: "ROOT" } }, + }).initialize(); + + // query param to force root loader + await currentRouter.navigate("/child/grandchild?key=value"); + + expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); + expect(calls).toMatchInlineSnapshot(` + [ + "beforeRequest start - root", + "beforeRequest end - root", + "beforeRequest start - child", + "beforeRequest end - child", + "beforeRequest start - grandchild", + "beforeRequest end - grandchild", + "loader start - root", + "loader start - child", + "loader start - grandchild", + "loader end - root", + "loader end - child", + "loader end - grandchild", + ] + `); + }); + + it("passes context into loaders", async () => { + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + async beforeRequest({ context }) { + let count = 1; + context.set("root", count); + }, + async loader({ context }) { + return context.get("root"); + }, + children: [ + { + id: "child", + path: "child", + async beforeRequest({ context }) { + let count = (context.get("root") as number) + 1; + context.set("child", count); + }, + async loader({ context }) { + return context.get("child"); + }, + children: [ + { + id: "grandchild", + path: "grandchild", + async beforeRequest({ context }) { + let count = (context.get("child") as number) + 1; + context.set("grandchild", count); + }, + async loader({ context }) { + return context.get("grandchild"); + }, + }, + ], + }, + ], + }, + ], + history: createMemoryHistory({ initialEntries: ["/"] }), + hydrationData: { loaderData: { root: "ROOT" } }, + }).initialize(); + + // query param to force root loader + await currentRouter.navigate("/child/grandchild?key=value"); + + expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); + expect(currentRouter.state.loaderData).toEqual({ + root: 1, + child: 2, + grandchild: 3, + }); + }); + + it("runs beforeRequest sequentially before an action", async () => { + let calls: string[] = []; + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + async beforeRequest() { + calls.push("beforeRequest start - root"); + await tick(); + calls.push("beforeRequest end - root"); + }, + async loader() { + calls.push("loader start - root"); + await tick(); + calls.push("loader end - root"); + return null; + }, + children: [ + { + id: "child", + path: "child", + async beforeRequest() { + calls.push("beforeRequest start - child"); + await tick(); + calls.push("beforeRequest end - child"); + }, + async loader() { + calls.push("loader start - child"); + await tick(); + calls.push("loader end - child"); + return null; + }, + children: [ + { + id: "grandchild", + path: "grandchild", + async beforeRequest() { + calls.push("beforeRequest start - grandchild"); + await tick(); + calls.push("beforeRequest end - grandchild"); + }, + async action() { + calls.push("action start - grandchild"); + await tick(); + calls.push("action end - grandchild"); + return null; + }, + async loader() { + calls.push("loader start - grandchild"); + await tick(); + calls.push("loader end - grandchild"); + return null; + }, + }, + ], + }, + ], + }, + ], + history: createMemoryHistory({ initialEntries: ["/"] }), + hydrationData: { loaderData: { root: "ROOT" } }, + }).initialize(); + + await currentRouter.navigate("/child/grandchild", { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + + expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); + expect(calls).toMatchInlineSnapshot(` + [ + "beforeRequest start - root", + "beforeRequest end - root", + "beforeRequest start - child", + "beforeRequest end - child", + "beforeRequest start - grandchild", + "beforeRequest end - grandchild", + "action start - grandchild", + "action end - grandchild", + "beforeRequest start - root", + "beforeRequest end - root", + "beforeRequest start - child", + "beforeRequest end - child", + "beforeRequest start - grandchild", + "beforeRequest end - grandchild", + "loader start - root", + "loader start - child", + "loader start - grandchild", + "loader end - root", + "loader end - child", + "loader end - grandchild", + ] + `); + }); + + it("passes separate contexts into action and revalidating loaders", async () => { + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + async beforeRequest({ type, context }) { + let count = type === "action" ? 11 : 1; + context.set(`root-${type}`, count); + }, + async loader({ context }) { + return context.get(`root-loader`); + }, + children: [ + { + id: "child", + path: "child", + async beforeRequest({ type, context }) { + let count = (context.get(`root-${type}`) as number) + 1; + context.set(`child-${type}`, count); + }, + async loader({ context }) { + return context.get("child-loader"); + }, + children: [ + { + id: "grandchild", + path: "grandchild", + async beforeRequest({ type, context }) { + let count = (context.get(`child-${type}`) as number) + 1; + context.set(`grandchild-${type}`, count); + }, + async action({ context }) { + return context.get(`grandchild-action`); + }, + async loader({ context }) { + return context.get("grandchild-loader"); + }, + }, + ], + }, + ], + }, + ], + history: createMemoryHistory({ initialEntries: ["/"] }), + hydrationData: { loaderData: { root: "ROOT" } }, + }).initialize(); + + await currentRouter.navigate("/child/grandchild", { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + + expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); + expect(currentRouter.state.actionData).toEqual({ + grandchild: 13, + }); + expect(currentRouter.state.loaderData).toEqual({ + root: 1, + child: 2, + grandchild: 3, + }); + }); + + // TODO: + it.todo("runs beforeRequest sequentially before fetcher.load loader"); + it.todo("passes context into fetcher.load loader"); + it.todo("runs beforeRequest sequentially before fetcher.submit action"); + it.todo( + "passes separate contexts into fetcher.submit action and revalidating loaders" + ); + + it("runs beforeRequests with maximum parallelization with with revalidating fetchers", async () => { + let calls: string[] = []; + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + async beforeRequest() { + calls.push("beforeRequest start - root"); + await tick(); + calls.push("beforeRequest end - root"); + }, + async loader() { + calls.push("loader start - root"); + await tick(); + calls.push("loader end - root"); + return null; + }, + children: [ + { + id: "child", + path: "child", + async beforeRequest() { + calls.push("beforeRequest start - child"); + await tick(); + calls.push("beforeRequest end - child"); + }, + async loader() { + calls.push("loader start - child"); + await tick(); + calls.push("loader end - child"); + return null; + }, + children: [ + { + id: "grandchild", + path: "grandchild", + async beforeRequest() { + calls.push("beforeRequest start - grandchild"); + await tick(); + calls.push("beforeRequest end - grandchild"); + }, + async loader() { + calls.push("loader start - grandchild"); + await tick(); + calls.push("loader end - grandchild"); + return null; + }, + }, + { + id: "fetch2", + path: "fetch2", + async beforeRequest() { + calls.push("beforeRequest start - fetch2"); + await tick(); + calls.push("beforeRequest end - fetch2"); + }, + async loader() { + calls.push("loader start - fetch2"); + await tick(); + calls.push("loader end - fetch2"); + return null; + }, + }, + ], + }, + { + id: "fetch1", + path: "fetch1", + async beforeRequest() { + calls.push("beforeRequest start - fetch1"); + await tick(); + calls.push("beforeRequest end - fetch1"); + }, + async loader() { + calls.push("loader start - fetch1"); + await tick(); + calls.push("loader end - fetch1"); + return null; + }, + }, + ], + }, + ], + history: createMemoryHistory({ initialEntries: ["/"] }), + hydrationData: { loaderData: { root: "ROOT" } }, + }).initialize(); + + await currentRouter.fetch("1", "root", "/fetch1"); + await tick(); + await tick(); + await tick(); + expect(calls).toMatchInlineSnapshot(` + [ + "beforeRequest start - root", + "beforeRequest end - root", + "beforeRequest start - fetch1", + "beforeRequest end - fetch1", + "loader start - fetch1", + "loader end - fetch1", + ] + `); + + calls = []; + await currentRouter.fetch("2", "root", "/child/fetch2"); + await tick(); + await tick(); + await tick(); + await tick(); + expect(calls).toMatchInlineSnapshot(` + [ + "beforeRequest start - root", + "beforeRequest end - root", + "beforeRequest start - child", + "beforeRequest end - child", + "beforeRequest start - fetch2", + "beforeRequest end - fetch2", + "loader start - fetch2", + "loader end - fetch2", + ] + `); + + // query param to force root loader + calls = []; + await currentRouter.navigate("/child/grandchild?key=value"); + + expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); + expect(calls).toEqual([ + // root runs sequentially + "beforeRequest start - root", + "beforeRequest end - root", + // root unblocks child/fetch1 so they run in parallel + "beforeRequest start - child", + "beforeRequest start - fetch1", + "beforeRequest end - child", + // child unblocks grandchild/fetch2 so they run in parallel + "beforeRequest start - grandchild", + "beforeRequest start - fetch2", + "beforeRequest end - fetch1", + "beforeRequest end - grandchild", + "beforeRequest end - fetch2", + // all 5 loaders run in parallel + "loader start - root", + "loader start - child", + "loader start - grandchild", + "loader start - fetch1", + "loader start - fetch2", + "loader end - root", + "loader end - child", + "loader end - grandchild", + "loader end - fetch1", + "loader end - fetch2", + ]); + }); + }); + describe("ssr", () => { const SSR_ROUTES = [ { diff --git a/packages/router/router.ts b/packages/router/router.ts index 874be611d6..1262447540 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -7,32 +7,34 @@ import { parsePath, } from "./history"; import type { - DataResult, AgnosticDataRouteMatch, AgnosticDataRouteObject, + AgnosticRouteMatch, + AgnosticRouteObject, + BeforeRequestContext, + BeforeRequestFunctionArgs, + DataResult, DeferredResult, ErrorResult, FormEncType, FormMethod, + MutationFormMethod, RedirectResult, RouteData, - AgnosticRouteObject, + ShouldRevalidateFunction, Submission, SuccessResult, - AgnosticRouteMatch, - MutationFormMethod, - ShouldRevalidateFunction, } from "./utils"; import { + convertRoutesToDataRoutes, DeferredData, ErrorResponse, - ResultType, - convertRoutesToDataRoutes, getPathContributingMatches, isRouteErrorResponse, joinPaths, matchRoutes, resolveTo, + ResultType, warning, } from "./utils"; @@ -1252,12 +1254,20 @@ export function createRouter(init: RouterInit): Router { }), }; } else { + let context: BeforeRequestContext | undefined = undefined; + if (matches.some((m) => m.route.beforeRequest)) { + context = await callBeforeRequestMethods("action", request, matches); + } + result = await callLoaderOrAction( "action", request, actionMatch, matches, - router.basename + router.basename, + false, + false, + context ); if (request.signal.aborted) { @@ -1797,6 +1807,12 @@ export function createRouter(init: RouterInit): Router { abortController.signal ); fetchControllers.set(key, abortController); + + let context: BeforeRequestContext | undefined = undefined; + if (matches.some((m) => m.route.beforeRequest)) { + context = await callBeforeRequestMethods("loader", fetchRequest, matches); + } + let result: DataResult = await callLoaderOrAction( "loader", fetchRequest, @@ -1983,6 +1999,149 @@ export function createRouter(init: RouterInit): Router { } } + /** + * beforeRequest logic + * - each route defines beforeRequest which receives a type + * - called top-down for all matches + * - called individually for action/loader pass - called twice on mutation submissions + * - sharing data via context in remix: + * - beforeRequest in action/loader HTTP request - duplicate beforeRequest + * invocations across parallel loaders + * - beforeRequest in it's own HTTP request - context data would need to be + * shared over the wire and re-sent as URL params in loader HTTP call + */ + + interface BeforeRequestTreeNode { + match: AgnosticDataRouteMatch; + children: BeforeRequestTreeNode[]; + } + + async function callBeforeRequestMethods( + type: "action" | "loader", + request: Request, + matches: AgnosticDataRouteMatch[], + fetchersToLoad?: RevalidatingFetcher[] + ) { + // Call beforeRequest sequentially for all matches + let store = new Map(); + let context: BeforeRequestContext = { + get(k) { + if (!store.has(k)) { + throw new Error(`No "${k}" key found in beforeRequest context`); + } + return store.get(k); + }, + set(k, v) { + store.set(k, v); + }, + }; + + // Create a tree of beforeRequest calls so we can call them with optimal + // parallelism. This is only useful when we have revalidating fetchers + // since they'll almost always re-use _some_ portion of the revalidating + // navigation routes. + // + // Assume a routing tree of: + // ROOT + // / \ + // A D + // / \ / \ + // B C E F + // \ + // G + // + // If we have fetcher.load("/A/B") and fetcher.load("/D/E") and we POST to + // /D/F/G, then we need to revalidate all of the following loaders: + // - navigation: ROOT, D, F, G + // - fetcher 1: ROOT, A, B + // - fetcher 2: ROOT, D, E + // + // This algorithm creates us a tree, similar to our router tree, but + // containing only the matches we need to load in this pass. That way we + // can run async chains down each path through the tree in parallel + let matchesArrays = [ + matches, + ...(fetchersToLoad || []).map((f) => f.matches), + ]; + let tree: BeforeRequestTreeNode[] = []; + + // TODO: This can be cleaned up a bit + for (let i = 0; i < matchesArrays.length; i++) { + let pointer: BeforeRequestTreeNode | undefined; + for (let j = 0; j < matchesArrays[i].length; j++) { + let match = matchesArrays[i][j]; + if (j === 0) { + pointer = tree.find((m) => m.match.route.id === match.route.id); + if (!pointer) { + pointer = { match, children: [] }; + tree.push(pointer); + } + } else { + let newPointer = pointer?.children.find( + (m) => m.match.route.id === match.route.id + ); + if (!newPointer) { + newPointer = { match, children: [] }; + pointer?.children.push(newPointer); + } + pointer = newPointer; + } + } + } + + async function runBeforeRequestOnTreeNode(node: BeforeRequestTreeNode) { + if (node.match.route.beforeRequest) { + await node.match.route.beforeRequest({ + type, + request, + params: node.match.params, + context, + }); + } + await Promise.all( + node.children.map((c) => runBeforeRequestOnTreeNode(c)) + ); + } + + // Run beforeRequest methods asynchronously down the tree paths + await Promise.all(tree.map((n) => runBeforeRequestOnTreeNode(n))); + + // FIXME: OLD NAIVE IMPLEMENTATION IN CASE I NEED IT BACK LOL + // for (let match of matches) { + // if (!match.route.beforeRequest) { + // continue; + // } + // await match.route.beforeRequest({ + // type, + // request, + // params: match.params, + // context, + // }); + // } + + // if (fetchersToLoad) { + // for (let fetcher of fetchersToLoad) { + // for (let match of fetcher.matches) { + // if (!match.route.beforeRequest) { + // continue; + // } + // if (matches.some((m) => m.route.id === match.route.id)) { + // // Skip if we already called this with our navigation matches above + // continue; + // } + // await match.route.beforeRequest({ + // type, + // request, + // params: match.params, + // context, + // }); + // } + // } + // } + + return context; + } + async function callLoadersAndMaybeResolveData( currentMatches: AgnosticDataRouteMatch[], matches: AgnosticDataRouteMatch[], @@ -1990,12 +2149,39 @@ export function createRouter(init: RouterInit): Router { fetchersToLoad: RevalidatingFetcher[], request: Request ) { + let context: BeforeRequestContext | undefined = undefined; + + // Don't await to keep up on the synchronous path if there aren't any + // beforeRequest functions to call. + // TODO: This is mostly an issue in unit tests so maybe we can toss a tick + // in the harness? + if ( + matches.some((m) => m.route.beforeRequest) || + fetchersToLoad.some((f) => f.matches.some((m) => m.route.beforeRequest)) + ) { + context = await callBeforeRequestMethods( + "loader", + request, + matches, + fetchersToLoad + ); + } + // Call all navigation loaders and revalidating fetcher loaders in parallel, // then slice off the results into separate arrays so we can handle them // accordingly let results = await Promise.all([ ...matchesToLoad.map((match) => - callLoaderOrAction("loader", request, match, matches, router.basename) + callLoaderOrAction( + "loader", + request, + match, + matches, + router.basename, + false, + false, + context + ) ), ...fetchersToLoad.map((f) => callLoaderOrAction( diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 172d52c36a..b3e434fc8a 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -90,9 +90,28 @@ export interface Submission { interface DataFunctionArgs { request: Request; params: Params; + // TODO: Do we need to go with beforeRequestContext or something for back-compat context?: any; } +/** + * Context object passed through beforeRequest functions and into action/loaders. + * + * Supports only key/value for now, eventually will be enhanced + */ +export interface BeforeRequestContext { + get(key: string): unknown; + set(key: string, value: unknown): void; +} + +/** + * Arguments passed to beforeRequest functions + */ +export interface BeforeRequestFunctionArgs extends DataFunctionArgs { + context: BeforeRequestContext; + type: "action" | "loader"; +} + /** * Arguments passed to loader functions */ @@ -103,6 +122,13 @@ export interface LoaderFunctionArgs extends DataFunctionArgs {} */ export interface ActionFunctionArgs extends DataFunctionArgs {} +/** + * Route loader function signature + */ +export interface BeforeRequestFunction { + (args: BeforeRequestFunctionArgs): Promise | void; +} + /** * Route loader function signature */ @@ -146,6 +172,7 @@ type AgnosticBaseRouteObject = { caseSensitive?: boolean; path?: string; id?: string; + beforeRequest?: BeforeRequestFunction; loader?: LoaderFunction; action?: ActionFunction; hasErrorBoundary?: boolean; From f4ee6b7de33676ebc13f3f9bc1bc4df27486d68b Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 25 Jan 2023 18:42:58 -0500 Subject: [PATCH 03/30] Switch to middleware/next API --- packages/router/__tests__/router-test.ts | 69 ++++++ packages/router/router.ts | 254 +++++++---------------- packages/router/utils.ts | 32 +-- 3 files changed, 159 insertions(+), 196 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index e0617cc12f..41c10951d6 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -11481,6 +11481,75 @@ describe("a router", () => { }); describe("beforeRequest", () => { + it.only("middleware next fn", async () => { + let calls: string[] = []; + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + async middleware({ middleware }) { + calls.push("middleware start - root"); + let res = await middleware.next(); + calls.push("middleware end - root"); + return res; + }, + children: [ + { + id: "child", + path: "child", + async middleware({ middleware }) { + calls.push("middleware start - child"); + let res = await middleware.next(); + calls.push("middleware end - child"); + return res; + }, + children: [ + { + id: "grandchild", + path: "grandchild", + async middleware({ middleware }) { + calls.push("middleware start - grandchild"); + let res = await middleware.next(); + calls.push("middleware end - grandchild"); + return res; + }, + async loader() { + calls.push("loader start - grandchild"); + calls.push("loader end - grandchild"); + return json({ test: "value" }); + }, + }, + ], + }, + ], + }, + ], + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/child/grandchild?key=value"); + + expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); + expect(currentRouter.state.loaderData).toEqual({ + grandchild: { + test: "value", + }, + }); + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - root", + "middleware start - child", + "middleware start - grandchild", + "loader start - grandchild", + "loader end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - root", + ] + `); + }); + it("runs beforeRequest sequentially before loaders", async () => { let calls: string[] = []; currentRouter = createRouter({ diff --git a/packages/router/router.ts b/packages/router/router.ts index 1262447540..105f083049 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -11,8 +11,7 @@ import type { AgnosticDataRouteObject, AgnosticRouteMatch, AgnosticRouteObject, - BeforeRequestContext, - BeforeRequestFunctionArgs, + MiddlewareContext, DataResult, DeferredResult, ErrorResult, @@ -24,6 +23,8 @@ import type { ShouldRevalidateFunction, Submission, SuccessResult, + LoaderFunction, + ActionFunction, } from "./utils"; import { convertRoutesToDataRoutes, @@ -1254,10 +1255,7 @@ export function createRouter(init: RouterInit): Router { }), }; } else { - let context: BeforeRequestContext | undefined = undefined; - if (matches.some((m) => m.route.beforeRequest)) { - context = await callBeforeRequestMethods("action", request, matches); - } + let context: MiddlewareContext | undefined = undefined; result = await callLoaderOrAction( "action", @@ -1808,11 +1806,6 @@ export function createRouter(init: RouterInit): Router { ); fetchControllers.set(key, abortController); - let context: BeforeRequestContext | undefined = undefined; - if (matches.some((m) => m.route.beforeRequest)) { - context = await callBeforeRequestMethods("loader", fetchRequest, matches); - } - let result: DataResult = await callLoaderOrAction( "loader", fetchRequest, @@ -1999,149 +1992,6 @@ export function createRouter(init: RouterInit): Router { } } - /** - * beforeRequest logic - * - each route defines beforeRequest which receives a type - * - called top-down for all matches - * - called individually for action/loader pass - called twice on mutation submissions - * - sharing data via context in remix: - * - beforeRequest in action/loader HTTP request - duplicate beforeRequest - * invocations across parallel loaders - * - beforeRequest in it's own HTTP request - context data would need to be - * shared over the wire and re-sent as URL params in loader HTTP call - */ - - interface BeforeRequestTreeNode { - match: AgnosticDataRouteMatch; - children: BeforeRequestTreeNode[]; - } - - async function callBeforeRequestMethods( - type: "action" | "loader", - request: Request, - matches: AgnosticDataRouteMatch[], - fetchersToLoad?: RevalidatingFetcher[] - ) { - // Call beforeRequest sequentially for all matches - let store = new Map(); - let context: BeforeRequestContext = { - get(k) { - if (!store.has(k)) { - throw new Error(`No "${k}" key found in beforeRequest context`); - } - return store.get(k); - }, - set(k, v) { - store.set(k, v); - }, - }; - - // Create a tree of beforeRequest calls so we can call them with optimal - // parallelism. This is only useful when we have revalidating fetchers - // since they'll almost always re-use _some_ portion of the revalidating - // navigation routes. - // - // Assume a routing tree of: - // ROOT - // / \ - // A D - // / \ / \ - // B C E F - // \ - // G - // - // If we have fetcher.load("/A/B") and fetcher.load("/D/E") and we POST to - // /D/F/G, then we need to revalidate all of the following loaders: - // - navigation: ROOT, D, F, G - // - fetcher 1: ROOT, A, B - // - fetcher 2: ROOT, D, E - // - // This algorithm creates us a tree, similar to our router tree, but - // containing only the matches we need to load in this pass. That way we - // can run async chains down each path through the tree in parallel - let matchesArrays = [ - matches, - ...(fetchersToLoad || []).map((f) => f.matches), - ]; - let tree: BeforeRequestTreeNode[] = []; - - // TODO: This can be cleaned up a bit - for (let i = 0; i < matchesArrays.length; i++) { - let pointer: BeforeRequestTreeNode | undefined; - for (let j = 0; j < matchesArrays[i].length; j++) { - let match = matchesArrays[i][j]; - if (j === 0) { - pointer = tree.find((m) => m.match.route.id === match.route.id); - if (!pointer) { - pointer = { match, children: [] }; - tree.push(pointer); - } - } else { - let newPointer = pointer?.children.find( - (m) => m.match.route.id === match.route.id - ); - if (!newPointer) { - newPointer = { match, children: [] }; - pointer?.children.push(newPointer); - } - pointer = newPointer; - } - } - } - - async function runBeforeRequestOnTreeNode(node: BeforeRequestTreeNode) { - if (node.match.route.beforeRequest) { - await node.match.route.beforeRequest({ - type, - request, - params: node.match.params, - context, - }); - } - await Promise.all( - node.children.map((c) => runBeforeRequestOnTreeNode(c)) - ); - } - - // Run beforeRequest methods asynchronously down the tree paths - await Promise.all(tree.map((n) => runBeforeRequestOnTreeNode(n))); - - // FIXME: OLD NAIVE IMPLEMENTATION IN CASE I NEED IT BACK LOL - // for (let match of matches) { - // if (!match.route.beforeRequest) { - // continue; - // } - // await match.route.beforeRequest({ - // type, - // request, - // params: match.params, - // context, - // }); - // } - - // if (fetchersToLoad) { - // for (let fetcher of fetchersToLoad) { - // for (let match of fetcher.matches) { - // if (!match.route.beforeRequest) { - // continue; - // } - // if (matches.some((m) => m.route.id === match.route.id)) { - // // Skip if we already called this with our navigation matches above - // continue; - // } - // await match.route.beforeRequest({ - // type, - // request, - // params: match.params, - // context, - // }); - // } - // } - // } - - return context; - } - async function callLoadersAndMaybeResolveData( currentMatches: AgnosticDataRouteMatch[], matches: AgnosticDataRouteMatch[], @@ -2149,39 +1999,12 @@ export function createRouter(init: RouterInit): Router { fetchersToLoad: RevalidatingFetcher[], request: Request ) { - let context: BeforeRequestContext | undefined = undefined; - - // Don't await to keep up on the synchronous path if there aren't any - // beforeRequest functions to call. - // TODO: This is mostly an issue in unit tests so maybe we can toss a tick - // in the harness? - if ( - matches.some((m) => m.route.beforeRequest) || - fetchersToLoad.some((f) => f.matches.some((m) => m.route.beforeRequest)) - ) { - context = await callBeforeRequestMethods( - "loader", - request, - matches, - fetchersToLoad - ); - } - // Call all navigation loaders and revalidating fetcher loaders in parallel, // then slice off the results into separate arrays so we can handle them // accordingly let results = await Promise.all([ ...matchesToLoad.map((match) => - callLoaderOrAction( - "loader", - request, - match, - matches, - router.basename, - false, - false, - context - ) + callLoaderOrAction("loader", request, match, matches, router.basename) ), ...fetchersToLoad.map((f) => callLoaderOrAction( @@ -3218,6 +3041,71 @@ function shouldRevalidateLoader( return arg.defaultShouldRevalidate; } +async function callRoutePipeline( + request: Request, + match: AgnosticDataRouteMatch, + matches: AgnosticDataRouteMatch[], + handler: LoaderFunction | ActionFunction +) { + let store = new Map(); + let context: MiddlewareContext = { + get(k) { + if (!store.has(k)) { + throw new Error(`No "${k}" key found in beforeRequest context`); + } + return store.get(k); + }, + set(k, v) { + store.set(k, v); + }, + next: () => {}, + }; + + return callRouteSubPipeline(request, context, match, matches, handler); +} + +function callRouteSubPipeline( + request: Request, + context: MiddlewareContext, + match: AgnosticDataRouteMatch, + matches: AgnosticDataRouteMatch[], + handler: LoaderFunction | ActionFunction +): ReturnType { + if (request.signal.aborted) { + throw new Error("Request aborted"); + } + + // We've still got matches, continue on the middleware train + if (matches.length > 0) { + let activeMatch = matches[0]; + + // The next function will "bubble" back up the middlewares + let next: MiddlewareContext["next"] = () => { + return callRouteSubPipeline( + request, + context, + match, + matches.slice(1), + handler + ); + }; + + if (!activeMatch.route.middleware) { + return next(); + } + + context.next = next; + return activeMatch.route.middleware({ + request, + params: activeMatch.params, + middleware: context, + }); + } + + // We reached the end of our middlewares, call the handler + return handler({ request, params: match.params, middleware: context }); +} + async function callLoaderOrAction( type: "loader" | "action", request: Request, @@ -3245,7 +3133,7 @@ async function callLoaderOrAction( ); result = await Promise.race([ - handler({ request, params: match.params, context: requestContext }), + callRoutePipeline(request, match, matches, handler), abortPromise, ]); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index b3e434fc8a..ed037c5dd9 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -90,27 +90,27 @@ export interface Submission { interface DataFunctionArgs { request: Request; params: Params; - // TODO: Do we need to go with beforeRequestContext or something for back-compat context?: any; + // TODO: Make this undefined if there are no middlewares? + middleware: MiddlewareContext; } /** - * Context object passed through beforeRequest functions and into action/loaders. + * Context object passed through middleware functions and into action/loaders. * * Supports only key/value for now, eventually will be enhanced */ -export interface BeforeRequestContext { +export interface MiddlewareContext { get(key: string): unknown; set(key: string, value: unknown): void; + // TODO: Make this undefined in loader/actions? + next: () => DataFunctionReturnValue; } /** - * Arguments passed to beforeRequest functions + * Arguments passed to middleware functions */ -export interface BeforeRequestFunctionArgs extends DataFunctionArgs { - context: BeforeRequestContext; - type: "action" | "loader"; -} +export interface MiddlewareFunctionArgs extends DataFunctionArgs {} /** * Arguments passed to loader functions @@ -125,22 +125,28 @@ export interface ActionFunctionArgs extends DataFunctionArgs {} /** * Route loader function signature */ -export interface BeforeRequestFunction { - (args: BeforeRequestFunctionArgs): Promise | void; +export interface MiddlewareFunction { + (args: MiddlewareFunctionArgs): Promise | void; } +type DataFunctionReturnValue = + | Promise + | Response + | Promise + | any; + /** * Route loader function signature */ export interface LoaderFunction { - (args: LoaderFunctionArgs): Promise | Response | Promise | any; + (args: LoaderFunctionArgs): DataFunctionReturnValue; } /** * Route action function signature */ export interface ActionFunction { - (args: ActionFunctionArgs): Promise | Response | Promise | any; + (args: ActionFunctionArgs): DataFunctionReturnValue; } /** @@ -172,7 +178,7 @@ type AgnosticBaseRouteObject = { caseSensitive?: boolean; path?: string; id?: string; - beforeRequest?: BeforeRequestFunction; + middleware?: MiddlewareFunction; loader?: LoaderFunction; action?: ActionFunction; hasErrorBoundary?: boolean; From fad256d31edfac047a6c5bdefe8ff655ebf5c8b5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 26 Jan 2023 12:45:12 -0500 Subject: [PATCH 04/30] Middleware enhancements and tests --- packages/router/__tests__/router-test.ts | 991 ++++++++++++----------- packages/router/router.ts | 100 ++- 2 files changed, 566 insertions(+), 525 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 41c10951d6..b6a5f91a9f 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -148,7 +148,7 @@ function createDeferred() { }; } -function createFormData(obj: Record): FormData { +function createFormData(obj: Record = {}): FormData { let formData = new FormData(); Object.entries(obj).forEach((e) => formData.append(e[0], e[1])); return formData; @@ -1399,7 +1399,7 @@ describe("a router", () => { let t = initializeTmTest(); expect(t.router.state.loaderData).toMatchObject({ root: "ROOT" }); let A = await t.navigate("/#bar", { - formData: createFormData({}), + formData: createFormData(), }); expect(A.loaders.root.stub.mock.calls.length).toBe(0); expect(t.router.state.loaderData).toMatchObject({ root: "ROOT" }); @@ -1433,7 +1433,7 @@ describe("a router", () => { // Submit while we have an active hash causing us to lose it let B = await t.navigate("/foo", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); expect(t.router.state.navigation.state).toBe("submitting"); await B.actions.foo.resolve("ACTION"); @@ -1761,7 +1761,7 @@ describe("a router", () => { ); router.navigate("/child", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await tick(); expect(rootLoader.mock.calls.length).toBe(0); @@ -2055,7 +2055,7 @@ describe("a router", () => { // defaultShouldRevalidate=true router.navigate("/child", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await tick(); expect(router.state.fetchers.get(key)).toMatchObject({ @@ -2069,7 +2069,7 @@ describe("a router", () => { nextParams: {}, nextUrl: new URL("http://localhost/child"), formAction: "/child", - formData: createFormData({}), + formData: createFormData(), formEncType: "application/x-www-form-urlencoded", formMethod: "post", defaultShouldRevalidate: true, @@ -5218,6 +5218,7 @@ describe("a router", () => { request: new Request("http://localhost/tasks", { signal: nav.loaders.tasks.stub.mock.calls[0][0].request.signal, }), + middleware: expect.any(Object), }); let nav2 = await t.navigate("/tasks/1"); @@ -5226,6 +5227,7 @@ describe("a router", () => { request: new Request("http://localhost/tasks/1", { signal: nav2.loaders.tasksId.stub.mock.calls[0][0].request.signal, }), + middleware: expect.any(Object), }); let nav3 = await t.navigate("/tasks?foo=bar#hash"); @@ -5234,6 +5236,7 @@ describe("a router", () => { request: new Request("http://localhost/tasks?foo=bar", { signal: nav3.loaders.tasks.stub.mock.calls[0][0].request.signal, }), + middleware: expect.any(Object), }); let nav4 = await t.navigate("/tasks#hash", { @@ -5244,6 +5247,7 @@ describe("a router", () => { request: new Request("http://localhost/tasks?foo=bar", { signal: nav4.loaders.tasks.stub.mock.calls[0][0].request.signal, }), + middleware: expect.any(Object), }); expect(t.router.state.navigation.formAction).toBe("/tasks"); @@ -5640,6 +5644,7 @@ describe("a router", () => { expect(nav.actions.tasks.stub).toHaveBeenCalledWith({ params: {}, request: expect.any(Request), + middleware: expect.any(Object), }); // Assert request internals, cannot do a deep comparison above since some @@ -5682,6 +5687,7 @@ describe("a router", () => { expect(nav.actions.tasks.stub).toHaveBeenCalledWith({ params: {}, request: expect.any(Request), + middleware: expect.any(Object), }); // Assert request internals, cannot do a deep comparison above since some // internals aren't the same on separate creations @@ -5851,7 +5857,7 @@ describe("a router", () => { let nav3 = await t.navigate("/path", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await nav3.actions.path.resolve(undefined); expect(t.router.state).toMatchObject({ @@ -6113,7 +6119,7 @@ describe("a router", () => { let nav1 = await t.navigate("/parent/child", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); let nav2 = await nav1.actions.child.redirectReturn( @@ -6140,7 +6146,7 @@ describe("a router", () => { let nav1 = await t.fetch("/parent/child", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); let nav2 = await nav1.actions.child.redirectReturn( @@ -6167,7 +6173,7 @@ describe("a router", () => { let nav1 = await t.navigate("/parent/child", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); let nav2 = await nav1.actions.child.redirectReturn( @@ -6193,7 +6199,7 @@ describe("a router", () => { let nav1 = await t.navigate("/parent/child", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); let nav2 = await nav1.actions.child.redirectReturn( @@ -6239,7 +6245,7 @@ describe("a router", () => { let A = await t.navigate("/parent/child", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await A.actions.child.redirectReturn(url); @@ -6272,7 +6278,7 @@ describe("a router", () => { let A = await t.navigate("/parent/child", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), replace: true, }); @@ -6289,7 +6295,7 @@ describe("a router", () => { let A = await t.navigate("/parent/child", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); let B = await A.actions.child.redirectReturn( @@ -6671,7 +6677,7 @@ describe("a router", () => { let nav1 = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); const nav2 = await nav1.actions.tasks.redirectReturn("/tasks"); await nav2.loaders.tasks.resolve("TASKS"); @@ -6760,7 +6766,7 @@ describe("a router", () => { let nav1 = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await nav1.actions.tasks.resolve("ACTION"); await nav1.loaders.tasks.resolve("TASKS"); @@ -6791,7 +6797,7 @@ describe("a router", () => { let nav1 = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); let nav2 = await nav1.actions.tasks.redirectReturn("/"); await nav2.loaders.index.resolve("INDEX_DATA2"); @@ -6822,7 +6828,7 @@ describe("a router", () => { let nav1 = await t.fetch("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); let nav2 = await nav1.actions.tasks.redirectReturn("/tasks"); await nav2.loaders.tasks.resolve("TASKS 2"); @@ -6910,7 +6916,7 @@ describe("a router", () => { let nav1 = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), preventScrollReset: true, }); await nav1.actions.tasks.resolve("ACTION"); @@ -6942,7 +6948,7 @@ describe("a router", () => { let nav1 = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), preventScrollReset: true, }); let nav2 = await nav1.actions.tasks.redirectReturn("/"); @@ -6974,7 +6980,7 @@ describe("a router", () => { let nav1 = await t.fetch("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), preventScrollReset: true, }); let nav2 = await nav1.actions.tasks.redirectReturn("/tasks"); @@ -8127,6 +8133,7 @@ describe("a router", () => { request: new Request("http://localhost/foo", { signal: A.loaders.root.stub.mock.calls[0][0].request.signal, }), + middleware: expect.any(Object), }); }); }); @@ -9160,7 +9167,7 @@ describe("a router", () => { let C = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); // Add a helper for the fetcher that will be revalidating t.shimHelper(C.loaders, "navigation", "loader", "tasksId"); @@ -9183,7 +9190,7 @@ describe("a router", () => { // If a fetcher does a submission, it unsets the revalidation aspect let D = await t.fetch("/tasks/3", key1, { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await D.actions.tasksId.resolve("TASKS 3"); await D.loaders.root.resolve("ROOT**"); @@ -9195,7 +9202,7 @@ describe("a router", () => { let E = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await E.actions.tasks.resolve("TASKS ACTION"); await E.loaders.root.resolve("ROOT***"); @@ -9224,7 +9231,7 @@ describe("a router", () => { let C = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); // Redirect the action @@ -9259,7 +9266,7 @@ describe("a router", () => { let C = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); t.shimHelper(C.loaders, "navigation", "loader", "tasksId"); @@ -9422,7 +9429,7 @@ describe("a router", () => { // Post to the current route router.navigate("/two/three", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await tick(); expect(router.state.loaderData).toMatchObject({ @@ -9481,7 +9488,7 @@ describe("a router", () => { let B = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); t.shimHelper(B.loaders, "navigation", "loader", "tasksId"); await B.actions.tasks.resolve("TASKS ACTION"); @@ -9523,7 +9530,7 @@ describe("a router", () => { // Submit a fetcher, leaves loaded fetcher untouched let C = await t.fetch("/tasks", actionKey, { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); t.shimHelper(C.loaders, "fetch", "loader", "tasksId"); expect(t.router.state.fetchers.get(key)).toMatchObject({ @@ -9585,7 +9592,7 @@ describe("a router", () => { // Navigate such that the index route will be removed let B = await t.navigate("/tasks", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); // Resolve the action @@ -9687,7 +9694,7 @@ describe("a router", () => { // shouldRevalidate should be ignored on subsequent fetch let D = await t.navigate("/action", { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); // Add a helper for the fetcher that will be revalidating t.shimHelper(D.loaders, "navigation", "loader", "fetchA"); @@ -9773,14 +9780,14 @@ describe("a router", () => { // fetcher.submit({}, { method: 'get' }) let C = await t.fetch("/parent", key, { formMethod: "get", - formData: createFormData({}), + formData: createFormData(), }); await C.loaders.parent.resolve("PARENT LOADER"); expect(t.router.getFetcher(key).data).toBe("PARENT LOADER"); let D = await t.fetch("/parent?index", key, { formMethod: "get", - formData: createFormData({}), + formData: createFormData(), }); await D.loaders.index.resolve("INDEX LOADER"); expect(t.router.getFetcher(key).data).toBe("INDEX LOADER"); @@ -9788,14 +9795,14 @@ describe("a router", () => { // fetcher.submit({}, { method: 'post' }) let E = await t.fetch("/parent", key, { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await E.actions.parent.resolve("PARENT ACTION"); expect(t.router.getFetcher(key).data).toBe("PARENT ACTION"); let F = await t.fetch("/parent?index", key, { formMethod: "post", - formData: createFormData({}), + formData: createFormData(), }); await F.actions.index.resolve("INDEX ACTION"); expect(t.router.getFetcher(key).data).toBe("INDEX ACTION"); @@ -11480,306 +11487,466 @@ describe("a router", () => { }); }); - describe("beforeRequest", () => { - it.only("middleware next fn", async () => { - let calls: string[] = []; - currentRouter = createRouter({ - routes: [ - { - id: "root", - path: "/", - async middleware({ middleware }) { - calls.push("middleware start - root"); - let res = await middleware.next(); - calls.push("middleware end - root"); - return res; + describe("middleware", () => { + describe("ordering", () => { + async function createMiddlewareRouterAndNavigate( + path: string, + opts?: RouterNavigateOptions + ) { + let calls: string[] = []; + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", }, - children: [ - { - id: "child", - path: "child", - async middleware({ middleware }) { - calls.push("middleware start - child"); - let res = await middleware.next(); - calls.push("middleware end - child"); - return res; - }, - children: [ - { - id: "grandchild", - path: "grandchild", - async middleware({ middleware }) { - calls.push("middleware start - grandchild"); - let res = await middleware.next(); - calls.push("middleware end - grandchild"); - return res; - }, - async loader() { - calls.push("loader start - grandchild"); - calls.push("loader end - grandchild"); - return json({ test: "value" }); - }, - }, - ], + { + id: "parent", + path: "/parent", + async middleware({ middleware }) { + calls.push("middleware start - parent"); + await tick(); + let res = await middleware.next(); + calls.push("middleware end - parent"); + return res; }, - ], - }, - ], - history: createMemoryHistory(), - }).initialize(); + async action() { + calls.push("action start - parent"); + await tick(); + calls.push("action end - parent"); + return "PARENT ACTION"; + }, + async loader() { + calls.push("loader start - parent"); + await tick(); + calls.push("loader end - parent"); + return "PARENT"; + }, + children: [ + { + id: "child", + path: "child", + async middleware({ middleware }) { + calls.push("middleware start - child"); + await tick(); + let res = await middleware.next(); + calls.push("middleware end - child"); + return res; + }, + async loader() { + calls.push("loader start - child"); + await tick(); + calls.push("loader end - child"); + return "CHILD"; + }, + children: [ + { + id: "grandchild", + path: "grandchild", + async middleware({ middleware }) { + calls.push("middleware start - grandchild"); + await tick(); + let res = await middleware.next(); + calls.push("middleware end - grandchild"); + return res; + }, + async action() { + calls.push("action start - grandchild"); + await tick(); + calls.push("action end - grandchild"); + return "GRANDCHILD ACTION"; + }, + async loader() { + calls.push("loader start - grandchild"); + await tick(); + calls.push("loader end - grandchild"); + return "GRANDCHILD"; + }, + }, + ], + }, + ], + }, + ], + history: createMemoryHistory(), + }).initialize(); - await currentRouter.navigate("/child/grandchild?key=value"); + await currentRouter.navigate(path, opts); + return { router: currentRouter, calls }; + } - expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); - expect(currentRouter.state.loaderData).toEqual({ - grandchild: { - test: "value", - }, + it("runs non-nested middleware before a loader", async () => { + let { router, calls } = await createMiddlewareRouterAndNavigate( + "/parent" + ); + expect(router.state.location.pathname).toBe("/parent"); + expect(router.state.loaderData).toEqual({ + parent: "PARENT", + }); + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "loader start - parent", + "loader end - parent", + "middleware end - parent", + ] + `); }); - expect(calls).toMatchInlineSnapshot(` - [ - "middleware start - root", - "middleware start - child", - "middleware start - grandchild", - "loader start - grandchild", - "loader end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - root", - ] - `); - }); - it("runs beforeRequest sequentially before loaders", async () => { - let calls: string[] = []; - currentRouter = createRouter({ - routes: [ - { - id: "root", - path: "/", - async beforeRequest() { - calls.push("beforeRequest start - root"); - await tick(); - calls.push("beforeRequest end - root"); - }, - async loader() { - calls.push("loader start - root"); - await tick(); - calls.push("loader end - root"); - return null; - }, - children: [ - { - id: "child", - path: "child", - async beforeRequest() { - calls.push("beforeRequest start - child"); - await tick(); - calls.push("beforeRequest end - child"); - }, - async loader() { - calls.push("loader start - child"); - await tick(); - calls.push("loader end - child"); - return null; - }, - children: [ - { - id: "grandchild", - path: "grandchild", - async beforeRequest() { - calls.push("beforeRequest start - grandchild"); - await tick(); - calls.push("beforeRequest end - grandchild"); - }, - async loader() { - calls.push("loader start - grandchild"); - await tick(); - calls.push("loader end - grandchild"); - return null; - }, - }, - ], - }, - ], - }, - ], - history: createMemoryHistory({ initialEntries: ["/"] }), - hydrationData: { loaderData: { root: "ROOT" } }, - }).initialize(); + it("runs non-nested middleware before an action", async () => { + let { router, calls } = await createMiddlewareRouterAndNavigate( + "/parent", + { formMethod: "post", formData: createFormData() } + ); + expect(router.state.location.pathname).toBe("/parent"); + expect(router.state.actionData).toEqual({ + parent: "PARENT ACTION", + }); + expect(router.state.loaderData).toEqual({ + parent: "PARENT", + }); + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "action start - parent", + "action end - parent", + "middleware end - parent", + "middleware start - parent", + "loader start - parent", + "loader end - parent", + "middleware end - parent", + ] + `); + }); - // query param to force root loader - await currentRouter.navigate("/child/grandchild?key=value"); + it("runs nested middleware before a loader", async () => { + let { router, calls } = await createMiddlewareRouterAndNavigate( + "/parent/child/grandchild" + ); + expect(router.state.location.pathname).toBe("/parent/child/grandchild"); + expect(router.state.loaderData).toEqual({ + parent: "PARENT", + child: "CHILD", + grandchild: "GRANDCHILD", + }); + + // 1. Middleware chains all start in parallel for each loader and run + // sequentially down the matches + // 2. Then loaders all run in parallel + // 3. When a loader ends, it triggers the bubbling back up the + // middleware chain + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "middleware start - parent", + "middleware start - parent", + "middleware start - child", + "middleware start - child", + "middleware start - child", + "middleware start - grandchild", + "middleware start - grandchild", + "middleware start - grandchild", + "loader start - parent", + "loader start - child", + "loader start - grandchild", + "loader end - parent", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "loader end - child", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "loader end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + ] + `); + }); - expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); - expect(calls).toMatchInlineSnapshot(` - [ - "beforeRequest start - root", - "beforeRequest end - root", - "beforeRequest start - child", - "beforeRequest end - child", - "beforeRequest start - grandchild", - "beforeRequest end - grandchild", - "loader start - root", - "loader start - child", - "loader start - grandchild", - "loader end - root", - "loader end - child", - "loader end - grandchild", - ] - `); - }); + it("runs nested middleware before an action", async () => { + let { router, calls } = await createMiddlewareRouterAndNavigate( + "/parent/child/grandchild", + { formMethod: "post", formData: createFormData() } + ); + expect(router.state.location.pathname).toBe("/parent/child/grandchild"); + expect(router.state.actionData).toEqual({ + grandchild: "GRANDCHILD ACTION", + }); + expect(router.state.loaderData).toEqual({ + parent: "PARENT", + child: "CHILD", + grandchild: "GRANDCHILD", + }); + + // 1. Middleware chain runs top-down for the action + // 2. Then the action runs + // 3. When the action ends, it bubbled back up the middleware chain + // 4. Middleware chains then start in parallel for each loader and run + // top-down down the matches + // 5. Then loaders run in parallel + // 6. When a loader ends, it triggers the bubbling back up the + // middleware chain + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "middleware start - child", + "middleware start - grandchild", + "action start - grandchild", + "action end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "middleware start - parent", + "middleware start - parent", + "middleware start - parent", + "middleware start - child", + "middleware start - child", + "middleware start - child", + "middleware start - grandchild", + "middleware start - grandchild", + "middleware start - grandchild", + "loader start - parent", + "loader start - child", + "loader start - grandchild", + "loader end - parent", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "loader end - child", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "loader end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + ] + `); + }); - it("passes context into loaders", async () => { - currentRouter = createRouter({ - routes: [ - { - id: "root", - path: "/", - async beforeRequest({ context }) { - let count = 1; - context.set("root", count); - }, - async loader({ context }) { - return context.get("root"); + it("does not require middlewares to call next()", async () => { + let calls: string[] = []; + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", }, - children: [ - { - id: "child", - path: "child", - async beforeRequest({ context }) { - let count = (context.get("root") as number) + 1; - context.set("child", count); - }, - async loader({ context }) { - return context.get("child"); - }, - children: [ - { - id: "grandchild", - path: "grandchild", - async beforeRequest({ context }) { - let count = (context.get("child") as number) + 1; - context.set("grandchild", count); - }, - async loader({ context }) { - return context.get("grandchild"); - }, - }, - ], + { + id: "parent", + path: "/parent", + async middleware({ middleware }) { + calls.push("middleware start - parent"); + await tick(); + calls.push("middleware end - parent"); }, - ], - }, - ], - history: createMemoryHistory({ initialEntries: ["/"] }), - hydrationData: { loaderData: { root: "ROOT" } }, - }).initialize(); + async loader() { + calls.push("loader start - parent"); + await tick(); + calls.push("loader end - parent"); + return "PARENT"; + }, + children: [ + { + id: "child", + path: "child", + async middleware() { + calls.push("middleware start - child"); + await tick(); + calls.push("middleware end - child"); + }, + async loader() { + calls.push("loader start - child"); + await tick(); + calls.push("loader end - child"); + return "CHILD"; + }, + }, + ], + }, + ], + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent/child"); + + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "middleware start - parent", + "middleware end - parent", + "middleware start - child", + "middleware end - parent", + "middleware start - child", + "middleware end - child", + "loader start - parent", + "middleware end - child", + "loader start - child", + "loader end - parent", + "loader end - child", + ] + `); + }); - // query param to force root loader - await currentRouter.navigate("/child/grandchild?key=value"); + it("throws an error if next() is twice in a middleware", async () => { + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + }, + { + id: "parent", + path: "/parent", + async middleware({ middleware }) { + await middleware.next(); + await middleware.next(); + }, + async loader({ middleware }) { + return "PARENT"; + }, + }, + ], + history: createMemoryHistory(), + }).initialize(); - expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); - expect(currentRouter.state.loaderData).toEqual({ - root: 1, - child: 2, - grandchild: 3, + await currentRouter?.navigate("/parent"); + expect(currentRouter.state.location.pathname).toBe("/parent"); + expect(currentRouter.state.errors).toEqual({ + parent: new Error( + "You may only call `next()` once per middleware and you may not call it in an action or loader" + ), + }); }); - }); - it("runs beforeRequest sequentially before an action", async () => { - let calls: string[] = []; - currentRouter = createRouter({ - routes: [ - { - id: "root", - path: "/", - async beforeRequest() { - calls.push("beforeRequest start - root"); - await tick(); - calls.push("beforeRequest end - root"); + it("throws an error if next() is called in a loader", async () => { + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", }, - async loader() { - calls.push("loader start - root"); - await tick(); - calls.push("loader end - root"); - return null; + { + id: "parent", + path: "/parent", + async middleware({ middleware }) { + return middleware.next(); + }, + async loader({ middleware }) { + await middleware.next(); + return "PARENT"; + }, }, - children: [ - { - id: "child", - path: "child", - async beforeRequest() { - calls.push("beforeRequest start - child"); - await tick(); - calls.push("beforeRequest end - child"); - }, - async loader() { - calls.push("loader start - child"); - await tick(); - calls.push("loader end - child"); - return null; - }, - children: [ - { - id: "grandchild", - path: "grandchild", - async beforeRequest() { - calls.push("beforeRequest start - grandchild"); - await tick(); - calls.push("beforeRequest end - grandchild"); - }, - async action() { - calls.push("action start - grandchild"); - await tick(); - calls.push("action end - grandchild"); - return null; - }, - async loader() { - calls.push("loader start - grandchild"); - await tick(); - calls.push("loader end - grandchild"); - return null; - }, - }, - ], + ], + history: createMemoryHistory(), + }).initialize(); + + await currentRouter?.navigate("/parent"); + expect(currentRouter.state.location.pathname).toBe("/parent"); + expect(currentRouter.state.errors).toEqual({ + parent: new Error( + "You may only call `next()` once per middleware and you may not call it in an action or loader" + ), + }); + }); + + it("throws an error if next() is called in an action", async () => { + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + }, + { + id: "parent", + path: "/parent", + async middleware({ middleware }) { + return middleware.next(); }, - ], - }, - ], - history: createMemoryHistory({ initialEntries: ["/"] }), - hydrationData: { loaderData: { root: "ROOT" } }, - }).initialize(); + async action({ middleware }) { + await middleware.next(); + return "PARENT ACTION"; + }, + async loader() { + return "PARENT"; + }, + }, + ], + history: createMemoryHistory(), + }).initialize(); - await currentRouter.navigate("/child/grandchild", { - formMethod: "post", - formData: createFormData({ key: "value" }), + await currentRouter?.navigate("/parent", { + formMethod: "post", + formData: createFormData(), + }); + expect(currentRouter.state.location.pathname).toBe("/parent"); + expect(currentRouter.state.errors).toEqual({ + parent: new Error( + "You may only call `next()` once per middleware and you may not call it in an action or loader" + ), + }); }); + }); - expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); - expect(calls).toMatchInlineSnapshot(` - [ - "beforeRequest start - root", - "beforeRequest end - root", - "beforeRequest start - child", - "beforeRequest end - child", - "beforeRequest start - grandchild", - "beforeRequest end - grandchild", - "action start - grandchild", - "action end - grandchild", - "beforeRequest start - root", - "beforeRequest end - root", - "beforeRequest start - child", - "beforeRequest end - child", - "beforeRequest start - grandchild", - "beforeRequest end - grandchild", - "loader start - root", - "loader start - child", - "loader start - grandchild", - "loader end - root", - "loader end - child", - "loader end - grandchild", - ] - `); + describe("middleware context", () => { + it("passes context into loaders", async () => { + currentRouter = createRouter({ + routes: [ + { path: "/" }, + { + id: "parent", + path: "/parent", + async middleware({ middleware }) { + let count = 1; + middleware.set("parent", count); + }, + async loader({ middleware }) { + return middleware.get("parent"); + }, + children: [ + { + id: "child", + path: "child", + async middleware({ middleware }) { + let count = (middleware.get("parent") as number) + 1; + middleware.set("child", count); + }, + async loader({ middleware }) { + return middleware.get("child"); + }, + children: [ + { + id: "grandchild", + path: "grandchild", + async middleware({ middleware }) { + let count = (middleware.get("child") as number) + 1; + middleware.set("grandchild", count); + }, + async loader({ middleware }) { + return middleware.get("grandchild"); + }, + }, + ], + }, + ], + }, + ], + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent/child/grandchild"); + + expect(currentRouter.state.location.pathname).toBe( + "/parent/child/grandchild" + ); + expect(currentRouter.state.loaderData).toEqual({ + parent: 1, + child: 2, + grandchild: 3, + }); + }); }); it("passes separate contexts into action and revalidating loaders", async () => { @@ -11788,37 +11955,42 @@ describe("a router", () => { { id: "root", path: "/", - async beforeRequest({ type, context }) { + async middleware({ request, middleware }) { + let type = request.method === "POST" ? "action" : "loader"; let count = type === "action" ? 11 : 1; - context.set(`root-${type}`, count); + middleware.set(`root-${type}`, count); }, - async loader({ context }) { - return context.get(`root-loader`); + async loader({ middleware }) { + return middleware.get(`root-loader`); }, children: [ { id: "child", path: "child", - async beforeRequest({ type, context }) { - let count = (context.get(`root-${type}`) as number) + 1; - context.set(`child-${type}`, count); + async middleware({ request, middleware }) { + let type = request.method === "POST" ? "action" : "loader"; + let count = (middleware.get(`root-${type}`) as number) + 1; + middleware.set(`child-${type}`, count); }, - async loader({ context }) { - return context.get("child-loader"); + async loader({ middleware }) { + return middleware.get("child-loader"); }, children: [ { id: "grandchild", path: "grandchild", - async beforeRequest({ type, context }) { - let count = (context.get(`child-${type}`) as number) + 1; - context.set(`grandchild-${type}`, count); + async middleware({ request, middleware }) { + let type = + request.method === "POST" ? "action" : "loader"; + let count = + (middleware.get(`child-${type}`) as number) + 1; + middleware.set(`grandchild-${type}`, count); }, - async action({ context }) { - return context.get(`grandchild-action`); + async action({ middleware }) { + return middleware.get(`grandchild-action`); }, - async loader({ context }) { - return context.get("grandchild-loader"); + async loader({ middleware }) { + return middleware.get("grandchild-loader"); }, }, ], @@ -11847,167 +12019,12 @@ describe("a router", () => { }); // TODO: - it.todo("runs beforeRequest sequentially before fetcher.load loader"); + it.todo("runs middleware sequentially before fetcher.load loader"); it.todo("passes context into fetcher.load loader"); - it.todo("runs beforeRequest sequentially before fetcher.submit action"); + it.todo("runs middleware sequentially before fetcher.submit action"); it.todo( "passes separate contexts into fetcher.submit action and revalidating loaders" ); - - it("runs beforeRequests with maximum parallelization with with revalidating fetchers", async () => { - let calls: string[] = []; - currentRouter = createRouter({ - routes: [ - { - id: "root", - path: "/", - async beforeRequest() { - calls.push("beforeRequest start - root"); - await tick(); - calls.push("beforeRequest end - root"); - }, - async loader() { - calls.push("loader start - root"); - await tick(); - calls.push("loader end - root"); - return null; - }, - children: [ - { - id: "child", - path: "child", - async beforeRequest() { - calls.push("beforeRequest start - child"); - await tick(); - calls.push("beforeRequest end - child"); - }, - async loader() { - calls.push("loader start - child"); - await tick(); - calls.push("loader end - child"); - return null; - }, - children: [ - { - id: "grandchild", - path: "grandchild", - async beforeRequest() { - calls.push("beforeRequest start - grandchild"); - await tick(); - calls.push("beforeRequest end - grandchild"); - }, - async loader() { - calls.push("loader start - grandchild"); - await tick(); - calls.push("loader end - grandchild"); - return null; - }, - }, - { - id: "fetch2", - path: "fetch2", - async beforeRequest() { - calls.push("beforeRequest start - fetch2"); - await tick(); - calls.push("beforeRequest end - fetch2"); - }, - async loader() { - calls.push("loader start - fetch2"); - await tick(); - calls.push("loader end - fetch2"); - return null; - }, - }, - ], - }, - { - id: "fetch1", - path: "fetch1", - async beforeRequest() { - calls.push("beforeRequest start - fetch1"); - await tick(); - calls.push("beforeRequest end - fetch1"); - }, - async loader() { - calls.push("loader start - fetch1"); - await tick(); - calls.push("loader end - fetch1"); - return null; - }, - }, - ], - }, - ], - history: createMemoryHistory({ initialEntries: ["/"] }), - hydrationData: { loaderData: { root: "ROOT" } }, - }).initialize(); - - await currentRouter.fetch("1", "root", "/fetch1"); - await tick(); - await tick(); - await tick(); - expect(calls).toMatchInlineSnapshot(` - [ - "beforeRequest start - root", - "beforeRequest end - root", - "beforeRequest start - fetch1", - "beforeRequest end - fetch1", - "loader start - fetch1", - "loader end - fetch1", - ] - `); - - calls = []; - await currentRouter.fetch("2", "root", "/child/fetch2"); - await tick(); - await tick(); - await tick(); - await tick(); - expect(calls).toMatchInlineSnapshot(` - [ - "beforeRequest start - root", - "beforeRequest end - root", - "beforeRequest start - child", - "beforeRequest end - child", - "beforeRequest start - fetch2", - "beforeRequest end - fetch2", - "loader start - fetch2", - "loader end - fetch2", - ] - `); - - // query param to force root loader - calls = []; - await currentRouter.navigate("/child/grandchild?key=value"); - - expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); - expect(calls).toEqual([ - // root runs sequentially - "beforeRequest start - root", - "beforeRequest end - root", - // root unblocks child/fetch1 so they run in parallel - "beforeRequest start - child", - "beforeRequest start - fetch1", - "beforeRequest end - child", - // child unblocks grandchild/fetch2 so they run in parallel - "beforeRequest start - grandchild", - "beforeRequest start - fetch2", - "beforeRequest end - fetch1", - "beforeRequest end - grandchild", - "beforeRequest end - fetch2", - // all 5 loaders run in parallel - "loader start - root", - "loader start - child", - "loader start - grandchild", - "loader start - fetch1", - "loader start - fetch2", - "loader end - root", - "loader end - child", - "loader end - grandchild", - "loader end - fetch1", - "loader end - fetch2", - ]); - }); }); describe("ssr", () => { diff --git a/packages/router/router.ts b/packages/router/router.ts index 105f083049..109867f85b 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -25,6 +25,9 @@ import type { SuccessResult, LoaderFunction, ActionFunction, + LoaderFunctionArgs, + ActionFunctionArgs, + Params, } from "./utils"; import { convertRoutesToDataRoutes, @@ -1255,17 +1258,12 @@ export function createRouter(init: RouterInit): Router { }), }; } else { - let context: MiddlewareContext | undefined = undefined; - result = await callLoaderOrAction( "action", request, actionMatch, matches, - router.basename, - false, - false, - context + router.basename ); if (request.signal.aborted) { @@ -3043,12 +3041,12 @@ function shouldRevalidateLoader( async function callRoutePipeline( request: Request, - match: AgnosticDataRouteMatch, matches: AgnosticDataRouteMatch[], + requestContext: unknown, handler: LoaderFunction | ActionFunction ) { let store = new Map(); - let context: MiddlewareContext = { + let middlewareContext: MiddlewareContext = { get(k) { if (!store.has(k)) { throw new Error(`No "${k}" key found in beforeRequest context`); @@ -3061,49 +3059,75 @@ async function callRoutePipeline( next: () => {}, }; - return callRouteSubPipeline(request, context, match, matches, handler); + return callRouteSubPipeline( + request, + matches, + matches[0].params, + requestContext, + middlewareContext, + handler + ); } -function callRouteSubPipeline( +async function callRouteSubPipeline( request: Request, - context: MiddlewareContext, - match: AgnosticDataRouteMatch, matches: AgnosticDataRouteMatch[], + params: Params, + requestContext: unknown, + middlewareContext: MiddlewareContext, handler: LoaderFunction | ActionFunction -): ReturnType { +): Promise> { if (request.signal.aborted) { throw new Error("Request aborted"); } - // We've still got matches, continue on the middleware train - if (matches.length > 0) { - let activeMatch = matches[0]; - - // The next function will "bubble" back up the middlewares - let next: MiddlewareContext["next"] = () => { - return callRouteSubPipeline( - request, - context, - match, - matches.slice(1), - handler + if (matches.length === 0) { + // We reached the end of our middlewares, call the handler + middlewareContext.next = () => { + throw new Error( + "You may only call `next()` once per middleware and you may not call " + + "it in an action or loader" ); }; - - if (!activeMatch.route.middleware) { - return next(); - } - - context.next = next; - return activeMatch.route.middleware({ + return handler({ request, - params: activeMatch.params, - middleware: context, + params, + context: requestContext, + middleware: middlewareContext, }); } - // We reached the end of our middlewares, call the handler - return handler({ request, params: match.params, middleware: context }); + // We've still got matches, continue on the middleware train. The `next()` + // function will "bubble" back up the middlewares after handlers have executed + let nextCalled = false; + let next: MiddlewareContext["next"] = () => { + nextCalled = true; + return callRouteSubPipeline( + request, + matches.slice(1), + params, + requestContext, + middlewareContext, + handler + ); + }; + + if (!matches[0].route.middleware) { + return next(); + } + + middlewareContext.next = next; + let res = await matches[0].route.middleware({ + request, + params, + middleware: middlewareContext, + }); + + if (nextCalled) { + return res; + } else { + return next(); + } } async function callLoaderOrAction( @@ -3127,13 +3151,13 @@ async function callLoaderOrAction( try { let handler = match.route[type]; - invariant( + invariant( handler, `Could not find the ${type} to run on the "${match.route.id}" route` ); result = await Promise.race([ - callRoutePipeline(request, match, matches, handler), + callRoutePipeline(request, matches, requestContext, handler), abortPromise, ]); From ae498d8d4a93b06a46956c88fc1abee9a5a52dc1 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 26 Jan 2023 15:39:53 -0500 Subject: [PATCH 05/30] middleware done, context typess needed --- packages/router/__tests__/router-test.ts | 774 +++++++++++++++++------ packages/router/router.ts | 21 +- packages/router/utils.ts | 2 - 3 files changed, 582 insertions(+), 215 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index b6a5f91a9f..5d6bcd575f 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -397,7 +397,7 @@ function setup({ basename, history, routes: testRoutes, - hydrationData, + ...(hydrationData ? { hydrationData } : {}), }).initialize(); function getRouteHelpers( @@ -455,7 +455,7 @@ function setup({ let routeHelpers: Helpers = { get signal() { - return internalHelpers._signal; + return internalHelpers._signal as AbortSignal; }, // Note: This spread has to come _after_ the above getter, otherwise // we lose the getter nature of it somewhere in the babel/typescript @@ -799,6 +799,47 @@ function initializeTmTest(init?: { ...(init?.url ? { initialEntries: [init.url] } : {}), }); } + +function createRequest(path: string, opts?: RequestInit) { + return new Request(`http://localhost${path}`, { + signal: new AbortController().signal, + ...opts, + }); +} + +function createSubmitRequest(path: string, opts?: RequestInit) { + let searchParams = new URLSearchParams(); + searchParams.append("key", "value"); + + return createRequest(path, { + method: "post", + body: searchParams, + ...opts, + }); +} + +// Wrote this, then didn't need it, but it felt useful so I left it here +// function callRouterAndWait(router: Router, cb: () => any) { +// let idleRouterPromise = new Promise((resolve, reject) => { +// let unsub = router.subscribe((state) => { +// if ( +// state.navigation.state === "idle" && +// Array.from(state.fetchers.values()).every((f) => f.state === "idle") +// ) { +// unsub(); +// resolve(null); +// } +// }); +// }); +// cb(); +// return Promise.race([ +// idleRouterPromise, +// new Promise((_, r) => +// setTimeout(() => r("callRouterAndWait Timeout"), 2000) +// ), +// ]); +// } + //#endregion /////////////////////////////////////////////////////////////////////////////// @@ -5215,7 +5256,7 @@ describe("a router", () => { let nav = await t.navigate("/tasks"); expect(nav.loaders.tasks.stub).toHaveBeenCalledWith({ params: {}, - request: new Request("http://localhost/tasks", { + request: createRequest("/tasks", { signal: nav.loaders.tasks.stub.mock.calls[0][0].request.signal, }), middleware: expect.any(Object), @@ -5224,7 +5265,7 @@ describe("a router", () => { let nav2 = await t.navigate("/tasks/1"); expect(nav2.loaders.tasksId.stub).toHaveBeenCalledWith({ params: { id: "1" }, - request: new Request("http://localhost/tasks/1", { + request: createRequest("/tasks/1", { signal: nav2.loaders.tasksId.stub.mock.calls[0][0].request.signal, }), middleware: expect.any(Object), @@ -5233,7 +5274,7 @@ describe("a router", () => { let nav3 = await t.navigate("/tasks?foo=bar#hash"); expect(nav3.loaders.tasks.stub).toHaveBeenCalledWith({ params: {}, - request: new Request("http://localhost/tasks?foo=bar", { + request: createRequest("/tasks?foo=bar", { signal: nav3.loaders.tasks.stub.mock.calls[0][0].request.signal, }), middleware: expect.any(Object), @@ -5244,7 +5285,7 @@ describe("a router", () => { }); expect(nav4.loaders.tasks.stub).toHaveBeenCalledWith({ params: {}, - request: new Request("http://localhost/tasks?foo=bar", { + request: createRequest("/tasks?foo=bar", { signal: nav4.loaders.tasks.stub.mock.calls[0][0].request.signal, }), middleware: expect.any(Object), @@ -8130,7 +8171,7 @@ describe("a router", () => { }); expect(A.loaders.root.stub).toHaveBeenCalledWith({ params: {}, - request: new Request("http://localhost/foo", { + request: createRequest("/foo", { signal: A.loaders.root.stub.mock.calls[0][0].request.signal, }), middleware: expect.any(Object), @@ -11489,98 +11530,95 @@ describe("a router", () => { describe("middleware", () => { describe("ordering", () => { - async function createMiddlewareRouterAndNavigate( - path: string, - opts?: RouterNavigateOptions - ) { - let calls: string[] = []; - currentRouter = createRouter({ - routes: [ - { - id: "root", - path: "/", - }, + let calls: string[]; + let MIDDLEWARE_ORDERING_ROUTES = [ + { + id: "root", + path: "/", + }, + { + id: "parent", + path: "/parent", + async middleware({ middleware }) { + calls.push("middleware start - parent"); + await tick(); + let res = await middleware.next(); + calls.push("middleware end - parent"); + return res; + }, + async action() { + calls.push("action start - parent"); + await tick(); + calls.push("action end - parent"); + return "PARENT ACTION"; + }, + async loader() { + calls.push("loader start - parent"); + await tick(); + calls.push("loader end - parent"); + return "PARENT"; + }, + children: [ { - id: "parent", - path: "/parent", + id: "child", + path: "child", async middleware({ middleware }) { - calls.push("middleware start - parent"); + calls.push("middleware start - child"); await tick(); let res = await middleware.next(); - calls.push("middleware end - parent"); + calls.push("middleware end - child"); return res; }, - async action() { - calls.push("action start - parent"); - await tick(); - calls.push("action end - parent"); - return "PARENT ACTION"; - }, async loader() { - calls.push("loader start - parent"); + calls.push("loader start - child"); await tick(); - calls.push("loader end - parent"); - return "PARENT"; + calls.push("loader end - child"); + return "CHILD"; }, children: [ { - id: "child", - path: "child", + id: "grandchild", + path: "grandchild", async middleware({ middleware }) { - calls.push("middleware start - child"); + calls.push("middleware start - grandchild"); await tick(); let res = await middleware.next(); - calls.push("middleware end - child"); + calls.push("middleware end - grandchild"); return res; }, + async action() { + calls.push("action start - grandchild"); + await tick(); + calls.push("action end - grandchild"); + return "GRANDCHILD ACTION"; + }, async loader() { - calls.push("loader start - child"); + calls.push("loader start - grandchild"); await tick(); - calls.push("loader end - child"); - return "CHILD"; + calls.push("loader end - grandchild"); + return "GRANDCHILD"; }, - children: [ - { - id: "grandchild", - path: "grandchild", - async middleware({ middleware }) { - calls.push("middleware start - grandchild"); - await tick(); - let res = await middleware.next(); - calls.push("middleware end - grandchild"); - return res; - }, - async action() { - calls.push("action start - grandchild"); - await tick(); - calls.push("action end - grandchild"); - return "GRANDCHILD ACTION"; - }, - async loader() { - calls.push("loader start - grandchild"); - await tick(); - calls.push("loader end - grandchild"); - return "GRANDCHILD"; - }, - }, - ], }, ], }, ], + }, + ]; + + beforeEach(() => { + calls = []; + }); + + it("runs non-nested middleware before a loader", async () => { + currentRouter = createRouter({ + routes: MIDDLEWARE_ORDERING_ROUTES, history: createMemoryHistory(), }).initialize(); - await currentRouter.navigate(path, opts); - return { router: currentRouter, calls }; - } + await currentRouter.navigate("/parent"); - it("runs non-nested middleware before a loader", async () => { - let { router, calls } = await createMiddlewareRouterAndNavigate( - "/parent" - ); - expect(router.state.location.pathname).toBe("/parent"); - expect(router.state.loaderData).toEqual({ + expect(currentRouter.state.location.pathname).toBe("/parent"); + expect(currentRouter.state.loaderData).toEqual({ parent: "PARENT", }); expect(calls).toMatchInlineSnapshot(` @@ -11594,15 +11632,21 @@ describe("a router", () => { }); it("runs non-nested middleware before an action", async () => { - let { router, calls } = await createMiddlewareRouterAndNavigate( - "/parent", - { formMethod: "post", formData: createFormData() } - ); - expect(router.state.location.pathname).toBe("/parent"); - expect(router.state.actionData).toEqual({ + currentRouter = createRouter({ + routes: MIDDLEWARE_ORDERING_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent", { + formMethod: "post", + formData: createFormData(), + }); + + expect(currentRouter.state.location.pathname).toBe("/parent"); + expect(currentRouter.state.actionData).toEqual({ parent: "PARENT ACTION", }); - expect(router.state.loaderData).toEqual({ + expect(currentRouter.state.loaderData).toEqual({ parent: "PARENT", }); expect(calls).toMatchInlineSnapshot(` @@ -11620,11 +11664,17 @@ describe("a router", () => { }); it("runs nested middleware before a loader", async () => { - let { router, calls } = await createMiddlewareRouterAndNavigate( + currentRouter = createRouter({ + routes: MIDDLEWARE_ORDERING_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent/child/grandchild"); + + expect(currentRouter.state.location.pathname).toBe( "/parent/child/grandchild" ); - expect(router.state.location.pathname).toBe("/parent/child/grandchild"); - expect(router.state.loaderData).toEqual({ + expect(currentRouter.state.loaderData).toEqual({ parent: "PARENT", child: "CHILD", grandchild: "GRANDCHILD", @@ -11666,15 +11716,23 @@ describe("a router", () => { }); it("runs nested middleware before an action", async () => { - let { router, calls } = await createMiddlewareRouterAndNavigate( - "/parent/child/grandchild", - { formMethod: "post", formData: createFormData() } + currentRouter = createRouter({ + routes: MIDDLEWARE_ORDERING_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent/child/grandchild", { + formMethod: "post", + formData: createFormData(), + }); + + expect(currentRouter.state.location.pathname).toBe( + "/parent/child/grandchild" ); - expect(router.state.location.pathname).toBe("/parent/child/grandchild"); - expect(router.state.actionData).toEqual({ + expect(currentRouter.state.actionData).toEqual({ grandchild: "GRANDCHILD ACTION", }); - expect(router.state.loaderData).toEqual({ + expect(currentRouter.state.loaderData).toEqual({ parent: "PARENT", child: "CHILD", grandchild: "GRANDCHILD", @@ -11726,8 +11784,240 @@ describe("a router", () => { `); }); + it("runs middleware before fetcher.load", async () => { + currentRouter = createRouter({ + routes: MIDDLEWARE_ORDERING_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.fetch( + "key", + "root", + "/parent/child/grandchild?from-fetcher" + ); + + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "middleware start - child", + "middleware start - grandchild", + "loader start - grandchild", + "loader end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + ] + `); + }); + + it("runs middleware before fetcher.submit", async () => { + currentRouter = createRouter({ + routes: MIDDLEWARE_ORDERING_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.fetch( + "key", + "root", + "/parent/child/grandchild?from-fetcher", + { + formMethod: "post", + formData: createFormData(), + } + ); + + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "middleware start - child", + "middleware start - grandchild", + "action start - grandchild", + "action end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + ] + `); + }); + + it("runs middleware before fetcher.submit and loader revalidations", async () => { + currentRouter = createRouter({ + routes: MIDDLEWARE_ORDERING_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent/child/grandchild"); + + // Blow away the calls from this navigation + while (calls.length) calls.pop(); + + // Now fetch submit which should call the revalidations + await currentRouter.fetch( + "key", + "root", + "/parent/child/grandchild?from-fetcher", + { + formMethod: "post", + formData: createFormData(), + } + ); + + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "middleware start - child", + "middleware start - grandchild", + "action start - grandchild", + "action end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "middleware start - parent", + "middleware start - parent", + "middleware start - parent", + "middleware start - child", + "middleware start - child", + "middleware start - child", + "middleware start - grandchild", + "middleware start - grandchild", + "middleware start - grandchild", + "loader start - parent", + "loader start - child", + "loader start - grandchild", + "loader end - parent", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "loader end - child", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "loader end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + ] + `); + }); + + it("runs middleware before fetcher revalidations", async () => { + currentRouter = createRouter({ + routes: MIDDLEWARE_ORDERING_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent"); + await currentRouter.fetch("a", "parent", "/parent?from-fetcher"); + await currentRouter.fetch("b", "parent", "/parent/child?from-fetcher"); + + // Blow away the calls from the navigation + fetches + while (calls.length) calls.pop(); + + // Now submit which should call the revalidations + await currentRouter.navigate("/parent", { + formMethod: "post", + formData: createFormData(), + }); + + await tick(); + await tick(); + await tick(); + await tick(); + await tick(); + + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "action start - parent", + "action end - parent", + "middleware end - parent", + "middleware start - parent", + "middleware start - parent", + "middleware start - parent", + "loader start - parent", + "loader start - parent", + "middleware start - child", + "loader end - parent", + "middleware end - parent", + "loader end - parent", + "middleware end - parent", + "loader start - child", + "loader end - child", + "middleware end - child", + "middleware end - parent", + ] + `); + }); + + it("runs middleware before staticHandler.query", async () => { + let { query } = createStaticHandler(MIDDLEWARE_ORDERING_ROUTES); + + let context = await query(createRequest("/parent/child/grandchild")); + + invariant( + !(context instanceof Response), + "Expected StaticHandlerContext" + ); + expect(context.loaderData).toMatchInlineSnapshot(` + { + "child": "CHILD", + "grandchild": "GRANDCHILD", + "parent": "PARENT", + } + `); + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "middleware start - parent", + "middleware start - parent", + "middleware start - child", + "middleware start - child", + "middleware start - child", + "middleware start - grandchild", + "middleware start - grandchild", + "middleware start - grandchild", + "loader start - parent", + "loader start - child", + "loader start - grandchild", + "loader end - parent", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "loader end - child", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + "loader end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + ] + `); + }); + + it("runs middleware before staticHandler.queryRoute", async () => { + let { queryRoute } = createStaticHandler(MIDDLEWARE_ORDERING_ROUTES); + + let result = await queryRoute( + createRequest("/parent/child/grandchild") + ); + + expect(result).toEqual("GRANDCHILD"); + expect(calls).toMatchInlineSnapshot(` + [ + "middleware start - parent", + "middleware start - child", + "middleware start - grandchild", + "loader start - grandchild", + "loader end - grandchild", + "middleware end - grandchild", + "middleware end - child", + "middleware end - parent", + ] + `); + }); + it("does not require middlewares to call next()", async () => { - let calls: string[] = []; currentRouter = createRouter({ routes: [ { @@ -11737,7 +12027,7 @@ describe("a router", () => { { id: "parent", path: "/parent", - async middleware({ middleware }) { + async middleware() { calls.push("middleware start - parent"); await tick(); calls.push("middleware end - parent"); @@ -11772,6 +12062,10 @@ describe("a router", () => { await currentRouter.navigate("/parent/child"); + expect(currentRouter.state.loaderData).toEqual({ + parent: "PARENT", + child: "CHILD", + }); expect(calls).toMatchInlineSnapshot(` [ "middleware start - parent", @@ -11790,7 +12084,7 @@ describe("a router", () => { `); }); - it("throws an error if next() is twice in a middleware", async () => { + it("throws an error if next() is called twice in a middleware", async () => { currentRouter = createRouter({ routes: [ { @@ -11891,48 +12185,56 @@ describe("a router", () => { }); describe("middleware context", () => { - it("passes context into loaders", async () => { - currentRouter = createRouter({ - routes: [ - { path: "/" }, + let MIDDLEWARE_CONTEXT_ROUTES = [ + { path: "/" }, + { + id: "parent", + path: "/parent", + async middleware({ request, middleware }) { + let type = request.method === "POST" ? "action" : "loader"; + let count = type === "action" ? 101 : 1; + middleware.set(`parent-${type}`, count); + }, + async loader({ middleware }) { + return middleware.get(`parent-loader`); + }, + children: [ { - id: "parent", - path: "/parent", - async middleware({ middleware }) { - let count = 1; - middleware.set("parent", count); + id: "child", + path: "child", + async middleware({ request, middleware }) { + let type = request.method === "POST" ? "action" : "loader"; + let count = (middleware.get(`parent-${type}`) as number) + 1; + middleware.set(`child-${type}`, count); }, async loader({ middleware }) { - return middleware.get("parent"); + return middleware.get("child-loader"); }, children: [ { - id: "child", - path: "child", - async middleware({ middleware }) { - let count = (middleware.get("parent") as number) + 1; - middleware.set("child", count); + id: "grandchild", + path: "grandchild", + async middleware({ request, middleware }) { + let type = request.method === "POST" ? "action" : "loader"; + let count = (middleware.get(`child-${type}`) as number) + 1; + middleware.set(`grandchild-${type}`, count); + }, + async action({ middleware }) { + return middleware.get(`grandchild-action`); }, async loader({ middleware }) { - return middleware.get("child"); + return middleware.get("grandchild-loader"); }, - children: [ - { - id: "grandchild", - path: "grandchild", - async middleware({ middleware }) { - let count = (middleware.get("child") as number) + 1; - middleware.set("grandchild", count); - }, - async loader({ middleware }) { - return middleware.get("grandchild"); - }, - }, - ], }, ], }, ], + }, + ]; + + it("passes context into loaders", async () => { + currentRouter = createRouter({ + routes: MIDDLEWARE_CONTEXT_ROUTES, history: createMemoryHistory(), }).initialize(); @@ -11947,84 +12249,160 @@ describe("a router", () => { grandchild: 3, }); }); - }); - it("passes separate contexts into action and revalidating loaders", async () => { - currentRouter = createRouter({ - routes: [ - { - id: "root", - path: "/", - async middleware({ request, middleware }) { - let type = request.method === "POST" ? "action" : "loader"; - let count = type === "action" ? 11 : 1; - middleware.set(`root-${type}`, count); - }, - async loader({ middleware }) { - return middleware.get(`root-loader`); + it("passes separate contexts into action and revalidating loaders", async () => { + currentRouter = createRouter({ + routes: MIDDLEWARE_CONTEXT_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent/child/grandchild", { + formMethod: "post", + formData: createFormData(), + }); + + expect(currentRouter.state.location.pathname).toBe( + "/parent/child/grandchild" + ); + expect(currentRouter.state.actionData).toEqual({ + grandchild: 103, + }); + expect(currentRouter.state.loaderData).toEqual({ + parent: 1, + child: 2, + grandchild: 3, + }); + }); + + it("passes context into fetcher.load loaders", async () => { + currentRouter = createRouter({ + routes: MIDDLEWARE_CONTEXT_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.fetch("key", "root", "/parent/child/grandchild"); + + expect(currentRouter.state.fetchers.get("key")).toMatchObject({ + state: "idle", + data: 3, + }); + }); + + it("passes context into fetcher.submit actions", async () => { + currentRouter = createRouter({ + routes: MIDDLEWARE_CONTEXT_ROUTES, + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.fetch("key", "root", "/parent/child/grandchild", { + formMethod: "post", + formData: createFormData(), + }); + + expect(currentRouter.state.fetchers.get("key")).toMatchObject({ + state: "idle", + data: 103, + }); + }); + + it("throws if no value is available via middleware.get()", async () => { + currentRouter = createRouter({ + routes: [ + { + path: "/", }, - children: [ - { - id: "child", - path: "child", - async middleware({ request, middleware }) { - let type = request.method === "POST" ? "action" : "loader"; - let count = (middleware.get(`root-${type}`) as number) + 1; - middleware.set(`child-${type}`, count); - }, - async loader({ middleware }) { - return middleware.get("child-loader"); - }, - children: [ - { - id: "grandchild", - path: "grandchild", - async middleware({ request, middleware }) { - let type = - request.method === "POST" ? "action" : "loader"; - let count = - (middleware.get(`child-${type}`) as number) + 1; - middleware.set(`grandchild-${type}`, count); - }, - async action({ middleware }) { - return middleware.get(`grandchild-action`); - }, - async loader({ middleware }) { - return middleware.get("grandchild-loader"); - }, - }, - ], + { + id: "broken", + path: "broken", + loader({ middleware }) { + return middleware.get("nope"); }, - ], - }, - ], - history: createMemoryHistory({ initialEntries: ["/"] }), - hydrationData: { loaderData: { root: "ROOT" } }, - }).initialize(); + }, + ], + history: createMemoryHistory(), + }).initialize(); - await currentRouter.navigate("/child/grandchild", { - formMethod: "post", - formData: createFormData({ key: "value" }), + await currentRouter.navigate("/broken"); + + expect(currentRouter.state.location.pathname).toBe("/broken"); + expect(currentRouter.state.errors).toMatchInlineSnapshot(` + { + "broken": [Error: Unable to find a value in the middleware context], + } + `); }); - expect(currentRouter.state.location.pathname).toBe("/child/grandchild"); - expect(currentRouter.state.actionData).toEqual({ - grandchild: 13, + it("throws if you try to set an undefined value in middleware.set()", async () => { + currentRouter = createRouter({ + routes: [ + { + path: "/", + }, + { + id: "broken", + path: "broken", + middleware({ middleware }) { + return middleware.set("nope", undefined); + }, + loader() { + return "DATA"; + }, + }, + ], + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/broken"); + + expect(currentRouter.state.location.pathname).toBe("/broken"); + expect(currentRouter.state.errors).toMatchInlineSnapshot(` + { + "broken": [Error: You cannot set an undefined value in the middleware context], + } + `); }); - expect(currentRouter.state.loaderData).toEqual({ - root: 1, - child: 2, - grandchild: 3, + + it("allows null/falsey values in middleware.set()", async () => { + currentRouter = createRouter({ + routes: [ + { + path: "/", + }, + { + id: "works", + path: "works", + middleware({ middleware }) { + middleware.set("a", null); + middleware.set("b", false); + middleware.set("c", ""); + }, + loader({ middleware }) { + return { + a: middleware.get("a"), + b: middleware.get("b"), + c: middleware.get("c"), + }; + }, + }, + ], + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/works"); + + expect(currentRouter.state.location.pathname).toBe("/works"); + expect(currentRouter.state.loaderData).toMatchInlineSnapshot(` + { + "works": { + "a": null, + "b": false, + "c": "", + }, + } + `); + expect(currentRouter.state.errors).toBe(null); }); }); - - // TODO: - it.todo("runs middleware sequentially before fetcher.load loader"); - it.todo("passes context into fetcher.load loader"); - it.todo("runs middleware sequentially before fetcher.submit action"); - it.todo( - "passes separate contexts into fetcher.submit action and revalidating loaders" - ); }); describe("ssr", () => { @@ -12126,24 +12504,6 @@ describe("a router", () => { "web+remix:whatever", ]; - function createRequest(path: string, opts?: RequestInit) { - return new Request(`http://localhost${path}`, { - signal: new AbortController().signal, - ...opts, - }); - } - - function createSubmitRequest(path: string, opts?: RequestInit) { - let searchParams = new URLSearchParams(); - searchParams.append("key", "value"); - - return createRequest(path, { - method: "post", - body: searchParams, - ...opts, - }); - } - describe("document requests", () => { it("should support document load navigations", async () => { let { query } = createStaticHandler(SSR_ROUTES); diff --git a/packages/router/router.ts b/packages/router/router.ts index 109867f85b..e322cba542 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -25,8 +25,6 @@ import type { SuccessResult, LoaderFunction, ActionFunction, - LoaderFunctionArgs, - ActionFunctionArgs, Params, } from "./utils"; import { @@ -1528,14 +1526,20 @@ export function createRouter(init: RouterInit): Router { pendingPreventScrollReset = (opts && opts.preventScrollReset) === true; if (submission && isMutationMethod(submission.formMethod)) { - handleFetcherAction(key, routeId, path, match, matches, submission); - return; + return handleFetcherAction( + key, + routeId, + path, + match, + matches, + submission + ); } // Store off the match so we can call it's shouldRevalidate on subsequent // revalidations fetchLoadMatches.set(key, { routeId, path, match, matches }); - handleFetcherLoader(key, routeId, path, match, matches, submission); + return handleFetcherLoader(key, routeId, path, match, matches, submission); } // Call the action for the matched fetcher.submit(), and then handle redirects, @@ -3049,11 +3053,16 @@ async function callRoutePipeline( let middlewareContext: MiddlewareContext = { get(k) { if (!store.has(k)) { - throw new Error(`No "${k}" key found in beforeRequest context`); + throw new Error("Unable to find a value in the middleware context"); } return store.get(k); }, set(k, v) { + if (typeof v === "undefined") { + throw new Error( + "You cannot set an undefined value in the middleware context" + ); + } store.set(k, v); }, next: () => {}, diff --git a/packages/router/utils.ts b/packages/router/utils.ts index ed037c5dd9..4a094156d1 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -91,7 +91,6 @@ interface DataFunctionArgs { request: Request; params: Params; context?: any; - // TODO: Make this undefined if there are no middlewares? middleware: MiddlewareContext; } @@ -103,7 +102,6 @@ interface DataFunctionArgs { export interface MiddlewareContext { get(key: string): unknown; set(key: string, value: unknown): void; - // TODO: Make this undefined in loader/actions? next: () => DataFunctionReturnValue; } From 890066f2da0b9b189915dd727cd16acc023b4e9d Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 26 Jan 2023 18:12:21 -0500 Subject: [PATCH 06/30] fix depth and add context typings --- packages/router/__tests__/router-test.ts | 585 +++++++++++------------ packages/router/index.ts | 4 + packages/router/router.ts | 35 +- packages/router/utils.ts | 45 +- 4 files changed, 350 insertions(+), 319 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 5d6bcd575f..0e66bd5792 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -13,6 +13,7 @@ import type { } from "../index"; import { createMemoryHistory, + createMiddlewareContext, createRouter, createStaticHandler, defer, @@ -29,10 +30,12 @@ import { // Private API import type { + ActionFunctionArgs, AgnosticIndexRouteObject, AgnosticNonIndexRouteObject, AgnosticRouteObject, DeferredData, + LoaderFunctionArgs, TrackedPromise, } from "../utils"; import { @@ -11531,7 +11534,44 @@ describe("a router", () => { describe("middleware", () => { describe("ordering", () => { let calls: string[]; - let MIDDLEWARE_ORDERING_ROUTES = [ + + let indentationContext = createMiddlewareContext(""); + + async function trackMiddlewareCall( + routeId: string, + { request, middleware }: ActionFunctionArgs | LoaderFunctionArgs + ) { + let indentation = middleware.get(indentationContext); + let type = request.method === "POST" ? "action" : "loader"; + let fetchSuffix = request.url.includes("?from-fetch") ? " (fetch)" : ""; + + calls.push( + `${indentation}${routeId} ${type} middleware start${fetchSuffix}` + ); + middleware.set(indentationContext, indentation + " "); + await tick(); + let res = await middleware.next(); + calls.push( + `${indentation}${routeId} ${type} middleware end${fetchSuffix}` + ); + return res; + } + + async function trackHandlerCall( + routeId: string, + { request, middleware }: ActionFunctionArgs | LoaderFunctionArgs + ) { + let indentation = middleware.get(indentationContext); + let type = request.method === "POST" ? "action" : "loader"; + let fetchSuffix = request.url.includes("?from-fetch") ? " (fetch)" : ""; + + calls.push(`${indentation}${routeId} ${type} start${fetchSuffix}`); + await tick(); + calls.push(`${indentation}${routeId} ${type} end${fetchSuffix}`); + return routeId.toUpperCase() + " " + type.toUpperCase(); + } + + let MIDDLEWARE_ORDERING_ROUTES: AgnosticRouteObject[] = [ { id: "root", path: "/", @@ -11539,64 +11579,37 @@ describe("a router", () => { { id: "parent", path: "/parent", - async middleware({ middleware }) { - calls.push("middleware start - parent"); - await tick(); - let res = await middleware.next(); - calls.push("middleware end - parent"); - return res; - }, - async action() { - calls.push("action start - parent"); - await tick(); - calls.push("action end - parent"); - return "PARENT ACTION"; - }, - async loader() { - calls.push("loader start - parent"); - await tick(); - calls.push("loader end - parent"); - return "PARENT"; + middleware(args) { + return trackMiddlewareCall("parent", args); + }, + action(args) { + return trackHandlerCall("parent", args); + }, + loader(args) { + return trackHandlerCall("parent", args); }, children: [ { id: "child", path: "child", - async middleware({ middleware }) { - calls.push("middleware start - child"); - await tick(); - let res = await middleware.next(); - calls.push("middleware end - child"); - return res; + middleware(args) { + return trackMiddlewareCall("child", args); }, - async loader() { - calls.push("loader start - child"); - await tick(); - calls.push("loader end - child"); - return "CHILD"; + loader(args) { + return trackHandlerCall("child", args); }, children: [ { id: "grandchild", path: "grandchild", - async middleware({ middleware }) { - calls.push("middleware start - grandchild"); - await tick(); - let res = await middleware.next(); - calls.push("middleware end - grandchild"); - return res; + middleware(args) { + return trackMiddlewareCall("grandchild", args); }, - async action() { - calls.push("action start - grandchild"); - await tick(); - calls.push("action end - grandchild"); - return "GRANDCHILD ACTION"; + action(args) { + return trackHandlerCall("grandchild", args); }, - async loader() { - calls.push("loader start - grandchild"); - await tick(); - calls.push("loader end - grandchild"); - return "GRANDCHILD"; + loader(args) { + return trackHandlerCall("grandchild", args); }, }, ], @@ -11619,14 +11632,14 @@ describe("a router", () => { expect(currentRouter.state.location.pathname).toBe("/parent"); expect(currentRouter.state.loaderData).toEqual({ - parent: "PARENT", + parent: "PARENT LOADER", }); expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "loader start - parent", - "loader end - parent", - "middleware end - parent", + "parent loader middleware start", + " parent loader start", + " parent loader end", + "parent loader middleware end", ] `); }); @@ -11647,18 +11660,18 @@ describe("a router", () => { parent: "PARENT ACTION", }); expect(currentRouter.state.loaderData).toEqual({ - parent: "PARENT", + parent: "PARENT LOADER", }); expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "action start - parent", - "action end - parent", - "middleware end - parent", - "middleware start - parent", - "loader start - parent", - "loader end - parent", - "middleware end - parent", + "parent action middleware start", + " parent action start", + " parent action end", + "parent action middleware end", + "parent loader middleware start", + " parent loader start", + " parent loader end", + "parent loader middleware end", ] `); }); @@ -11675,42 +11688,37 @@ describe("a router", () => { "/parent/child/grandchild" ); expect(currentRouter.state.loaderData).toEqual({ - parent: "PARENT", - child: "CHILD", - grandchild: "GRANDCHILD", + parent: "PARENT LOADER", + child: "CHILD LOADER", + grandchild: "GRANDCHILD LOADER", }); - // 1. Middleware chains all start in parallel for each loader and run - // sequentially down the matches - // 2. Then loaders all run in parallel - // 3. When a loader ends, it triggers the bubbling back up the - // middleware chain + // - Middleware chains all start in parallel for each loader and run + // sequentially down the matches + // - Loaders run slightly offset since they have different middleware + // depths + // - When a loader ends, it triggers the bubbling back up the + // middleware chain expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "middleware start - parent", - "middleware start - parent", - "middleware start - child", - "middleware start - child", - "middleware start - child", - "middleware start - grandchild", - "middleware start - grandchild", - "middleware start - grandchild", - "loader start - parent", - "loader start - child", - "loader start - grandchild", - "loader end - parent", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "loader end - child", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "loader end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", + "parent loader middleware start", + "parent loader middleware start", + "parent loader middleware start", + " parent loader start", + " child loader middleware start", + " child loader middleware start", + " parent loader end", + "parent loader middleware end", + " child loader start", + " grandchild loader middleware start", + " child loader end", + " child loader middleware end", + "parent loader middleware end", + " grandchild loader start", + " grandchild loader end", + " grandchild loader middleware end", + " child loader middleware end", + "parent loader middleware end", ] `); }); @@ -11733,53 +11741,48 @@ describe("a router", () => { grandchild: "GRANDCHILD ACTION", }); expect(currentRouter.state.loaderData).toEqual({ - parent: "PARENT", - child: "CHILD", - grandchild: "GRANDCHILD", - }); - - // 1. Middleware chain runs top-down for the action - // 2. Then the action runs - // 3. When the action ends, it bubbled back up the middleware chain - // 4. Middleware chains then start in parallel for each loader and run - // top-down down the matches - // 5. Then loaders run in parallel - // 6. When a loader ends, it triggers the bubbling back up the - // middleware chain + parent: "PARENT LOADER", + child: "CHILD LOADER", + grandchild: "GRANDCHILD LOADER", + }); + + // - Middleware chain runs top-down for the action + // - Then the action runs + // - When the action ends, it bubbled back up the middleware chain + // - Middleware chains all start in parallel for each loader and run + // sequentially down the matches + // - Loaders run slightly offset since they have different middleware + // depths + // - When a loader ends, it triggers the bubbling back up the + // middleware chain expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "middleware start - child", - "middleware start - grandchild", - "action start - grandchild", - "action end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "middleware start - parent", - "middleware start - parent", - "middleware start - parent", - "middleware start - child", - "middleware start - child", - "middleware start - child", - "middleware start - grandchild", - "middleware start - grandchild", - "middleware start - grandchild", - "loader start - parent", - "loader start - child", - "loader start - grandchild", - "loader end - parent", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "loader end - child", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "loader end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", + "parent action middleware start", + " child action middleware start", + " grandchild action middleware start", + " grandchild action start", + " grandchild action end", + " grandchild action middleware end", + " child action middleware end", + "parent action middleware end", + "parent loader middleware start", + "parent loader middleware start", + "parent loader middleware start", + " parent loader start", + " child loader middleware start", + " child loader middleware start", + " parent loader end", + "parent loader middleware end", + " child loader start", + " grandchild loader middleware start", + " child loader end", + " child loader middleware end", + "parent loader middleware end", + " grandchild loader start", + " grandchild loader end", + " grandchild loader middleware end", + " child loader middleware end", + "parent loader middleware end", ] `); }); @@ -11798,14 +11801,14 @@ describe("a router", () => { expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "middleware start - child", - "middleware start - grandchild", - "loader start - grandchild", - "loader end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", + "parent loader middleware start (fetch)", + " child loader middleware start (fetch)", + " grandchild loader middleware start (fetch)", + " grandchild loader start (fetch)", + " grandchild loader end (fetch)", + " grandchild loader middleware end (fetch)", + " child loader middleware end (fetch)", + "parent loader middleware end (fetch)", ] `); }); @@ -11828,14 +11831,14 @@ describe("a router", () => { expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "middleware start - child", - "middleware start - grandchild", - "action start - grandchild", - "action end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", + "parent action middleware start (fetch)", + " child action middleware start (fetch)", + " grandchild action middleware start (fetch)", + " grandchild action start (fetch)", + " grandchild action end (fetch)", + " grandchild action middleware end (fetch)", + " child action middleware end (fetch)", + "parent action middleware end (fetch)", ] `); }); @@ -11864,38 +11867,32 @@ describe("a router", () => { expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "middleware start - child", - "middleware start - grandchild", - "action start - grandchild", - "action end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "middleware start - parent", - "middleware start - parent", - "middleware start - parent", - "middleware start - child", - "middleware start - child", - "middleware start - child", - "middleware start - grandchild", - "middleware start - grandchild", - "middleware start - grandchild", - "loader start - parent", - "loader start - child", - "loader start - grandchild", - "loader end - parent", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "loader end - child", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "loader end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", + "parent action middleware start (fetch)", + " child action middleware start (fetch)", + " grandchild action middleware start (fetch)", + " grandchild action start (fetch)", + " grandchild action end (fetch)", + " grandchild action middleware end (fetch)", + " child action middleware end (fetch)", + "parent action middleware end (fetch)", + "parent loader middleware start", + "parent loader middleware start", + "parent loader middleware start", + " parent loader start", + " child loader middleware start", + " child loader middleware start", + " parent loader end", + "parent loader middleware end", + " child loader start", + " grandchild loader middleware start", + " child loader end", + " child loader middleware end", + "parent loader middleware end", + " grandchild loader start", + " grandchild loader end", + " grandchild loader middleware end", + " child loader middleware end", + "parent loader middleware end", ] `); }); @@ -11919,32 +11916,26 @@ describe("a router", () => { formData: createFormData(), }); - await tick(); - await tick(); - await tick(); - await tick(); - await tick(); - expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "action start - parent", - "action end - parent", - "middleware end - parent", - "middleware start - parent", - "middleware start - parent", - "middleware start - parent", - "loader start - parent", - "loader start - parent", - "middleware start - child", - "loader end - parent", - "middleware end - parent", - "loader end - parent", - "middleware end - parent", - "loader start - child", - "loader end - child", - "middleware end - child", - "middleware end - parent", + "parent action middleware start", + " parent action start", + " parent action end", + "parent action middleware end", + "parent loader middleware start", + "parent loader middleware start (fetch)", + "parent loader middleware start (fetch)", + " parent loader start", + " parent loader start (fetch)", + " child loader middleware start (fetch)", + " parent loader end", + "parent loader middleware end", + " parent loader end (fetch)", + "parent loader middleware end (fetch)", + " child loader start (fetch)", + " child loader end (fetch)", + " child loader middleware end (fetch)", + "parent loader middleware end (fetch)", ] `); }); @@ -11960,37 +11951,31 @@ describe("a router", () => { ); expect(context.loaderData).toMatchInlineSnapshot(` { - "child": "CHILD", - "grandchild": "GRANDCHILD", - "parent": "PARENT", + "child": "CHILD LOADER", + "grandchild": "GRANDCHILD LOADER", + "parent": "PARENT LOADER", } `); expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "middleware start - parent", - "middleware start - parent", - "middleware start - child", - "middleware start - child", - "middleware start - child", - "middleware start - grandchild", - "middleware start - grandchild", - "middleware start - grandchild", - "loader start - parent", - "loader start - child", - "loader start - grandchild", - "loader end - parent", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "loader end - child", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", - "loader end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", + "parent loader middleware start", + "parent loader middleware start", + "parent loader middleware start", + " parent loader start", + " child loader middleware start", + " child loader middleware start", + " parent loader end", + "parent loader middleware end", + " child loader start", + " grandchild loader middleware start", + " child loader end", + " child loader middleware end", + "parent loader middleware end", + " grandchild loader start", + " grandchild loader end", + " grandchild loader middleware end", + " child loader middleware end", + "parent loader middleware end", ] `); }); @@ -12002,17 +11987,17 @@ describe("a router", () => { createRequest("/parent/child/grandchild") ); - expect(result).toEqual("GRANDCHILD"); + expect(result).toEqual("GRANDCHILD LOADER"); expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "middleware start - child", - "middleware start - grandchild", - "loader start - grandchild", - "loader end - grandchild", - "middleware end - grandchild", - "middleware end - child", - "middleware end - parent", + "parent loader middleware start", + " child loader middleware start", + " grandchild loader middleware start", + " grandchild loader start", + " grandchild loader end", + " grandchild loader middleware end", + " child loader middleware end", + "parent loader middleware end", ] `); }); @@ -12028,30 +12013,22 @@ describe("a router", () => { id: "parent", path: "/parent", async middleware() { - calls.push("middleware start - parent"); - await tick(); - calls.push("middleware end - parent"); + calls.push("parent middleware"); }, async loader() { - calls.push("loader start - parent"); - await tick(); - calls.push("loader end - parent"); - return "PARENT"; + calls.push("parent loader"); + return "PARENT LOADER"; }, children: [ { id: "child", path: "child", async middleware() { - calls.push("middleware start - child"); - await tick(); - calls.push("middleware end - child"); + calls.push("child middleware"); }, async loader() { - calls.push("loader start - child"); - await tick(); - calls.push("loader end - child"); - return "CHILD"; + calls.push("child loader"); + return "CHILD LOADER"; }, }, ], @@ -12063,23 +12040,16 @@ describe("a router", () => { await currentRouter.navigate("/parent/child"); expect(currentRouter.state.loaderData).toEqual({ - parent: "PARENT", - child: "CHILD", + parent: "PARENT LOADER", + child: "CHILD LOADER", }); expect(calls).toMatchInlineSnapshot(` [ - "middleware start - parent", - "middleware start - parent", - "middleware end - parent", - "middleware start - child", - "middleware end - parent", - "middleware start - child", - "middleware end - child", - "loader start - parent", - "middleware end - child", - "loader start - child", - "loader end - parent", - "loader end - child", + "parent middleware", + "parent middleware", + "parent loader", + "child middleware", + "child loader", ] `); }); @@ -12185,45 +12155,52 @@ describe("a router", () => { }); describe("middleware context", () => { - let MIDDLEWARE_CONTEXT_ROUTES = [ + let loaderCountContext = createMiddlewareContext(0); + let actionCountContext = createMiddlewareContext(100); + + function incrementContextCount(request, middleware) { + if (request.method === "POST") { + let count = middleware.get(actionCountContext); + middleware.set(actionCountContext, count + 1); + } else { + let count = middleware.get(loaderCountContext); + middleware.set(loaderCountContext, count + 1); + } + } + + let MIDDLEWARE_CONTEXT_ROUTES: AgnosticRouteObject[] = [ { path: "/" }, { id: "parent", path: "/parent", async middleware({ request, middleware }) { - let type = request.method === "POST" ? "action" : "loader"; - let count = type === "action" ? 101 : 1; - middleware.set(`parent-${type}`, count); + incrementContextCount(request, middleware); }, async loader({ middleware }) { - return middleware.get(`parent-loader`); + return middleware.get(loaderCountContext); }, children: [ { id: "child", path: "child", async middleware({ request, middleware }) { - let type = request.method === "POST" ? "action" : "loader"; - let count = (middleware.get(`parent-${type}`) as number) + 1; - middleware.set(`child-${type}`, count); + incrementContextCount(request, middleware); }, async loader({ middleware }) { - return middleware.get("child-loader"); + return middleware.get(loaderCountContext); }, children: [ { id: "grandchild", path: "grandchild", async middleware({ request, middleware }) { - let type = request.method === "POST" ? "action" : "loader"; - let count = (middleware.get(`child-${type}`) as number) + 1; - middleware.set(`grandchild-${type}`, count); + incrementContextCount(request, middleware); }, async action({ middleware }) { - return middleware.get(`grandchild-action`); + return middleware.get(actionCountContext); }, async loader({ middleware }) { - return middleware.get("grandchild-loader"); + return middleware.get(loaderCountContext); }, }, ], @@ -12306,6 +12283,8 @@ describe("a router", () => { }); it("throws if no value is available via middleware.get()", async () => { + let theContext = createMiddlewareContext(); + currentRouter = createRouter({ routes: [ { @@ -12315,7 +12294,7 @@ describe("a router", () => { id: "broken", path: "broken", loader({ middleware }) { - return middleware.get("nope"); + return middleware.get(theContext); }, }, ], @@ -12333,6 +12312,8 @@ describe("a router", () => { }); it("throws if you try to set an undefined value in middleware.set()", async () => { + let theContext = createMiddlewareContext(); + currentRouter = createRouter({ routes: [ { @@ -12342,7 +12323,7 @@ describe("a router", () => { id: "broken", path: "broken", middleware({ middleware }) { - return middleware.set("nope", undefined); + return middleware.set(theContext, undefined); }, loader() { return "DATA"; @@ -12363,6 +12344,11 @@ describe("a router", () => { }); it("allows null/falsey values in middleware.set()", async () => { + let booleanContext = createMiddlewareContext(); + let numberContext = createMiddlewareContext(); + let stringContext = createMiddlewareContext(); + let whateverContext = createMiddlewareContext(); + currentRouter = createRouter({ routes: [ { @@ -12372,15 +12358,17 @@ describe("a router", () => { id: "works", path: "works", middleware({ middleware }) { - middleware.set("a", null); - middleware.set("b", false); - middleware.set("c", ""); + middleware.set(booleanContext, false); + middleware.set(numberContext, 0); + middleware.set(stringContext, ""); + middleware.set(whateverContext, null); }, loader({ middleware }) { return { - a: middleware.get("a"), - b: middleware.get("b"), - c: middleware.get("c"), + boolean: middleware.get(booleanContext), + number: middleware.get(numberContext), + string: middleware.get(stringContext), + whatever: middleware.get(whateverContext), }; }, }, @@ -12394,9 +12382,10 @@ describe("a router", () => { expect(currentRouter.state.loaderData).toMatchInlineSnapshot(` { "works": { - "a": null, - "b": false, - "c": "", + "boolean": false, + "number": 0, + "string": "", + "whatever": null, }, } `); diff --git a/packages/router/index.ts b/packages/router/index.ts index 21670631d3..a5c77de03f 100644 --- a/packages/router/index.ts +++ b/packages/router/index.ts @@ -15,6 +15,9 @@ export type { JsonFunction, LoaderFunction, LoaderFunctionArgs, + MiddlewareContext, + MiddlewareFunction, + MiddlewareFunctionArgs, ParamParseKey, Params, PathMatch, @@ -27,6 +30,7 @@ export type { export { AbortedDeferredError, ErrorResponse, + createMiddlewareContext, defer, generatePath, getToPathname, diff --git a/packages/router/router.ts b/packages/router/router.ts index e322cba542..1c0989c111 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -7,25 +7,26 @@ import { parsePath, } from "./history"; import type { + ActionFunction, AgnosticDataRouteMatch, AgnosticDataRouteObject, AgnosticRouteMatch, AgnosticRouteObject, - MiddlewareContext, DataResult, DeferredResult, ErrorResult, FormEncType, FormMethod, + LoaderFunction, + MiddlewareContext, + MiddlewareContextInstance, MutationFormMethod, + Params, RedirectResult, RouteData, ShouldRevalidateFunction, Submission, SuccessResult, - LoaderFunction, - ActionFunction, - Params, } from "./utils"; import { convertRoutesToDataRoutes, @@ -3049,15 +3050,22 @@ async function callRoutePipeline( requestContext: unknown, handler: LoaderFunction | ActionFunction ) { - let store = new Map(); + // Avoid memory leaks since we don't control the key + // TODO: Any way to type this so that .get/.set can infer the value type + // from the key? This would avoid our `as T` below. + let store = new WeakMap(); let middlewareContext: MiddlewareContext = { - get(k) { + get(k: MiddlewareContextInstance) { if (!store.has(k)) { - throw new Error("Unable to find a value in the middleware context"); + let defaultValue = k.getDefaultValue(); + if (defaultValue == null) { + throw new Error("Unable to find a value in the middleware context"); + } + return defaultValue; } - return store.get(k); + return store.get(k) as T; }, - set(k, v) { + set(k: MiddlewareContextInstance, v: T) { if (typeof v === "undefined") { throw new Error( "You cannot set an undefined value in the middleware context" @@ -3165,8 +3173,15 @@ async function callLoaderOrAction( `Could not find the ${type} to run on the "${match.route.id}" route` ); + // Only call the pipeline for the matches up to this specific match + let idx = matches.findIndex((m) => m.route.id === match.route.id); result = await Promise.race([ - callRoutePipeline(request, matches, requestContext, handler), + callRoutePipeline( + request, + matches.slice(0, idx + 1), + requestContext, + handler + ), abortPromise, ]); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 4a094156d1..56f6cabb98 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -94,17 +94,6 @@ interface DataFunctionArgs { middleware: MiddlewareContext; } -/** - * Context object passed through middleware functions and into action/loaders. - * - * Supports only key/value for now, eventually will be enhanced - */ -export interface MiddlewareContext { - get(key: string): unknown; - set(key: string, value: unknown): void; - next: () => DataFunctionReturnValue; -} - /** * Arguments passed to middleware functions */ @@ -1442,3 +1431,37 @@ export function isRouteErrorResponse(error: any): error is ErrorResponse { "data" in error ); } + +/** + * Context object passed through middleware functions and into action/loaders. + * + * Supports only key/value for now, eventually will be enhanced + */ +export interface MiddlewareContext { + get(key: MiddlewareContextInstance): T; + set(key: MiddlewareContextInstance, value: T): void; + next: () => DataFunctionReturnValue; +} + +/** + * Generic class to "hold" a default middleware value and the generic type so + * we can enforce typings on middleware.get/set + */ +export class MiddlewareContextInstance { + private defaultValue: T | null; + + constructor(defaultValue?: T | null) { + this.defaultValue = + typeof defaultValue !== "undefined" ? defaultValue : null; + } + + getDefaultValue() { + return this.defaultValue; + } +} + +export function createMiddlewareContext( + defaultValue: T +): MiddlewareContextInstance { + return new MiddlewareContextInstance(defaultValue); +} From 0a56a10f77ea79e1f7d0e6f545b006070a464c34 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 27 Jan 2023 11:36:14 -0500 Subject: [PATCH 07/30] Add future flag --- packages/router/__tests__/router-test.ts | 113 ++++++++++++++++++++++- packages/router/router.ts | 79 +++++++++++++--- 2 files changed, 175 insertions(+), 17 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 0e66bd5792..0b9b39a843 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -11626,6 +11626,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_ORDERING_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/parent"); @@ -11648,6 +11649,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_ORDERING_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/parent", { @@ -11680,6 +11682,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_ORDERING_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/parent/child/grandchild"); @@ -11727,6 +11730,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_ORDERING_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/parent/child/grandchild", { @@ -11791,6 +11795,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_ORDERING_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.fetch( @@ -11817,6 +11822,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_ORDERING_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.fetch( @@ -11847,6 +11853,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_ORDERING_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/parent/child/grandchild"); @@ -11901,6 +11908,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_ORDERING_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/parent"); @@ -11941,7 +11949,9 @@ describe("a router", () => { }); it("runs middleware before staticHandler.query", async () => { - let { query } = createStaticHandler(MIDDLEWARE_ORDERING_ROUTES); + let { query } = createStaticHandler(MIDDLEWARE_ORDERING_ROUTES, { + future: { unstable_middleware: true }, + }); let context = await query(createRequest("/parent/child/grandchild")); @@ -11981,7 +11991,9 @@ describe("a router", () => { }); it("runs middleware before staticHandler.queryRoute", async () => { - let { queryRoute } = createStaticHandler(MIDDLEWARE_ORDERING_ROUTES); + let { queryRoute } = createStaticHandler(MIDDLEWARE_ORDERING_ROUTES, { + future: { unstable_middleware: true }, + }); let result = await queryRoute( createRequest("/parent/child/grandchild") @@ -12035,6 +12047,7 @@ describe("a router", () => { }, ], history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/parent/child"); @@ -12074,6 +12087,7 @@ describe("a router", () => { }, ], history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter?.navigate("/parent"); @@ -12105,6 +12119,7 @@ describe("a router", () => { }, ], history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter?.navigate("/parent"); @@ -12139,6 +12154,7 @@ describe("a router", () => { }, ], history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter?.navigate("/parent", { @@ -12152,6 +12168,92 @@ describe("a router", () => { ), }); }); + + it("does not run middleware if flag is not enabled", async () => { + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + }, + { + id: "parent", + path: "/parent", + middleware() { + throw new Error("Nope!"); + }, + loader() { + calls.push("parent loader"); + return "PARENT LOADER"; + }, + }, + ], + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent"); + + expect(currentRouter.state.location.pathname).toBe("/parent"); + expect(currentRouter.state.loaderData).toEqual({ + parent: "PARENT LOADER", + }); + expect(calls).toMatchInlineSnapshot(` + [ + "parent loader", + ] + `); + }); + + it("throws if middleware get methods are called when flag is not enabled", async () => { + currentRouter = createRouter({ + routes: [ + { + id: "root", + path: "/", + }, + { + id: "parent", + path: "/parent", + loader({ request, middleware }) { + let sp = new URL(request.url).searchParams; + if (sp.has("get")) { + middleware.get(createMiddlewareContext(0)); + } else if (sp.has("set")) { + middleware.set(createMiddlewareContext(0), 1); + } else if (sp.has("next")) { + middleware.next(); + } + + return "PARENT LOADER"; + }, + }, + ], + history: createMemoryHistory(), + }).initialize(); + + await currentRouter.navigate("/parent?get"); + expect(currentRouter.state.errors).toMatchInlineSnapshot(` + { + "parent": [Error: Middleware not enabled (\`future.unstable_middleware\`)], + } + `); + + await currentRouter.navigate("/"); + await currentRouter.navigate("/parent?set"); + expect(currentRouter.state.errors).toMatchInlineSnapshot(` + { + "parent": [Error: Middleware not enabled (\`future.unstable_middleware\`)], + } + `); + + await currentRouter.navigate("/"); + await currentRouter.navigate("/parent?next"); + expect(currentRouter.state.errors).toMatchInlineSnapshot(` + { + "parent": [Error: Middleware not enabled (\`future.unstable_middleware\`)], + } + `); + }); }); describe("middleware context", () => { @@ -12213,6 +12315,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_CONTEXT_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/parent/child/grandchild"); @@ -12231,6 +12334,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_CONTEXT_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/parent/child/grandchild", { @@ -12255,6 +12359,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_CONTEXT_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.fetch("key", "root", "/parent/child/grandchild"); @@ -12269,6 +12374,7 @@ describe("a router", () => { currentRouter = createRouter({ routes: MIDDLEWARE_CONTEXT_ROUTES, history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.fetch("key", "root", "/parent/child/grandchild", { @@ -12299,6 +12405,7 @@ describe("a router", () => { }, ], history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/broken"); @@ -12331,6 +12438,7 @@ describe("a router", () => { }, ], history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/broken"); @@ -12374,6 +12482,7 @@ describe("a router", () => { }, ], history: createMemoryHistory(), + future: { unstable_middleware: true }, }).initialize(); await currentRouter.navigate("/works"); diff --git a/packages/router/router.ts b/packages/router/router.ts index 1c0989c111..85821576b5 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -315,6 +315,13 @@ export type HydrationState = Partial< Pick >; +/** + * Future flags to toggle on new feature behavior + */ +export interface FutureConfig { + unstable_middleware: boolean; +} + /** * Initialization options for createRouter */ @@ -323,6 +330,7 @@ export interface RouterInit { routes: AgnosticRouteObject[]; history: History; hydrationData?: HydrationState; + future?: FutureConfig; } /** @@ -582,6 +590,7 @@ interface QueryRouteResponse { response: Response; } +const defaultFutureConfig: FutureConfig = { unstable_middleware: false }; const validMutationMethodsArr: MutationFormMethod[] = [ "post", "put", @@ -649,6 +658,8 @@ export function createRouter(init: RouterInit): Router { ); let dataRoutes = convertRoutesToDataRoutes(init.routes); + let future: FutureConfig = { ...defaultFutureConfig, ...init.future }; + // Cleanup function for history let unlistenHistory: (() => void) | null = null; // Externally-provided functions to call on all state changes @@ -1262,7 +1273,8 @@ export function createRouter(init: RouterInit): Router { request, actionMatch, matches, - router.basename + router.basename, + future.unstable_middleware ); if (request.signal.aborted) { @@ -1592,7 +1604,8 @@ export function createRouter(init: RouterInit): Router { fetchRequest, match, requestMatches, - router.basename + router.basename, + future.unstable_middleware ); if (fetchRequest.signal.aborted) { @@ -1814,7 +1827,8 @@ export function createRouter(init: RouterInit): Router { fetchRequest, match, matches, - router.basename + router.basename, + future.unstable_middleware ); // Deferred isn't supported for fetcher loads, await everything and treat it @@ -2007,7 +2021,14 @@ export function createRouter(init: RouterInit): Router { // accordingly let results = await Promise.all([ ...matchesToLoad.map((match) => - callLoaderOrAction("loader", request, match, matches, router.basename) + callLoaderOrAction( + "loader", + request, + match, + matches, + router.basename, + future.unstable_middleware + ) ), ...fetchersToLoad.map((f) => callLoaderOrAction( @@ -2015,7 +2036,8 @@ export function createRouter(init: RouterInit): Router { createClientSideRequest(init.history, f.path, request.signal), f.match, f.matches, - router.basename + router.basename, + future.unstable_middleware ) ), ]); @@ -2317,11 +2339,14 @@ export function createRouter(init: RouterInit): Router { export const UNSAFE_DEFERRED_SYMBOL = Symbol("deferred"); +export interface StaticHandlerInit { + basename?: string; + future?: FutureConfig; +} + export function createStaticHandler( routes: AgnosticRouteObject[], - opts?: { - basename?: string; - } + init?: StaticHandlerInit ): StaticHandler { invariant( routes.length > 0, @@ -2329,7 +2354,11 @@ export function createStaticHandler( ); let dataRoutes = convertRoutesToDataRoutes(routes); - let basename = (opts ? opts.basename : null) || "/"; + let basename = (init ? init.basename : null) || "/"; + let future: FutureConfig = { + ...defaultFutureConfig, + ...(init && init.future ? init.future : null), + }; /** * The query() method is intended for document requests, in which we want to @@ -2583,6 +2612,7 @@ export function createStaticHandler( actionMatch, matches, basename, + future.unstable_middleware, true, isRouteRequest, requestContext @@ -2743,6 +2773,7 @@ export function createStaticHandler( match, matches, basename, + future.unstable_middleware, true, isRouteRequest, requestContext @@ -3147,12 +3178,23 @@ async function callRouteSubPipeline( } } +function disabledMiddlewareFn() { + throw new Error("Middleware not enabled (`future.unstable_middleware`)"); +} + +const disabledMiddlewareContext: MiddlewareContext = { + get: disabledMiddlewareFn, + set: disabledMiddlewareFn, + next: disabledMiddlewareFn, +}; + async function callLoaderOrAction( type: "loader" | "action", request: Request, match: AgnosticDataRouteMatch, matches: AgnosticDataRouteMatch[], basename = "/", + enableMiddleware: boolean, isStaticRequest: boolean = false, isRouteRequest: boolean = false, requestContext?: unknown @@ -3176,12 +3218,19 @@ async function callLoaderOrAction( // Only call the pipeline for the matches up to this specific match let idx = matches.findIndex((m) => m.route.id === match.route.id); result = await Promise.race([ - callRoutePipeline( - request, - matches.slice(0, idx + 1), - requestContext, - handler - ), + enableMiddleware + ? callRoutePipeline( + request, + matches.slice(0, idx + 1), + requestContext, + handler + ) + : handler({ + request, + params: match.params, + context: requestContext, + middleware: disabledMiddlewareContext, + }), abortPromise, ]); From 545a8b7cc66a4edb58412365e3b03c147d122e75 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 27 Jan 2023 11:39:09 -0500 Subject: [PATCH 08/30] Bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index d2cf1bd74a..5b0853ba3c 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "41.5 kB" + "none": "42.6 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13 kB" From 9c41325888cb7d0ea234c36d3cb347d1595bb26f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 27 Jan 2023 12:17:31 -0500 Subject: [PATCH 09/30] Add changeset and re-exports --- .changeset/hip-geckos-fold.md | 73 ++++++++++++++++++++++++++ packages/react-router-dom/index.tsx | 5 ++ packages/react-router-native/index.tsx | 5 ++ packages/react-router/index.ts | 10 ++++ 4 files changed, 93 insertions(+) create mode 100644 .changeset/hip-geckos-fold.md diff --git a/.changeset/hip-geckos-fold.md b/.changeset/hip-geckos-fold.md new file mode 100644 index 0000000000..11fec9422c --- /dev/null +++ b/.changeset/hip-geckos-fold.md @@ -0,0 +1,73 @@ +--- +"@remix-run/router": minor +--- + +Adds support for "middleware" on routes to give you a common place to run before and after your loaders and actions in a single location higher up in the routing tree. The API we landed on is inspired by the middleware API in [Fresh](https://fresh.deno.dev/docs/concepts/middleware) since it supports the concept of nested routes and also allows you to run logic on the response _after_ the fact. + +This feature is behind a `future.unstable_middleware` flag at the moment, but major API changes are not expected and we believe it's ready for production usage. This flag allows us to make small "breaking" changes if required users un into unforeseen issues. + +To opt into the middleware feature, you pass the flag to your `createBrowserRouter` (or equivalent) method, and then you can define a `middleware` function on your routes: + +```tsx +import { + createBrowserRouter, + createMiddlewareContext, + RouterProvider, +} from "react-router-dom"; +import { getSession, commitSession } from "../session"; +import { getPosts } from "../posts"; + +// Create strongly-typed contexts to use for your middleware data +let userCtx = createMiddlewareContext(null); +let sessionCtx = createMiddlewareContext(null); + +const routes = [ + { + path: "/", + middleware: rootMiddleware, + children: [ + { + path: "path", + loader: childLoader, + }, + ], + }, +]; + +const router = createBrowserRouter(routes, { + future: { + unstable_middleware: true, + }, +}); + +function App() { + return ; +} + +async function rootMiddleware({ request, context }) { + // 🔥 Load common information in one spot in your middleware and make it + // available to child middleware/loaders/actions + let session = await getSession(request.headers.get("Cookie")); + let user = await getUser(session); + middleware.set(userCtx, user); + middleware.set(sessionCtx, session); + + // Call child middleware/loaders/actions + let response = await middleware.next(); + + // 🔥 Assign common response headers on the way out + response.headers.append("Set-Cookie", await commitSession(session)); + return response; +} + +async function childLoader({ context }) { + // 🔥 Read strongly-typed data from ancestor middlewares + let session = context.get(sessionCtx); + let user = context.get(userCtx); + + let posts = await getPosts({ author: user.id }); + return redirect(`/posts/${post.id}`); +} +``` + +⚠️ Please note that middleware is executed on a per-`loader`/`action` basis because they can alter the `Response` from the target `loader`/`action`. This means that if you have 3 `loader`'s being called in parallel on a navigation or revalidation, they will _each_ run any existing root middleware. If these duplicate middleware calls are problematic then you will need to de-dup any problematic logic manually. diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 882159e6cc..6f7b6c3f58 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -82,6 +82,7 @@ export type { DataRouteMatch, DataRouteObject, Fetcher, + FutureConfig, Hash, IndexRouteObject, IndexRouteProps, @@ -91,6 +92,9 @@ export type { LoaderFunctionArgs, Location, MemoryRouterProps, + MiddlewareContext, + MiddlewareFunction, + MiddlewareFunctionArgs, NavigateFunction, NavigateOptions, NavigateProps, @@ -129,6 +133,7 @@ export { RouterProvider, Routes, createMemoryRouter, + createMiddlewareContext, createPath, createRoutesFromChildren, createRoutesFromElements, diff --git a/packages/react-router-native/index.tsx b/packages/react-router-native/index.tsx index 07335e33ef..28b92cf36e 100644 --- a/packages/react-router-native/index.tsx +++ b/packages/react-router-native/index.tsx @@ -28,6 +28,7 @@ export type { DataRouteMatch, DataRouteObject, Fetcher, + FutureConfig, Hash, IndexRouteObject, IndexRouteProps, @@ -37,6 +38,9 @@ export type { LoaderFunctionArgs, Location, MemoryRouterProps, + MiddlewareContext, + MiddlewareFunction, + MiddlewareFunctionArgs, NavigateFunction, NavigateOptions, NavigateProps, @@ -75,6 +79,7 @@ export { RouterProvider, Routes, createMemoryRouter, + createMiddlewareContext, createPath, createRoutesFromChildren, createRoutesFromElements, diff --git a/packages/react-router/index.ts b/packages/react-router/index.ts index 3d55ed2e22..c72eba5e5f 100644 --- a/packages/react-router/index.ts +++ b/packages/react-router/index.ts @@ -4,11 +4,15 @@ import type { Blocker, BlockerFunction, Fetcher, + FutureConfig, HydrationState, JsonFunction, LoaderFunction, LoaderFunctionArgs, Location, + MiddlewareContext, + MiddlewareFunction, + MiddlewareFunctionArgs, Navigation, Params, ParamParseKey, @@ -25,6 +29,7 @@ import { AbortedDeferredError, Action as NavigationType, createMemoryHistory, + createMiddlewareContext, createPath, createRouter, defer, @@ -122,6 +127,7 @@ export type { DataRouteMatch, DataRouteObject, Fetcher, + FutureConfig, Hash, IndexRouteObject, IndexRouteProps, @@ -131,6 +137,9 @@ export type { LoaderFunctionArgs, Location, MemoryRouterProps, + MiddlewareContext, + MiddlewareFunction, + MiddlewareFunctionArgs, NavigateFunction, NavigateOptions, NavigateProps, @@ -168,6 +177,7 @@ export { Router, RouterProvider, Routes, + createMiddlewareContext, createPath, createRoutesFromChildren, createRoutesFromChildren as createRoutesFromElements, From 5e83ef56b11f7e3c3cd943eb035165682a430978 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 27 Jan 2023 13:04:53 -0500 Subject: [PATCH 10/30] bump --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 5b0853ba3c..5edd91e48a 100644 --- a/package.json +++ b/package.json @@ -114,7 +114,7 @@ "none": "15 kB" }, "packages/react-router-dom/dist/react-router-dom.production.min.js": { - "none": "11.5 kB" + "none": "11.6 kB" }, "packages/react-router-dom/dist/umd/react-router-dom.production.min.js": { "none": "17.5 kB" From 414c4fb3bcbb57b83a5bc6c660837153a86931cc Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 27 Jan 2023 15:58:27 -0500 Subject: [PATCH 11/30] Update typings --- packages/router/router.ts | 12 +++--------- packages/router/utils.ts | 20 ++++++++++++++------ 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 85821576b5..b7bb3345f8 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -3082,19 +3082,13 @@ async function callRoutePipeline( handler: LoaderFunction | ActionFunction ) { // Avoid memory leaks since we don't control the key - // TODO: Any way to type this so that .get/.set can infer the value type - // from the key? This would avoid our `as T` below. let store = new WeakMap(); let middlewareContext: MiddlewareContext = { get(k: MiddlewareContextInstance) { - if (!store.has(k)) { - let defaultValue = k.getDefaultValue(); - if (defaultValue == null) { - throw new Error("Unable to find a value in the middleware context"); - } - return defaultValue; + if (store.has(k)) { + return store.get(k) as T; } - return store.get(k) as T; + return k.getDefaultValue(); }, set(k: MiddlewareContextInstance, v: T) { if (typeof v === "undefined") { diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 56f6cabb98..70d7446b18 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -1448,20 +1448,28 @@ export interface MiddlewareContext { * we can enforce typings on middleware.get/set */ export class MiddlewareContextInstance { - private defaultValue: T | null; + private defaultValue: T | undefined; - constructor(defaultValue?: T | null) { - this.defaultValue = - typeof defaultValue !== "undefined" ? defaultValue : null; + constructor(defaultValue?: T) { + if (typeof defaultValue !== "undefined") { + this.defaultValue = defaultValue; + } } - getDefaultValue() { + getDefaultValue(): T { + if (typeof this.defaultValue === "undefined") { + throw new Error("Unable to find a value in the middleware context"); + } return this.defaultValue; } } +/** + * Create a middleware context that can be used as a "key" to set/get middleware + * values in a strongly-typed fashion + */ export function createMiddlewareContext( - defaultValue: T + defaultValue?: T ): MiddlewareContextInstance { return new MiddlewareContextInstance(defaultValue); } From 4b89566a98d2dad89c8fa5a4e9d564f444cf77d0 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 27 Jan 2023 16:30:30 -0500 Subject: [PATCH 12/30] Leverage context param when flag is set --- packages/react-router-dom/index.tsx | 4 + packages/react-router-native/index.tsx | 4 + packages/react-router/index.ts | 8 ++ packages/react-router/lib/components.tsx | 2 + packages/react-router/lib/context.ts | 2 + packages/router/__tests__/router-test.ts | 130 +++++++++++------------ packages/router/index.ts | 4 + packages/router/router.ts | 57 ++++++---- packages/router/utils.ts | 76 +++++++++---- 9 files changed, 177 insertions(+), 110 deletions(-) diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 6f7b6c3f58..93dd6961cf 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -76,6 +76,8 @@ export { createSearchParams }; export type { ActionFunction, ActionFunctionArgs, + ActionFunctionWithMiddleware, + ActionFunctionArgsWithMiddleware, AwaitProps, unstable_Blocker, unstable_BlockerFunction, @@ -90,6 +92,8 @@ export type { LayoutRouteProps, LoaderFunction, LoaderFunctionArgs, + LoaderFunctionWithMiddleware, + LoaderFunctionArgsWithMiddleware, Location, MemoryRouterProps, MiddlewareContext, diff --git a/packages/react-router-native/index.tsx b/packages/react-router-native/index.tsx index 28b92cf36e..777906d830 100644 --- a/packages/react-router-native/index.tsx +++ b/packages/react-router-native/index.tsx @@ -22,6 +22,8 @@ import URLSearchParams from "@ungap/url-search-params"; export type { ActionFunction, ActionFunctionArgs, + ActionFunctionWithMiddleware, + ActionFunctionArgsWithMiddleware, AwaitProps, unstable_Blocker, unstable_BlockerFunction, @@ -36,6 +38,8 @@ export type { LayoutRouteProps, LoaderFunction, LoaderFunctionArgs, + LoaderFunctionWithMiddleware, + LoaderFunctionArgsWithMiddleware, Location, MemoryRouterProps, MiddlewareContext, diff --git a/packages/react-router/index.ts b/packages/react-router/index.ts index c72eba5e5f..23af522352 100644 --- a/packages/react-router/index.ts +++ b/packages/react-router/index.ts @@ -1,6 +1,8 @@ import type { ActionFunction, ActionFunctionArgs, + ActionFunctionWithMiddleware, + ActionFunctionArgsWithMiddleware, Blocker, BlockerFunction, Fetcher, @@ -9,6 +11,8 @@ import type { JsonFunction, LoaderFunction, LoaderFunctionArgs, + LoaderFunctionWithMiddleware, + LoaderFunctionArgsWithMiddleware, Location, MiddlewareContext, MiddlewareFunction, @@ -121,6 +125,8 @@ type Search = string; export type { ActionFunction, ActionFunctionArgs, + ActionFunctionWithMiddleware, + ActionFunctionArgsWithMiddleware, AwaitProps, Blocker as unstable_Blocker, BlockerFunction as unstable_BlockerFunction, @@ -135,6 +141,8 @@ export type { LayoutRouteProps, LoaderFunction, LoaderFunctionArgs, + LoaderFunctionWithMiddleware, + LoaderFunctionArgsWithMiddleware, Location, MemoryRouterProps, MiddlewareContext, diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx index b76e5781e8..7b0b74501a 100644 --- a/packages/react-router/lib/components.tsx +++ b/packages/react-router/lib/components.tsx @@ -235,6 +235,7 @@ export interface PathRouteProps { caseSensitive?: NonIndexRouteObject["caseSensitive"]; path?: NonIndexRouteObject["path"]; id?: NonIndexRouteObject["id"]; + middleware?: NonIndexRouteObject["middleware"]; loader?: NonIndexRouteObject["loader"]; action?: NonIndexRouteObject["action"]; hasErrorBoundary?: NonIndexRouteObject["hasErrorBoundary"]; @@ -252,6 +253,7 @@ export interface IndexRouteProps { caseSensitive?: IndexRouteObject["caseSensitive"]; path?: IndexRouteObject["path"]; id?: IndexRouteObject["id"]; + middleware?: IndexRouteObject["middleware"]; loader?: IndexRouteObject["loader"]; action?: IndexRouteObject["action"]; hasErrorBoundary?: IndexRouteObject["hasErrorBoundary"]; diff --git a/packages/react-router/lib/context.ts b/packages/react-router/lib/context.ts index e29e01ef1b..b804e1e38b 100644 --- a/packages/react-router/lib/context.ts +++ b/packages/react-router/lib/context.ts @@ -18,6 +18,7 @@ export interface IndexRouteObject { caseSensitive?: AgnosticIndexRouteObject["caseSensitive"]; path?: AgnosticIndexRouteObject["path"]; id?: AgnosticIndexRouteObject["id"]; + middleware?: AgnosticIndexRouteObject["middleware"]; loader?: AgnosticIndexRouteObject["loader"]; action?: AgnosticIndexRouteObject["action"]; hasErrorBoundary?: AgnosticIndexRouteObject["hasErrorBoundary"]; @@ -33,6 +34,7 @@ export interface NonIndexRouteObject { caseSensitive?: AgnosticNonIndexRouteObject["caseSensitive"]; path?: AgnosticNonIndexRouteObject["path"]; id?: AgnosticNonIndexRouteObject["id"]; + middleware?: AgnosticNonIndexRouteObject["middleware"]; loader?: AgnosticNonIndexRouteObject["loader"]; action?: AgnosticNonIndexRouteObject["action"]; hasErrorBoundary?: AgnosticNonIndexRouteObject["hasErrorBoundary"]; diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 0b9b39a843..36e3e8742b 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -5262,7 +5262,7 @@ describe("a router", () => { request: createRequest("/tasks", { signal: nav.loaders.tasks.stub.mock.calls[0][0].request.signal, }), - middleware: expect.any(Object), + context: expect.any(Object), }); let nav2 = await t.navigate("/tasks/1"); @@ -5271,7 +5271,7 @@ describe("a router", () => { request: createRequest("/tasks/1", { signal: nav2.loaders.tasksId.stub.mock.calls[0][0].request.signal, }), - middleware: expect.any(Object), + context: expect.any(Object), }); let nav3 = await t.navigate("/tasks?foo=bar#hash"); @@ -5280,7 +5280,7 @@ describe("a router", () => { request: createRequest("/tasks?foo=bar", { signal: nav3.loaders.tasks.stub.mock.calls[0][0].request.signal, }), - middleware: expect.any(Object), + context: expect.any(Object), }); let nav4 = await t.navigate("/tasks#hash", { @@ -5291,7 +5291,7 @@ describe("a router", () => { request: createRequest("/tasks?foo=bar", { signal: nav4.loaders.tasks.stub.mock.calls[0][0].request.signal, }), - middleware: expect.any(Object), + context: expect.any(Object), }); expect(t.router.state.navigation.formAction).toBe("/tasks"); @@ -5688,7 +5688,7 @@ describe("a router", () => { expect(nav.actions.tasks.stub).toHaveBeenCalledWith({ params: {}, request: expect.any(Request), - middleware: expect.any(Object), + context: expect.any(Object), }); // Assert request internals, cannot do a deep comparison above since some @@ -5731,7 +5731,7 @@ describe("a router", () => { expect(nav.actions.tasks.stub).toHaveBeenCalledWith({ params: {}, request: expect.any(Request), - middleware: expect.any(Object), + context: expect.any(Object), }); // Assert request internals, cannot do a deep comparison above since some // internals aren't the same on separate creations @@ -8177,7 +8177,7 @@ describe("a router", () => { request: createRequest("/foo", { signal: A.loaders.root.stub.mock.calls[0][0].request.signal, }), - middleware: expect.any(Object), + context: expect.any(Object), }); }); }); @@ -11539,18 +11539,18 @@ describe("a router", () => { async function trackMiddlewareCall( routeId: string, - { request, middleware }: ActionFunctionArgs | LoaderFunctionArgs + { request, context }: ActionFunctionArgs | LoaderFunctionArgs ) { - let indentation = middleware.get(indentationContext); + let indentation = context.get(indentationContext); let type = request.method === "POST" ? "action" : "loader"; let fetchSuffix = request.url.includes("?from-fetch") ? " (fetch)" : ""; calls.push( `${indentation}${routeId} ${type} middleware start${fetchSuffix}` ); - middleware.set(indentationContext, indentation + " "); + context.set(indentationContext, indentation + " "); await tick(); - let res = await middleware.next(); + let res = await context.next(); calls.push( `${indentation}${routeId} ${type} middleware end${fetchSuffix}` ); @@ -11559,9 +11559,9 @@ describe("a router", () => { async function trackHandlerCall( routeId: string, - { request, middleware }: ActionFunctionArgs | LoaderFunctionArgs + { request, context }: ActionFunctionArgs | LoaderFunctionArgs ) { - let indentation = middleware.get(indentationContext); + let indentation = context.get(indentationContext); let type = request.method === "POST" ? "action" : "loader"; let fetchSuffix = request.url.includes("?from-fetch") ? " (fetch)" : ""; @@ -12077,11 +12077,11 @@ describe("a router", () => { { id: "parent", path: "/parent", - async middleware({ middleware }) { - await middleware.next(); - await middleware.next(); + async middleware({ context }) { + await context.next(); + await context.next(); }, - async loader({ middleware }) { + async loader({ context }) { return "PARENT"; }, }, @@ -12109,11 +12109,11 @@ describe("a router", () => { { id: "parent", path: "/parent", - async middleware({ middleware }) { - return middleware.next(); + async middleware({ context }) { + return context.next(); }, - async loader({ middleware }) { - await middleware.next(); + async loader({ context }) { + await context.next(); return "PARENT"; }, }, @@ -12141,11 +12141,11 @@ describe("a router", () => { { id: "parent", path: "/parent", - async middleware({ middleware }) { - return middleware.next(); + async middleware({ context }) { + return context.next(); }, - async action({ middleware }) { - await middleware.next(); + async action({ context }) { + await context.next(); return "PARENT ACTION"; }, async loader() { @@ -12214,14 +12214,14 @@ describe("a router", () => { { id: "parent", path: "/parent", - loader({ request, middleware }) { + loader({ request, context }) { let sp = new URL(request.url).searchParams; if (sp.has("get")) { - middleware.get(createMiddlewareContext(0)); + context.get(createMiddlewareContext(0)); } else if (sp.has("set")) { - middleware.set(createMiddlewareContext(0), 1); + context.set(createMiddlewareContext(0), 1); } else if (sp.has("next")) { - middleware.next(); + context.next(); } return "PARENT LOADER"; @@ -12260,13 +12260,13 @@ describe("a router", () => { let loaderCountContext = createMiddlewareContext(0); let actionCountContext = createMiddlewareContext(100); - function incrementContextCount(request, middleware) { + function incrementContextCount({ request, context }) { if (request.method === "POST") { - let count = middleware.get(actionCountContext); - middleware.set(actionCountContext, count + 1); + let count = context.get(actionCountContext); + context.set(actionCountContext, count + 1); } else { - let count = middleware.get(loaderCountContext); - middleware.set(loaderCountContext, count + 1); + let count = context.get(loaderCountContext); + context.set(loaderCountContext, count + 1); } } @@ -12275,34 +12275,28 @@ describe("a router", () => { { id: "parent", path: "/parent", - async middleware({ request, middleware }) { - incrementContextCount(request, middleware); - }, - async loader({ middleware }) { - return middleware.get(loaderCountContext); + middleware: incrementContextCount, + async loader({ context }) { + return context.get(loaderCountContext); }, children: [ { id: "child", path: "child", - async middleware({ request, middleware }) { - incrementContextCount(request, middleware); - }, - async loader({ middleware }) { - return middleware.get(loaderCountContext); + middleware: incrementContextCount, + async loader({ context }) { + return context.get(loaderCountContext); }, children: [ { id: "grandchild", path: "grandchild", - async middleware({ request, middleware }) { - incrementContextCount(request, middleware); - }, - async action({ middleware }) { - return middleware.get(actionCountContext); + middleware: incrementContextCount, + async action({ context }) { + return context.get(actionCountContext); }, - async loader({ middleware }) { - return middleware.get(loaderCountContext); + async loader({ context }) { + return context.get(loaderCountContext); }, }, ], @@ -12388,7 +12382,7 @@ describe("a router", () => { }); }); - it("throws if no value is available via middleware.get()", async () => { + it("throws if no value is available via context.get()", async () => { let theContext = createMiddlewareContext(); currentRouter = createRouter({ @@ -12399,8 +12393,8 @@ describe("a router", () => { { id: "broken", path: "broken", - loader({ middleware }) { - return middleware.get(theContext); + loader({ context }) { + return context.get(theContext); }, }, ], @@ -12418,7 +12412,7 @@ describe("a router", () => { `); }); - it("throws if you try to set an undefined value in middleware.set()", async () => { + it("throws if you try to set an undefined value in context.set()", async () => { let theContext = createMiddlewareContext(); currentRouter = createRouter({ @@ -12429,8 +12423,8 @@ describe("a router", () => { { id: "broken", path: "broken", - middleware({ middleware }) { - return middleware.set(theContext, undefined); + middleware({ context }) { + return context.set(theContext, undefined); }, loader() { return "DATA"; @@ -12451,7 +12445,7 @@ describe("a router", () => { `); }); - it("allows null/falsey values in middleware.set()", async () => { + it("allows null/falsey values in context.set()", async () => { let booleanContext = createMiddlewareContext(); let numberContext = createMiddlewareContext(); let stringContext = createMiddlewareContext(); @@ -12465,18 +12459,18 @@ describe("a router", () => { { id: "works", path: "works", - middleware({ middleware }) { - middleware.set(booleanContext, false); - middleware.set(numberContext, 0); - middleware.set(stringContext, ""); - middleware.set(whateverContext, null); + middleware({ context }) { + context.set(booleanContext, false); + context.set(numberContext, 0); + context.set(stringContext, ""); + context.set(whateverContext, null); }, - loader({ middleware }) { + loader({ context }) { return { - boolean: middleware.get(booleanContext), - number: middleware.get(numberContext), - string: middleware.get(stringContext), - whatever: middleware.get(whateverContext), + boolean: context.get(booleanContext), + number: context.get(numberContext), + string: context.get(stringContext), + whatever: context.get(whateverContext), }; }, }, diff --git a/packages/router/index.ts b/packages/router/index.ts index a5c77de03f..bb2bd8e707 100644 --- a/packages/router/index.ts +++ b/packages/router/index.ts @@ -1,6 +1,8 @@ export type { ActionFunction, ActionFunctionArgs, + ActionFunctionWithMiddleware, + ActionFunctionArgsWithMiddleware, AgnosticDataIndexRouteObject, AgnosticDataNonIndexRouteObject, AgnosticDataRouteMatch, @@ -15,6 +17,8 @@ export type { JsonFunction, LoaderFunction, LoaderFunctionArgs, + LoaderFunctionWithMiddleware, + LoaderFunctionArgsWithMiddleware, MiddlewareContext, MiddlewareFunction, MiddlewareFunctionArgs, diff --git a/packages/router/router.ts b/packages/router/router.ts index b7bb3345f8..fcf66dfe15 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1,4 +1,4 @@ -import type { History, Location, Path, To } from "./history"; +import type { Action, History, Location, Path, To } from "./history"; import { Action as HistoryAction, createLocation, @@ -8,6 +8,9 @@ import { } from "./history"; import type { ActionFunction, + ActionFunctionArgs, + ActionFunctionArgsWithMiddleware, + ActionFunctionWithMiddleware, AgnosticDataRouteMatch, AgnosticDataRouteObject, AgnosticRouteMatch, @@ -18,6 +21,9 @@ import type { FormEncType, FormMethod, LoaderFunction, + LoaderFunctionArgs, + LoaderFunctionArgsWithMiddleware, + LoaderFunctionWithMiddleware, MiddlewareContext, MiddlewareContextInstance, MutationFormMethod, @@ -3079,7 +3085,11 @@ async function callRoutePipeline( request: Request, matches: AgnosticDataRouteMatch[], requestContext: unknown, - handler: LoaderFunction | ActionFunction + handler: + | LoaderFunction + | ActionFunction + | LoaderFunctionWithMiddleware + | ActionFunctionWithMiddleware ) { // Avoid memory leaks since we don't control the key let store = new WeakMap(); @@ -3117,7 +3127,11 @@ async function callRouteSubPipeline( params: Params, requestContext: unknown, middlewareContext: MiddlewareContext, - handler: LoaderFunction | ActionFunction + handler: + | LoaderFunction + | ActionFunction + | LoaderFunctionWithMiddleware + | ActionFunctionWithMiddleware ): Promise> { if (request.signal.aborted) { throw new Error("Request aborted"); @@ -3134,8 +3148,8 @@ async function callRouteSubPipeline( return handler({ request, params, - context: requestContext, - middleware: middlewareContext, + context: middlewareContext, + requestContext, }); } @@ -3162,7 +3176,8 @@ async function callRouteSubPipeline( let res = await matches[0].route.middleware({ request, params, - middleware: middlewareContext, + context: middlewareContext, + requestContext, }); if (nextCalled) { @@ -3211,22 +3226,20 @@ async function callLoaderOrAction( // Only call the pipeline for the matches up to this specific match let idx = matches.findIndex((m) => m.route.id === match.route.id); - result = await Promise.race([ - enableMiddleware - ? callRoutePipeline( - request, - matches.slice(0, idx + 1), - requestContext, - handler - ) - : handler({ - request, - params: match.params, - context: requestContext, - middleware: disabledMiddlewareContext, - }), - abortPromise, - ]); + let dataPromise = enableMiddleware + ? callRoutePipeline( + request, + matches.slice(0, idx + 1), + requestContext, + handler + ) + : (handler as LoaderFunction | ActionFunction)({ + request, + params: match.params, + context: requestContext || disabledMiddlewareContext, + }); + + result = await Promise.race([dataPromise, abortPromise]); invariant( result !== undefined, diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 70d7446b18..af30650ee3 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -91,49 +91,85 @@ interface DataFunctionArgs { request: Request; params: Params; context?: any; - middleware: MiddlewareContext; } -/** - * Arguments passed to middleware functions - */ -export interface MiddlewareFunctionArgs extends DataFunctionArgs {} +type DataFunctionReturnValue = + | Promise + | Response + | Promise + | any; /** * Arguments passed to loader functions */ export interface LoaderFunctionArgs extends DataFunctionArgs {} +/** + * Route loader function signature + */ +export interface LoaderFunction { + (args: LoaderFunctionArgs): DataFunctionReturnValue; +} /** * Arguments passed to action functions */ export interface ActionFunctionArgs extends DataFunctionArgs {} /** - * Route loader function signature + * Route action function signature + */ +export interface ActionFunction { + (args: ActionFunctionArgs): DataFunctionReturnValue; +} + +/** + * @private + * Arguments passed to route loader/action functions when middleware is enabled. + */ +interface DataFunctionArgsWithMiddleware { + request: Request; + params: Params; + context: MiddlewareContext; + requestContext: any; +} + +/** + * Arguments passed to middleware functions when middleware is enabled + */ +export interface MiddlewareFunctionArgs + extends DataFunctionArgsWithMiddleware {} + +/** + * Route loader function signature when middleware is enabled */ export interface MiddlewareFunction { - (args: MiddlewareFunctionArgs): Promise | void; + (args: MiddlewareFunctionArgs): DataFunctionReturnValue; } -type DataFunctionReturnValue = - | Promise - | Response - | Promise - | any; +/** + * Arguments passed to loader functions when middleware is enabled + */ +export interface LoaderFunctionArgsWithMiddleware + extends DataFunctionArgsWithMiddleware {} /** - * Route loader function signature + * Route loader function signature when middleware is enabled */ -export interface LoaderFunction { - (args: LoaderFunctionArgs): DataFunctionReturnValue; +export interface LoaderFunctionWithMiddleware { + (args: LoaderFunctionArgsWithMiddleware): DataFunctionReturnValue; } /** - * Route action function signature + * Arguments passed to action functions when middleware is enabled */ -export interface ActionFunction { - (args: ActionFunctionArgs): DataFunctionReturnValue; +export interface ActionFunctionArgsWithMiddleware + extends DataFunctionArgsWithMiddleware {} + +/** + * Route action function signature when middleware is enabled + */ +export interface ActionFunctionWithMiddleware { + (args: ActionFunctionArgsWithMiddleware): DataFunctionReturnValue; } /** @@ -166,8 +202,8 @@ type AgnosticBaseRouteObject = { path?: string; id?: string; middleware?: MiddlewareFunction; - loader?: LoaderFunction; - action?: ActionFunction; + loader?: LoaderFunction | LoaderFunctionWithMiddleware; + action?: ActionFunction | ActionFunctionWithMiddleware; hasErrorBoundary?: boolean; shouldRevalidate?: ShouldRevalidateFunction; handle?: any; From 42cc7c3434ca7522c1995f3eb4d1929824896a8e Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 27 Jan 2023 16:37:09 -0500 Subject: [PATCH 13/30] dont move things around for no reason --- packages/router/utils.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index af30650ee3..eb50360f5a 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -104,16 +104,17 @@ type DataFunctionReturnValue = */ export interface LoaderFunctionArgs extends DataFunctionArgs {} +/** + * Arguments passed to action functions + */ +export interface ActionFunctionArgs extends DataFunctionArgs {} + /** * Route loader function signature */ export interface LoaderFunction { (args: LoaderFunctionArgs): DataFunctionReturnValue; } -/** - * Arguments passed to action functions - */ -export interface ActionFunctionArgs extends DataFunctionArgs {} /** * Route action function signature From d9b143bd1e0ada66d7b898e18dcfe609ba4d8538 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 30 Jan 2023 11:46:17 -0500 Subject: [PATCH 14/30] Fix typo Co-authored-by: Mehdi Achour --- .changeset/hip-geckos-fold.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/hip-geckos-fold.md b/.changeset/hip-geckos-fold.md index 11fec9422c..57190bd59e 100644 --- a/.changeset/hip-geckos-fold.md +++ b/.changeset/hip-geckos-fold.md @@ -4,7 +4,7 @@ Adds support for "middleware" on routes to give you a common place to run before and after your loaders and actions in a single location higher up in the routing tree. The API we landed on is inspired by the middleware API in [Fresh](https://fresh.deno.dev/docs/concepts/middleware) since it supports the concept of nested routes and also allows you to run logic on the response _after_ the fact. -This feature is behind a `future.unstable_middleware` flag at the moment, but major API changes are not expected and we believe it's ready for production usage. This flag allows us to make small "breaking" changes if required users un into unforeseen issues. +This feature is behind a `future.unstable_middleware` flag at the moment, but major API changes are not expected and we believe it's ready for production usage. This flag allows us to make small "breaking" changes if users run into unforeseen issues. To opt into the middleware feature, you pass the flag to your `createBrowserRouter` (or equivalent) method, and then you can define a `middleware` function on your routes: From 4d22813fe2a5e2db1aa75e73614933da19611885 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 31 Jan 2023 09:57:56 -0500 Subject: [PATCH 15/30] Make requestContext optional and fix get/set generics --- packages/router/utils.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index eb50360f5a..828235e9e3 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -131,7 +131,7 @@ interface DataFunctionArgsWithMiddleware { request: Request; params: Params; context: MiddlewareContext; - requestContext: any; + requestContext?: any; } /** @@ -1474,9 +1474,9 @@ export function isRouteErrorResponse(error: any): error is ErrorResponse { * * Supports only key/value for now, eventually will be enhanced */ -export interface MiddlewareContext { - get(key: MiddlewareContextInstance): T; - set(key: MiddlewareContextInstance, value: T): void; +export interface MiddlewareContext { + get(key: MiddlewareContextInstance): T; + set(key: MiddlewareContextInstance, value: T): void; next: () => DataFunctionReturnValue; } From e52c7e0a7d7a6a1efd52a9e3479b9fd8cf409dd5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 31 Jan 2023 12:53:38 -0500 Subject: [PATCH 16/30] Fix (aka ignore) TS error --- packages/router/router.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index fcf66dfe15..e501d2777d 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -3188,10 +3188,13 @@ async function callRouteSubPipeline( } function disabledMiddlewareFn() { - throw new Error("Middleware not enabled (`future.unstable_middleware`)"); + throw new Error( + "Middleware must be enabled via the `future.unstable_middleware` flag)" + ); } const disabledMiddlewareContext: MiddlewareContext = { + // @ts-expect-error get: disabledMiddlewareFn, set: disabledMiddlewareFn, next: disabledMiddlewareFn, From 1e51ffbeac9a23285e32c6b036d84e0f4c37dd50 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 1 Feb 2023 16:17:58 -0500 Subject: [PATCH 17/30] allow pre-populated context for static handler --- .../__tests__/navigation-blocking-test.ts | 10 ++ packages/router/__tests__/router-test.ts | 71 +++++++++- packages/router/index.ts | 1 + packages/router/router.ts | 128 ++++++++---------- packages/router/utils.ts | 49 ++++++- 5 files changed, 185 insertions(+), 74 deletions(-) diff --git a/packages/router/__tests__/navigation-blocking-test.ts b/packages/router/__tests__/navigation-blocking-test.ts index 5b010f8a1f..b2ee52a7f9 100644 --- a/packages/router/__tests__/navigation-blocking-test.ts +++ b/packages/router/__tests__/navigation-blocking-test.ts @@ -14,6 +14,16 @@ const routes = [ describe("navigation blocking", () => { let router: Router; + let warnSpy; + + beforeEach(() => { + warnSpy = jest.spyOn(console, "warn"); + }); + + afterEach(() => { + warnSpy.mockReset(); + }); + it("initializes an 'unblocked' blocker", () => { router = createRouter({ history: createMemoryHistory({ diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 36e3e8742b..11c90f0b8b 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -40,6 +40,7 @@ import type { } from "../utils"; import { AbortedDeferredError, + createMiddlewareStore, isRouteErrorResponse, stripBasename, } from "../utils"; @@ -12234,7 +12235,7 @@ describe("a router", () => { await currentRouter.navigate("/parent?get"); expect(currentRouter.state.errors).toMatchInlineSnapshot(` { - "parent": [Error: Middleware not enabled (\`future.unstable_middleware\`)], + "parent": [Error: Middleware must be enabled via the \`future.unstable_middleware\` flag)], } `); @@ -12242,7 +12243,7 @@ describe("a router", () => { await currentRouter.navigate("/parent?set"); expect(currentRouter.state.errors).toMatchInlineSnapshot(` { - "parent": [Error: Middleware not enabled (\`future.unstable_middleware\`)], + "parent": [Error: Middleware must be enabled via the \`future.unstable_middleware\` flag)], } `); @@ -12250,7 +12251,7 @@ describe("a router", () => { await currentRouter.navigate("/parent?next"); expect(currentRouter.state.errors).toMatchInlineSnapshot(` { - "parent": [Error: Middleware not enabled (\`future.unstable_middleware\`)], + "parent": [Error: Middleware must be enabled via the \`future.unstable_middleware\` flag)], } `); }); @@ -12382,6 +12383,70 @@ describe("a router", () => { }); }); + it("passes context into staticHandler.query", async () => { + let { query } = createStaticHandler(MIDDLEWARE_CONTEXT_ROUTES, { + future: { unstable_middleware: true }, + }); + + let ctx = await query(createRequest("/parent/child/grandchild")); + + if (ctx instanceof Response) { + throw new Error("Unexpected Response"); + } + + expect(ctx.location.pathname).toBe("/parent/child/grandchild"); + expect(ctx.loaderData).toEqual({ + parent: 1, + child: 2, + grandchild: 3, + }); + }); + + it("passes context into staticHandler.queryRoute", async () => { + let { queryRoute } = createStaticHandler(MIDDLEWARE_CONTEXT_ROUTES, { + future: { unstable_middleware: true }, + }); + + let res = await queryRoute(createRequest("/parent/child/grandchild")); + expect(res).toBe(3); + }); + + it("prefills context in staticHandler.query", async () => { + let { query } = createStaticHandler(MIDDLEWARE_CONTEXT_ROUTES, { + future: { unstable_middleware: true }, + }); + + let middlewareContext = createMiddlewareStore(); + middlewareContext.set(loaderCountContext, 50); + let ctx = await query(createRequest("/parent/child/grandchild"), { + middlewareContext, + }); + + if (ctx instanceof Response) { + throw new Error("Unexpected Response"); + } + + expect(ctx.location.pathname).toBe("/parent/child/grandchild"); + expect(ctx.loaderData).toEqual({ + parent: 51, + child: 52, + grandchild: 53, + }); + }); + + it("prefills context in staticHandler.queryRoute", async () => { + let { queryRoute } = createStaticHandler(MIDDLEWARE_CONTEXT_ROUTES, { + future: { unstable_middleware: true }, + }); + + let middlewareContext = createMiddlewareStore(); + middlewareContext.set(loaderCountContext, 50); + let res = await queryRoute(createRequest("/parent/child/grandchild"), { + middlewareContext, + }); + expect(res).toBe(53); + }); + it("throws if no value is available via context.get()", async () => { let theContext = createMiddlewareContext(); diff --git a/packages/router/index.ts b/packages/router/index.ts index bb2bd8e707..06870ed372 100644 --- a/packages/router/index.ts +++ b/packages/router/index.ts @@ -89,4 +89,5 @@ export { DeferredData as UNSAFE_DeferredData, convertRoutesToDataRoutes as UNSAFE_convertRoutesToDataRoutes, getPathContributingMatches as UNSAFE_getPathContributingMatches, + createMiddlewareStore as UNSAFE_createMiddlewareStore, } from "./utils"; diff --git a/packages/router/router.ts b/packages/router/router.ts index e501d2777d..422f367fa7 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -8,8 +8,6 @@ import { } from "./history"; import type { ActionFunction, - ActionFunctionArgs, - ActionFunctionArgsWithMiddleware, ActionFunctionWithMiddleware, AgnosticDataRouteMatch, AgnosticDataRouteObject, @@ -21,11 +19,8 @@ import type { FormEncType, FormMethod, LoaderFunction, - LoaderFunctionArgs, - LoaderFunctionArgsWithMiddleware, LoaderFunctionWithMiddleware, MiddlewareContext, - MiddlewareContextInstance, MutationFormMethod, Params, RedirectResult, @@ -36,6 +31,7 @@ import type { } from "./utils"; import { convertRoutesToDataRoutes, + createMiddlewareStore, DeferredData, ErrorResponse, getPathContributingMatches, @@ -356,6 +352,15 @@ export interface StaticHandlerContext { _deepestRenderedBoundaryId?: string | null; } +interface StaticHandlerQueryOpts { + requestContext?: unknown; + middlewareContext?: MiddlewareContext; +} + +interface StaticHandlerQueryRouteOpts extends StaticHandlerQueryOpts { + routeId?: string; +} + /** * A StaticHandler instance manages a singular SSR navigation/fetch event */ @@ -363,11 +368,11 @@ export interface StaticHandler { dataRoutes: AgnosticDataRouteObject[]; query( request: Request, - opts?: { requestContext?: unknown } + opts?: StaticHandlerQueryOpts ): Promise; queryRoute( request: Request, - opts?: { routeId?: string; requestContext?: unknown } + opts?: StaticHandlerQueryRouteOpts ): Promise; } @@ -2387,7 +2392,7 @@ export function createStaticHandler( */ async function query( request: Request, - { requestContext }: { requestContext?: unknown } = {} + { requestContext, middlewareContext }: StaticHandlerQueryOpts = {} ): Promise { let url = new URL(request.url); let method = request.method.toLowerCase(); @@ -2433,7 +2438,13 @@ export function createStaticHandler( }; } - let result = await queryImpl(request, location, matches, requestContext); + let result = await queryImpl( + request, + location, + matches, + requestContext, + middlewareContext + ); if (isResponse(result)) { return result; } @@ -2469,7 +2480,8 @@ export function createStaticHandler( { routeId, requestContext, - }: { requestContext?: unknown; routeId?: string } = {} + middlewareContext, + }: StaticHandlerQueryRouteOpts = {} ): Promise { let url = new URL(request.url); let method = request.method.toLowerCase(); @@ -2502,6 +2514,7 @@ export function createStaticHandler( location, matches, requestContext, + middlewareContext, match ); if (isResponse(result)) { @@ -2538,6 +2551,7 @@ export function createStaticHandler( location: Location, matches: AgnosticDataRouteMatch[], requestContext: unknown, + middlewareContext?: MiddlewareContext, routeMatch?: AgnosticDataRouteMatch ): Promise | Response> { invariant( @@ -2552,6 +2566,7 @@ export function createStaticHandler( matches, routeMatch || getTargetMatch(matches, location), requestContext, + middlewareContext, routeMatch != null ); return result; @@ -2561,6 +2576,7 @@ export function createStaticHandler( request, matches, requestContext, + middlewareContext, routeMatch ); return isResponse(result) @@ -2594,6 +2610,7 @@ export function createStaticHandler( matches: AgnosticDataRouteMatch[], actionMatch: AgnosticDataRouteMatch, requestContext: unknown, + middlewareContext: MiddlewareContext | undefined, isRouteRequest: boolean ): Promise | Response> { let result: DataResult; @@ -2621,7 +2638,8 @@ export function createStaticHandler( future.unstable_middleware, true, isRouteRequest, - requestContext + requestContext, + middlewareContext ); if (request.signal.aborted) { @@ -2683,6 +2701,7 @@ export function createStaticHandler( request, matches, requestContext, + middlewareContext, undefined, { [boundaryMatch.route.id]: result.error, @@ -2708,7 +2727,12 @@ export function createStaticHandler( redirect: request.redirect, signal: request.signal, }); - let context = await loadRouteData(loaderRequest, matches, requestContext); + let context = await loadRouteData( + loaderRequest, + matches, + requestContext, + middlewareContext + ); return { ...context, @@ -2727,6 +2751,7 @@ export function createStaticHandler( request: Request, matches: AgnosticDataRouteMatch[], requestContext: unknown, + middlewareContext: MiddlewareContext | undefined, routeMatch?: AgnosticDataRouteMatch, pendingActionError?: RouteData ): Promise< @@ -2782,7 +2807,8 @@ export function createStaticHandler( future.unstable_middleware, true, isRouteRequest, - requestContext + requestContext, + middlewareContext ) ), ]); @@ -3081,51 +3107,10 @@ function shouldRevalidateLoader( return arg.defaultShouldRevalidate; } -async function callRoutePipeline( - request: Request, - matches: AgnosticDataRouteMatch[], - requestContext: unknown, - handler: - | LoaderFunction - | ActionFunction - | LoaderFunctionWithMiddleware - | ActionFunctionWithMiddleware -) { - // Avoid memory leaks since we don't control the key - let store = new WeakMap(); - let middlewareContext: MiddlewareContext = { - get(k: MiddlewareContextInstance) { - if (store.has(k)) { - return store.get(k) as T; - } - return k.getDefaultValue(); - }, - set(k: MiddlewareContextInstance, v: T) { - if (typeof v === "undefined") { - throw new Error( - "You cannot set an undefined value in the middleware context" - ); - } - store.set(k, v); - }, - next: () => {}, - }; - - return callRouteSubPipeline( - request, - matches, - matches[0].params, - requestContext, - middlewareContext, - handler - ); -} - async function callRouteSubPipeline( request: Request, matches: AgnosticDataRouteMatch[], params: Params, - requestContext: unknown, middlewareContext: MiddlewareContext, handler: | LoaderFunction @@ -3149,7 +3134,6 @@ async function callRouteSubPipeline( request, params, context: middlewareContext, - requestContext, }); } @@ -3162,7 +3146,6 @@ async function callRouteSubPipeline( request, matches.slice(1), params, - requestContext, middlewareContext, handler ); @@ -3177,7 +3160,6 @@ async function callRouteSubPipeline( request, params, context: middlewareContext, - requestContext, }); if (nextCalled) { @@ -3209,7 +3191,8 @@ async function callLoaderOrAction( enableMiddleware: boolean, isStaticRequest: boolean = false, isRouteRequest: boolean = false, - requestContext?: unknown + requestContext?: unknown, + middlewareContext?: MiddlewareContext ): Promise { let resultType; let result; @@ -3229,18 +3212,23 @@ async function callLoaderOrAction( // Only call the pipeline for the matches up to this specific match let idx = matches.findIndex((m) => m.route.id === match.route.id); - let dataPromise = enableMiddleware - ? callRoutePipeline( - request, - matches.slice(0, idx + 1), - requestContext, - handler - ) - : (handler as LoaderFunction | ActionFunction)({ - request, - params: match.params, - context: requestContext || disabledMiddlewareContext, - }); + let dataPromise; + + if (enableMiddleware) { + dataPromise = callRouteSubPipeline( + request, + matches.slice(0, idx + 1), + matches[0].params, + createMiddlewareStore(middlewareContext), + handler + ); + } else { + dataPromise = (handler as LoaderFunction | ActionFunction)({ + request, + params: match.params, + context: requestContext || disabledMiddlewareContext, + }); + } result = await Promise.race([dataPromise, abortPromise]); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 828235e9e3..de6fbb6f31 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -131,7 +131,6 @@ interface DataFunctionArgsWithMiddleware { request: Request; params: Params; context: MiddlewareContext; - requestContext?: any; } /** @@ -1475,9 +1474,26 @@ export function isRouteErrorResponse(error: any): error is ErrorResponse { * Supports only key/value for now, eventually will be enhanced */ export interface MiddlewareContext { + /** + * Retrieve a value from context + */ get(key: MiddlewareContextInstance): T; + /** + * Set a value from context + */ set(key: MiddlewareContextInstance, value: T): void; + /** + * Call any child middlewares and the destination loader/action + */ next: () => DataFunctionReturnValue; + /** + * @internal + * PRIVATE - DO NOT USE + * + * Return the entries - needed so we can copy values from the serverMiddleware + * context into route-specific contexts + */ + entries(): IterableIterator<[MiddlewareContextInstance, unknown]>; } /** @@ -1510,3 +1526,34 @@ export function createMiddlewareContext( ): MiddlewareContextInstance { return new MiddlewareContextInstance(defaultValue); } + +/** + * @internal + * PRIVATE - DO NOT USE + * + * Create a middleware "context" to store values and provide a next() hook + */ +export function createMiddlewareStore( + initialMiddlewareContext?: MiddlewareContext +) { + let store = new Map(initialMiddlewareContext?.entries()); + let middlewareContext: MiddlewareContext = { + get(k: MiddlewareContextInstance) { + if (store.has(k)) { + return store.get(k) as T; + } + return k.getDefaultValue(); + }, + set(k: MiddlewareContextInstance, v: T) { + if (typeof v === "undefined") { + throw new Error( + "You cannot set an undefined value in the middleware context" + ); + } + store.set(k, v); + }, + next: () => {}, + entries: () => store.entries(), + }; + return middlewareContext; +} From efd2e54bf38907abaa7d383b511b60e80ffac3c5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 2 Feb 2023 15:36:25 -0500 Subject: [PATCH 18/30] bundle bump --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 5edd91e48a..c9a2d7b235 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "42.6 kB" + "none": "43 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13 kB" From 0a853c501c4c0c6526d26604c262e4f42201d078 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 6 Feb 2023 11:18:30 -0500 Subject: [PATCH 19/30] Add future config to react-router create*Router methods --- packages/react-router-dom/index.tsx | 5 +++++ packages/react-router/index.ts | 2 ++ packages/react-router/lib/components.tsx | 1 + packages/router/router.ts | 2 +- 4 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 93dd6961cf..d396d96d9b 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -30,6 +30,7 @@ import type { Fetcher, FormEncType, FormMethod, + FutureConfig, GetScrollRestorationKeyFunction, HashHistory, History, @@ -210,12 +211,14 @@ export function createBrowserRouter( routes: RouteObject[], opts?: { basename?: string; + future?: Partial; hydrationData?: HydrationState; window?: Window; } ): RemixRouter { return createRouter({ basename: opts?.basename, + future: opts?.future, history: createBrowserHistory({ window: opts?.window }), hydrationData: opts?.hydrationData || parseHydrationData(), routes: enhanceManualRouteObjects(routes), @@ -226,12 +229,14 @@ export function createHashRouter( routes: RouteObject[], opts?: { basename?: string; + future?: Partial; hydrationData?: HydrationState; window?: Window; } ): RemixRouter { return createRouter({ basename: opts?.basename, + future: opts?.future, history: createHashHistory({ window: opts?.window }), hydrationData: opts?.hydrationData || parseHydrationData(), routes: enhanceManualRouteObjects(routes), diff --git a/packages/react-router/index.ts b/packages/react-router/index.ts index 23af522352..1ae306d570 100644 --- a/packages/react-router/index.ts +++ b/packages/react-router/index.ts @@ -226,6 +226,7 @@ export function createMemoryRouter( routes: RouteObject[], opts?: { basename?: string; + future?: Partial; hydrationData?: HydrationState; initialEntries?: InitialEntry[]; initialIndex?: number; @@ -233,6 +234,7 @@ export function createMemoryRouter( ): RemixRouter { return createRouter({ basename: opts?.basename, + future: opts?.future, history: createMemoryHistory({ initialEntries: opts?.initialEntries, initialIndex: opts?.initialIndex, diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx index 7b0b74501a..db311cb288 100644 --- a/packages/react-router/lib/components.tsx +++ b/packages/react-router/lib/components.tsx @@ -589,6 +589,7 @@ export function createRoutesFromChildren( element: element.props.element, index: element.props.index, path: element.props.path, + middleware: element.props.middleware, loader: element.props.loader, action: element.props.action, errorElement: element.props.errorElement, diff --git a/packages/router/router.ts b/packages/router/router.ts index 422f367fa7..cbb843dfcd 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -332,7 +332,7 @@ export interface RouterInit { routes: AgnosticRouteObject[]; history: History; hydrationData?: HydrationState; - future?: FutureConfig; + future?: Partial; } /** From d4ecfafe9d00e3e89e1ecf45e0ccfc8c89004a53 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 8 Feb 2023 11:29:44 -0500 Subject: [PATCH 20/30] =?UTF-8?q?Lift=20queryImpl=20=F0=9F=91=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/router/router.ts | 173 ++++++++++++++++++++------------------ 1 file changed, 89 insertions(+), 84 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index cbb843dfcd..9675a1b879 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -2394,6 +2394,11 @@ export function createStaticHandler( request: Request, { requestContext, middlewareContext }: StaticHandlerQueryOpts = {} ): Promise { + invariant( + request.signal, + "query()/queryRoute() requests must contain an AbortController signal" + ); + let url = new URL(request.url); let method = request.method.toLowerCase(); let location = createLocation("", createPath(url), null, "default"); @@ -2438,15 +2443,33 @@ export function createStaticHandler( }; } - let result = await queryImpl( - request, - location, - matches, - requestContext, - middlewareContext - ); - if (isResponse(result)) { - return result; + let result: Omit; + + try { + if (isMutationMethod(request.method.toLowerCase())) { + result = await submit( + request, + matches, + getTargetMatch(matches, location), + requestContext, + middlewareContext, + false + ); + } else { + let loaderResult = await loadRouteData( + request, + matches, + requestContext, + middlewareContext + ); + result = { + ...loaderResult, + actionData: null, + actionHeaders: {}, + }; + } + } catch (e) { + return handleStaticError(e); } // When returning StaticHandlerContext, we patch back in the location here @@ -2483,6 +2506,11 @@ export function createStaticHandler( middlewareContext, }: StaticHandlerQueryRouteOpts = {} ): Promise { + invariant( + request.signal, + "query()/queryRoute() requests must contain an AbortController signal" + ); + let url = new URL(request.url); let method = request.method.toLowerCase(); let location = createLocation("", createPath(url), null, "default"); @@ -2509,16 +2537,35 @@ export function createStaticHandler( throw getInternalRouterError(404, { pathname: location.pathname }); } - let result = await queryImpl( - request, - location, - matches, - requestContext, - middlewareContext, - match - ); - if (isResponse(result)) { - return result; + let result: Omit; + + try { + if (isMutationMethod(request.method.toLowerCase())) { + result = await submit( + request, + matches, + match, + requestContext, + middlewareContext, + true + ); + } else { + let loadersResult = await loadRouteData( + request, + matches, + requestContext, + middlewareContext, + match + ); + + result = { + ...loadersResult, + actionData: null, + actionHeaders: {}, + }; + } + } catch (e) { + return handleStaticError(e); } let error = result.errors ? Object.values(result.errors)[0] : undefined; @@ -2546,65 +2593,6 @@ export function createStaticHandler( return undefined; } - async function queryImpl( - request: Request, - location: Location, - matches: AgnosticDataRouteMatch[], - requestContext: unknown, - middlewareContext?: MiddlewareContext, - routeMatch?: AgnosticDataRouteMatch - ): Promise | Response> { - invariant( - request.signal, - "query()/queryRoute() requests must contain an AbortController signal" - ); - - try { - if (isMutationMethod(request.method.toLowerCase())) { - let result = await submit( - request, - matches, - routeMatch || getTargetMatch(matches, location), - requestContext, - middlewareContext, - routeMatch != null - ); - return result; - } - - let result = await loadRouteData( - request, - matches, - requestContext, - middlewareContext, - routeMatch - ); - return isResponse(result) - ? result - : { - ...result, - actionData: null, - actionHeaders: {}, - }; - } catch (e) { - // If the user threw/returned a Response in callLoaderOrAction, we throw - // it to bail out and then return or throw here based on whether the user - // returned or threw - if (isQueryRouteResponse(e)) { - if (e.type === ResultType.error && !isRedirectResponse(e.response)) { - throw e.response; - } - return e.response; - } - // Redirects are always returned since they don't propagate to catch - // boundaries - if (isRedirectResponse(e)) { - return e; - } - throw e; - } - } - async function submit( request: Request, matches: AgnosticDataRouteMatch[], @@ -2612,7 +2600,7 @@ export function createStaticHandler( requestContext: unknown, middlewareContext: MiddlewareContext | undefined, isRouteRequest: boolean - ): Promise | Response> { + ): Promise> { let result: DataResult; if (!actionMatch.route.action) { @@ -2755,11 +2743,10 @@ export function createStaticHandler( routeMatch?: AgnosticDataRouteMatch, pendingActionError?: RouteData ): Promise< - | Omit< - StaticHandlerContext, - "location" | "basename" | "actionData" | "actionHeaders" - > - | Response + Omit< + StaticHandlerContext, + "location" | "basename" | "actionData" | "actionHeaders" + > > { let isRouteRequest = routeMatch != null; @@ -2861,6 +2848,24 @@ export function createStaticHandler( //#region Helpers //////////////////////////////////////////////////////////////////////////////// +function handleStaticError(e: unknown) { + // If the user threw/returned a Response in callLoaderOrAction, we throw + // it to bail out and then return or throw here based on whether the user + // returned or threw + if (isQueryRouteResponse(e)) { + if (e.type === ResultType.error && !isRedirectResponse(e.response)) { + throw e.response; + } + return e.response; + } + // Redirects are always returned since they don't propagate to catch + // boundaries + if (isRedirectResponse(e)) { + return e; + } + throw e; +} + /** * Given an existing StaticHandlerContext and an error thrown at render time, * provide an updated StaticHandlerContext suitable for a second SSR render From 7ab2f19b96caa265fc8b3cf43509f9c9f2d1a8ce Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 8 Feb 2023 12:32:07 -0500 Subject: [PATCH 21/30] Split submit/loadRouteData by query/queryRoute --- packages/router/__tests__/router-test.ts | 4 +- packages/router/router.ts | 298 +++++++++++++++-------- 2 files changed, 194 insertions(+), 108 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 11c90f0b8b..574df73cb5 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -13179,7 +13179,7 @@ describe("a router", () => { e = _e; } expect(e).toMatchInlineSnapshot( - `[Error: query()/queryRoute() requests must contain an AbortController signal]` + `[Error: query() requests must contain an AbortController signal]` ); }); @@ -14411,7 +14411,7 @@ describe("a router", () => { e = _e; } expect(e).toMatchInlineSnapshot( - `[Error: query()/queryRoute() requests must contain an AbortController signal]` + `[Error: queryRoute() requests must contain an AbortController signal]` ); }); diff --git a/packages/router/router.ts b/packages/router/router.ts index 9675a1b879..548ddeda46 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -2396,7 +2396,7 @@ export function createStaticHandler( ): Promise { invariant( request.signal, - "query()/queryRoute() requests must contain an AbortController signal" + "query() requests must contain an AbortController signal" ); let url = new URL(request.url); @@ -2447,16 +2447,15 @@ export function createStaticHandler( try { if (isMutationMethod(request.method.toLowerCase())) { - result = await submit( + result = await handleQueryAction( request, matches, getTargetMatch(matches, location), requestContext, - middlewareContext, - false + middlewareContext ); } else { - let loaderResult = await loadRouteData( + let loaderResult = await handleQueryLoaders( request, matches, requestContext, @@ -2508,7 +2507,7 @@ export function createStaticHandler( ): Promise { invariant( request.signal, - "query()/queryRoute() requests must contain an AbortController signal" + "queryRoute() requests must contain an AbortController signal" ); let url = new URL(request.url); @@ -2537,84 +2536,64 @@ export function createStaticHandler( throw getInternalRouterError(404, { pathname: location.pathname }); } - let result: Omit; - try { if (isMutationMethod(request.method.toLowerCase())) { - result = await submit( + let result = await handleQueryRouteAction( request, matches, match, requestContext, - middlewareContext, - true - ); - } else { - let loadersResult = await loadRouteData( - request, - matches, - requestContext, - middlewareContext, - match + middlewareContext ); - - result = { - ...loadersResult, - actionData: null, - actionHeaders: {}, - }; + return result; } - } catch (e) { - return handleStaticError(e); - } - let error = result.errors ? Object.values(result.errors)[0] : undefined; - if (error !== undefined) { - // If we got back result.errors, that means the loader/action threw - // _something_ that wasn't a Response, but it's not guaranteed/required - // to be an `instanceof Error` either, so we have to use throw here to - // preserve the "error" state outside of queryImpl. - throw error; - } + let result = await handleQueryRouteLoaders( + request, + matches, + requestContext, + middlewareContext, + match + ); - // Pick off the right state value to return - if (result.actionData) { - return Object.values(result.actionData)[0]; - } + let error = result.errors ? Object.values(result.errors)[0] : undefined; + if (error !== undefined) { + // If we got back result.errors, that means the loader/action threw + // _something_ that wasn't a Response, but it's not guaranteed/required + // to be an `instanceof Error` either, so we have to use throw here to + // preserve the "error" state outside of queryImpl. + throw error; + } - if (result.loaderData) { - let data = Object.values(result.loaderData)[0]; - if (result.activeDeferreds?.[match.route.id]) { - data[UNSAFE_DEFERRED_SYMBOL] = result.activeDeferreds[match.route.id]; + if (result.loaderData) { + let data = Object.values(result.loaderData)[0]; + if (result.activeDeferreds?.[match.route.id]) { + data[UNSAFE_DEFERRED_SYMBOL] = result.activeDeferreds[match.route.id]; + } + return data; } - return data; + } catch (e) { + return handleStaticError(e); } - - return undefined; } - async function submit( + async function handleQueryAction( request: Request, matches: AgnosticDataRouteMatch[], actionMatch: AgnosticDataRouteMatch, requestContext: unknown, - middlewareContext: MiddlewareContext | undefined, - isRouteRequest: boolean + middlewareContext: MiddlewareContext | undefined ): Promise> { let result: DataResult; if (!actionMatch.route.action) { - let error = getInternalRouterError(405, { - method: request.method, - pathname: new URL(request.url).pathname, - routeId: actionMatch.route.id, - }); - if (isRouteRequest) { - throw error; - } result = { type: ResultType.error, - error, + error: getInternalRouterError(405, { + method: request.method, + pathname: new URL(request.url).pathname, + routeId: actionMatch.route.id, + }), }; } else { result = await callLoaderOrAction( @@ -2625,14 +2604,13 @@ export function createStaticHandler( basename, future.unstable_middleware, true, - isRouteRequest, + false, requestContext, middlewareContext ); if (request.signal.aborted) { - let method = isRouteRequest ? "queryRoute" : "query"; - throw new Error(`${method}() call aborted`); + throw new Error(`query() call aborted`); } } @@ -2650,34 +2628,9 @@ export function createStaticHandler( } if (isDeferredResult(result)) { - let error = getInternalRouterError(400, { type: "defer-action" }); - if (isRouteRequest) { - throw error; - } result = { type: ResultType.error, - error, - }; - } - - if (isRouteRequest) { - // Note: This should only be non-Response values if we get here, since - // isRouteRequest should throw any Response received in callLoaderOrAction - if (isErrorResult(result)) { - throw result.error; - } - - return { - matches: [actionMatch], - loaderData: {}, - actionData: { [actionMatch.route.id]: result.data }, - errors: null, - // Note: statusCode + headers are unused here since queryRoute will - // return the raw Response or value - statusCode: 200, - loaderHeaders: {}, - actionHeaders: {}, - activeDeferreds: null, + error: getInternalRouterError(400, { type: "defer-action" }), }; } @@ -2685,12 +2638,11 @@ export function createStaticHandler( // Store off the pending error - we use it to determine which loaders // to call and will commit it when we complete the navigation let boundaryMatch = findNearestBoundary(matches, actionMatch.route.id); - let context = await loadRouteData( + let context = await handleQueryLoaders( request, matches, requestContext, middlewareContext, - undefined, { [boundaryMatch.route.id]: result.error, } @@ -2715,7 +2667,7 @@ export function createStaticHandler( redirect: request.redirect, signal: request.signal, }); - let context = await loadRouteData( + let context = await handleQueryLoaders( loaderRequest, matches, requestContext, @@ -2735,12 +2687,69 @@ export function createStaticHandler( }; } - async function loadRouteData( + async function handleQueryRouteAction( + request: Request, + matches: AgnosticDataRouteMatch[], + actionMatch: AgnosticDataRouteMatch, + requestContext: unknown, + middlewareContext: MiddlewareContext | undefined + ): Promise { + if (!actionMatch.route.action) { + throw getInternalRouterError(405, { + method: request.method, + pathname: new URL(request.url).pathname, + routeId: actionMatch.route.id, + }); + } + + let result = await callLoaderOrAction( + "action", + request, + actionMatch, + matches, + basename, + future.unstable_middleware, + true, + true, + requestContext, + middlewareContext + ); + + if (request.signal.aborted) { + throw new Error(`queryRoute() call aborted`); + } + + if (isRedirectResult(result)) { + // Uhhhh - this should never happen, we should always throw these from + // callLoaderOrAction, but the type narrowing here keeps TS happy and we + // can get back on the "throw all redirect responses" train here should + // this ever happen :/ + throw new Response(null, { + status: result.status, + headers: { + Location: result.location, + }, + }); + } + + if (isDeferredResult(result)) { + throw getInternalRouterError(400, { type: "defer-action" }); + } + + // Note: This should only be non-Response values if we get here, since + // isRouteRequest should throw any Response received in callLoaderOrAction + if (isErrorResult(result)) { + throw result.error; + } + + return result.data; + } + + async function handleQueryLoaders( request: Request, matches: AgnosticDataRouteMatch[], requestContext: unknown, middlewareContext: MiddlewareContext | undefined, - routeMatch?: AgnosticDataRouteMatch, pendingActionError?: RouteData ): Promise< Omit< @@ -2748,10 +2757,93 @@ export function createStaticHandler( "location" | "basename" | "actionData" | "actionHeaders" > > { - let isRouteRequest = routeMatch != null; + let requestMatches = getLoaderMatchesUntilBoundary( + matches, + Object.keys(pendingActionError || {})[0] + ); + let matchesToLoad = requestMatches.filter((m) => m.route.loader); + // Short circuit if we have no loaders to run (query()) + if (matchesToLoad.length === 0) { + return { + matches, + // Add a null for all matched routes for proper revalidation on the client + loaderData: matches.reduce( + (acc, m) => Object.assign(acc, { [m.route.id]: null }), + {} + ), + errors: pendingActionError || null, + statusCode: 200, + loaderHeaders: {}, + activeDeferreds: null, + }; + } + + let results = await Promise.all([ + ...matchesToLoad.map((match) => + callLoaderOrAction( + "loader", + request, + match, + matches, + basename, + future.unstable_middleware, + true, + false, + requestContext, + middlewareContext + ) + ), + ]); + + if (request.signal.aborted) { + throw new Error(`query() call aborted`); + } + + // Process and commit output from loaders + let activeDeferreds = new Map(); + let context = processRouteLoaderData( + matches, + matchesToLoad, + results, + pendingActionError, + activeDeferreds + ); + + // Add a null for any non-loader matches for proper revalidation on the client + let executedLoaders = new Set( + matchesToLoad.map((match) => match.route.id) + ); + matches.forEach((match) => { + if (!executedLoaders.has(match.route.id)) { + context.loaderData[match.route.id] = null; + } + }); + + return { + ...context, + matches, + activeDeferreds: + activeDeferreds.size > 0 + ? Object.fromEntries(activeDeferreds.entries()) + : null, + }; + } + + async function handleQueryRouteLoaders( + request: Request, + matches: AgnosticDataRouteMatch[], + requestContext: unknown, + middlewareContext: MiddlewareContext | undefined, + routeMatch: AgnosticDataRouteMatch + ): Promise< + Omit< + StaticHandlerContext, + "location" | "basename" | "actionData" | "actionHeaders" + > + > { // Short circuit if we have no loaders to run (queryRoute()) - if (isRouteRequest && !routeMatch?.route.loader) { + if (!routeMatch?.route.loader) { throw getInternalRouterError(400, { method: request.method, pathname: new URL(request.url).pathname, @@ -2759,12 +2851,7 @@ export function createStaticHandler( }); } - let requestMatches = routeMatch - ? [routeMatch] - : getLoaderMatchesUntilBoundary( - matches, - Object.keys(pendingActionError || {})[0] - ); + let requestMatches = [routeMatch]; let matchesToLoad = requestMatches.filter((m) => m.route.loader); // Short circuit if we have no loaders to run (query()) @@ -2776,7 +2863,7 @@ export function createStaticHandler( (acc, m) => Object.assign(acc, { [m.route.id]: null }), {} ), - errors: pendingActionError || null, + errors: null, statusCode: 200, loaderHeaders: {}, activeDeferreds: null, @@ -2793,7 +2880,7 @@ export function createStaticHandler( basename, future.unstable_middleware, true, - isRouteRequest, + true, requestContext, middlewareContext ) @@ -2801,8 +2888,7 @@ export function createStaticHandler( ]); if (request.signal.aborted) { - let method = isRouteRequest ? "queryRoute" : "query"; - throw new Error(`${method}() call aborted`); + throw new Error(`queryRoute() call aborted`); } // Process and commit output from loaders @@ -2811,7 +2897,7 @@ export function createStaticHandler( matches, matchesToLoad, results, - pendingActionError, + undefined, activeDeferreds ); From 0b0efe590f2633c9c0088ac40b20e621d45b1da6 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 8 Feb 2023 15:57:58 -0500 Subject: [PATCH 22/30] Dedup query middlewares with render function --- packages/router/__tests__/router-test.ts | 80 +++++++++------ packages/router/router.ts | 120 +++++++++++++++++------ packages/router/utils.ts | 85 ++++++++++++---- 3 files changed, 204 insertions(+), 81 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 574df73cb5..6cd94780cf 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -41,6 +41,7 @@ import type { import { AbortedDeferredError, createMiddlewareStore, + getRouteAwareMiddlewareContext, isRouteErrorResponse, stripBasename, } from "../utils"; @@ -11954,13 +11955,21 @@ describe("a router", () => { future: { unstable_middleware: true }, }); - let context = await query(createRequest("/parent/child/grandchild")); + let context = await query(createRequest("/parent/child/grandchild"), { + render: (context) => { + invariant( + !(context instanceof Response), + "Expected StaticHandlerContext" + ); + return Promise.resolve(json(context.loaderData)); + }, + }); invariant( - !(context instanceof Response), - "Expected StaticHandlerContext" + context instanceof Response, + "Expected Response from query() with render()" ); - expect(context.loaderData).toMatchInlineSnapshot(` + expect(await context.json()).toMatchInlineSnapshot(` { "child": "CHILD LOADER", "grandchild": "GRANDCHILD LOADER", @@ -11970,19 +11979,13 @@ describe("a router", () => { expect(calls).toMatchInlineSnapshot(` [ "parent loader middleware start", - "parent loader middleware start", - "parent loader middleware start", - " parent loader start", " child loader middleware start", - " child loader middleware start", - " parent loader end", - "parent loader middleware end", - " child loader start", " grandchild loader middleware start", - " child loader end", - " child loader middleware end", - "parent loader middleware end", + " parent loader start", + " child loader start", " grandchild loader start", + " parent loader end", + " child loader end", " grandchild loader end", " grandchild loader middleware end", " child loader middleware end", @@ -12094,9 +12097,7 @@ describe("a router", () => { await currentRouter?.navigate("/parent"); expect(currentRouter.state.location.pathname).toBe("/parent"); expect(currentRouter.state.errors).toEqual({ - parent: new Error( - "You may only call `next()` once per middleware and you may not call it in an action or loader" - ), + parent: new Error("You may only call `next()` once per middleware"), }); }); @@ -12127,7 +12128,7 @@ describe("a router", () => { expect(currentRouter.state.location.pathname).toBe("/parent"); expect(currentRouter.state.errors).toEqual({ parent: new Error( - "You may only call `next()` once per middleware and you may not call it in an action or loader" + "You can not call context.next() in a loader or action" ), }); }); @@ -12165,7 +12166,7 @@ describe("a router", () => { expect(currentRouter.state.location.pathname).toBe("/parent"); expect(currentRouter.state.errors).toEqual({ parent: new Error( - "You may only call `next()` once per middleware and you may not call it in an action or loader" + "You can not call context.next() in a loader or action" ), }); }); @@ -12388,14 +12389,17 @@ describe("a router", () => { future: { unstable_middleware: true }, }); - let ctx = await query(createRequest("/parent/child/grandchild")); + let ctx = await query(createRequest("/parent/child/grandchild"), { + render: (context) => { + return Promise.resolve( + json((context as StaticHandlerContext).loaderData) + ); + }, + }); - if (ctx instanceof Response) { - throw new Error("Unexpected Response"); - } + invariant(ctx instanceof Response, "Expected Response"); - expect(ctx.location.pathname).toBe("/parent/child/grandchild"); - expect(ctx.loaderData).toEqual({ + expect(await ctx.json()).toEqual({ parent: 1, child: 2, grandchild: 3, @@ -12417,17 +12421,24 @@ describe("a router", () => { }); let middlewareContext = createMiddlewareStore(); - middlewareContext.set(loaderCountContext, 50); + let routeMiddlewareContext = getRouteAwareMiddlewareContext( + middlewareContext, + -1, + () => {} + ); + routeMiddlewareContext.set(loaderCountContext, 50); let ctx = await query(createRequest("/parent/child/grandchild"), { middlewareContext, + render: (context) => { + return Promise.resolve( + json((context as StaticHandlerContext).loaderData) + ); + }, }); - if (ctx instanceof Response) { - throw new Error("Unexpected Response"); - } + invariant(ctx instanceof Response, "Expected Response"); - expect(ctx.location.pathname).toBe("/parent/child/grandchild"); - expect(ctx.loaderData).toEqual({ + expect(await ctx.json()).toEqual({ parent: 51, child: 52, grandchild: 53, @@ -12440,7 +12451,12 @@ describe("a router", () => { }); let middlewareContext = createMiddlewareStore(); - middlewareContext.set(loaderCountContext, 50); + let routeMiddlewareContext = getRouteAwareMiddlewareContext( + middlewareContext, + -1, + () => {} + ); + routeMiddlewareContext.set(loaderCountContext, 50); let res = await queryRoute(createRequest("/parent/child/grandchild"), { middlewareContext, }); diff --git a/packages/router/router.ts b/packages/router/router.ts index 548ddeda46..3e49a3a37d 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -18,6 +18,7 @@ import type { ErrorResult, FormEncType, FormMethod, + InternalMiddlewareContext, LoaderFunction, LoaderFunctionWithMiddleware, MiddlewareContext, @@ -35,6 +36,7 @@ import { DeferredData, ErrorResponse, getPathContributingMatches, + getRouteAwareMiddlewareContext, isRouteErrorResponse, joinPaths, matchRoutes, @@ -354,7 +356,8 @@ export interface StaticHandlerContext { interface StaticHandlerQueryOpts { requestContext?: unknown; - middlewareContext?: MiddlewareContext; + middlewareContext?: InternalMiddlewareContext; + render?: (context: StaticHandlerContext | Response) => Promise; } interface StaticHandlerQueryRouteOpts extends StaticHandlerQueryOpts { @@ -2392,7 +2395,7 @@ export function createStaticHandler( */ async function query( request: Request, - { requestContext, middlewareContext }: StaticHandlerQueryOpts = {} + { requestContext, middlewareContext, render }: StaticHandlerQueryOpts = {} ): Promise { invariant( request.signal, @@ -2443,21 +2446,67 @@ export function createStaticHandler( }; } + // Since this is a document request, we run middlewares once here for the Request + // so we don't duplicate middleware executions for parallel loaders + if (future.unstable_middleware) { + invariant( + render != null, + "Using middleware with staticHandler.query() requires passing a render() function" + ); + let ctx = middlewareContext || createMiddlewareStore(); + let result = await callRouteSubPipeline( + request, + matches, + 0, + matches[0].params, + ctx, + async () => { + let staticContext = await runQueryHandlers( + request, + location, + matches!, + undefined, + ctx + ); + let response = await render(staticContext); + return response; + } + ); + return result; + } else { + let result = runQueryHandlers( + request, + location, + matches!, + requestContext, + undefined + ); + return result; + } + } + + async function runQueryHandlers( + request: Request, + location: Location, + matches: AgnosticDataRouteMatch[], + requestContext?: unknown, + middlewareContext?: InternalMiddlewareContext + ): Promise { let result: Omit; try { if (isMutationMethod(request.method.toLowerCase())) { result = await handleQueryAction( request, - matches, - getTargetMatch(matches, location), + matches!, + getTargetMatch(matches!, location), requestContext, middlewareContext ); } else { let loaderResult = await handleQueryLoaders( request, - matches, + matches!, requestContext, middlewareContext ); @@ -2582,7 +2631,7 @@ export function createStaticHandler( matches: AgnosticDataRouteMatch[], actionMatch: AgnosticDataRouteMatch, requestContext: unknown, - middlewareContext: MiddlewareContext | undefined + middlewareContext: InternalMiddlewareContext | undefined ): Promise> { let result: DataResult; @@ -2602,7 +2651,7 @@ export function createStaticHandler( actionMatch, matches, basename, - future.unstable_middleware, + false, true, false, requestContext, @@ -2692,7 +2741,7 @@ export function createStaticHandler( matches: AgnosticDataRouteMatch[], actionMatch: AgnosticDataRouteMatch, requestContext: unknown, - middlewareContext: MiddlewareContext | undefined + middlewareContext: InternalMiddlewareContext | undefined ): Promise { if (!actionMatch.route.action) { throw getInternalRouterError(405, { @@ -2749,7 +2798,7 @@ export function createStaticHandler( request: Request, matches: AgnosticDataRouteMatch[], requestContext: unknown, - middlewareContext: MiddlewareContext | undefined, + middlewareContext: InternalMiddlewareContext | undefined, pendingActionError?: RouteData ): Promise< Omit< @@ -2787,7 +2836,7 @@ export function createStaticHandler( match, matches, basename, - future.unstable_middleware, + false, true, false, requestContext, @@ -2834,7 +2883,7 @@ export function createStaticHandler( request: Request, matches: AgnosticDataRouteMatch[], requestContext: unknown, - middlewareContext: MiddlewareContext | undefined, + middlewareContext: InternalMiddlewareContext | undefined, routeMatch: AgnosticDataRouteMatch ): Promise< Omit< @@ -2935,6 +2984,8 @@ export function createStaticHandler( //////////////////////////////////////////////////////////////////////////////// function handleStaticError(e: unknown) { + // TODO: Can this move to queryRoute()? + // If the user threw/returned a Response in callLoaderOrAction, we throw // it to bail out and then return or throw here based on whether the user // returned or threw @@ -3201,8 +3252,9 @@ function shouldRevalidateLoader( async function callRouteSubPipeline( request: Request, matches: AgnosticDataRouteMatch[], + idx: number, params: Params, - middlewareContext: MiddlewareContext, + middlewareContext: InternalMiddlewareContext, handler: | LoaderFunction | ActionFunction @@ -3213,44 +3265,47 @@ async function callRouteSubPipeline( throw new Error("Request aborted"); } - if (matches.length === 0) { + let match = matches[idx]; + + if (!match) { // We reached the end of our middlewares, call the handler - middlewareContext.next = () => { - throw new Error( - "You may only call `next()` once per middleware and you may not call " + - "it in an action or loader" - ); - }; return handler({ request, params, - context: middlewareContext, + context: getRouteAwareMiddlewareContext(middlewareContext, idx, () => { + throw new Error( + "You can not call context.next() in a loader or action" + ); + }), }); } // We've still got matches, continue on the middleware train. The `next()` // function will "bubble" back up the middlewares after handlers have executed let nextCalled = false; - let next: MiddlewareContext["next"] = () => { + let next: InternalMiddlewareContext["next"] = () => { + if (nextCalled) { + throw new Error("You may only call `next()` once per middleware"); + } nextCalled = true; return callRouteSubPipeline( request, - matches.slice(1), + matches, + idx + 1, params, middlewareContext, handler ); }; - if (!matches[0].route.middleware) { + if (!match.route.middleware) { return next(); } - middlewareContext.next = next; - let res = await matches[0].route.middleware({ + let res = await match.route.middleware({ request, params, - context: middlewareContext, + context: getRouteAwareMiddlewareContext(middlewareContext, idx, next), }); if (nextCalled) { @@ -3283,7 +3338,7 @@ async function callLoaderOrAction( isStaticRequest: boolean = false, isRouteRequest: boolean = false, requestContext?: unknown, - middlewareContext?: MiddlewareContext + middlewareContext?: InternalMiddlewareContext ): Promise { let resultType; let result; @@ -3309,15 +3364,22 @@ async function callLoaderOrAction( dataPromise = callRouteSubPipeline( request, matches.slice(0, idx + 1), + 0, matches[0].params, - createMiddlewareStore(middlewareContext), + middlewareContext || createMiddlewareStore(), handler ); } else { dataPromise = (handler as LoaderFunction | ActionFunction)({ request, params: match.params, - context: requestContext || disabledMiddlewareContext, + context: middlewareContext + ? getRouteAwareMiddlewareContext(middlewareContext, idx, () => { + throw new Error( + "You can not call context.next() in a loader or action" + ); + }) + : requestContext || disabledMiddlewareContext, }); } diff --git a/packages/router/utils.ts b/packages/router/utils.ts index de6fbb6f31..94f73bef3e 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -1468,10 +1468,27 @@ export function isRouteErrorResponse(error: any): error is ErrorResponse { ); } +/** + * Internal route-aware context object used to ensure parent loaders can't + * access child middleware values in document requests + */ +export interface InternalMiddlewareContext { + /** + * Retrieve a value from context + */ + get(idx: number, key: MiddlewareContextInstance): T; + /** + * Set a value from context + */ + set(idx: number, key: MiddlewareContextInstance, value: T): void; + /** + * Call any child middlewares and the destination loader/action + */ + next: () => DataFunctionReturnValue; +} + /** * Context object passed through middleware functions and into action/loaders. - * - * Supports only key/value for now, eventually will be enhanced */ export interface MiddlewareContext { /** @@ -1486,14 +1503,6 @@ export interface MiddlewareContext { * Call any child middlewares and the destination loader/action */ next: () => DataFunctionReturnValue; - /** - * @internal - * PRIVATE - DO NOT USE - * - * Return the entries - needed so we can copy values from the serverMiddleware - * context into route-specific contexts - */ - entries(): IterableIterator<[MiddlewareContextInstance, unknown]>; } /** @@ -1533,27 +1542,63 @@ export function createMiddlewareContext( * * Create a middleware "context" to store values and provide a next() hook */ -export function createMiddlewareStore( - initialMiddlewareContext?: MiddlewareContext -) { - let store = new Map(initialMiddlewareContext?.entries()); - let middlewareContext: MiddlewareContext = { - get(k: MiddlewareContextInstance) { +export function createMiddlewareStore() { + let store = new Map(); + let middlewareContext: InternalMiddlewareContext = { + get(idx: number, k: MiddlewareContextInstance) { if (store.has(k)) { - return store.get(k) as T; + let arr = store.get(k) as [number, T][]; + let i = arr.length - 1; + while (i >= 0) { + if (arr[i][0] <= idx) { + let v = arr[i][1]; + return v; + } + i--; + } } return k.getDefaultValue(); }, - set(k: MiddlewareContextInstance, v: T) { + set(idx: number, k: MiddlewareContextInstance, v: T) { if (typeof v === "undefined") { throw new Error( "You cannot set an undefined value in the middleware context" ); } - store.set(k, v); + /* + Document requests make this a bit tricky. Since we want call middlewares + on a per-Request/Response basis, we only want to call them one time on + document request even though we have multiple loaders to call in parallel. + That means the execution looks something like on an /a/b/c document request + where A*, B*, B* are the middlewares: + + A loader + A* -> B* -> C* -> B loader -> HTML Response -> C* -> B* -> A* + C loader + + However, we don't want to expose the results of B's middleware context.set() + calls to A's loader since it's a child of A. So we actually track context + get/set calls by the match index. We associate a set value with an index, + and find the value at or above our own index when calling get above. + */ + let arr: [number, T][] = store.get(k) || []; + arr.push([idx, v]); + store.set(k, arr); }, next: () => {}, - entries: () => store.entries(), }; return middlewareContext; } + +export function getRouteAwareMiddlewareContext( + context: InternalMiddlewareContext, + idx: number, + next: MiddlewareContext["next"] +) { + let routeAwareContext: MiddlewareContext = { + get: (k) => context.get(idx, k), + set: (k, v) => context.set(idx, k, v), + next, + }; + return routeAwareContext; +} From 14c7d4e153e745fda57cbdedc9def0a20566cfee Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 8 Feb 2023 16:30:44 -0500 Subject: [PATCH 23/30] Switch to queryAndRender API --- packages/router/__tests__/router-test.ts | 54 ++++--- packages/router/router.ts | 192 +++++++++++++++-------- 2 files changed, 162 insertions(+), 84 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 6cd94780cf..db181209bf 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -11951,19 +11951,23 @@ describe("a router", () => { }); it("runs middleware before staticHandler.query", async () => { - let { query } = createStaticHandler(MIDDLEWARE_ORDERING_ROUTES, { - future: { unstable_middleware: true }, - }); + let { queryAndRender } = createStaticHandler( + MIDDLEWARE_ORDERING_ROUTES, + { + future: { unstable_middleware: true }, + } + ); - let context = await query(createRequest("/parent/child/grandchild"), { - render: (context) => { + let context = await queryAndRender( + createRequest("/parent/child/grandchild"), + (context) => { invariant( !(context instanceof Response), "Expected StaticHandlerContext" ); return Promise.resolve(json(context.loaderData)); - }, - }); + } + ); invariant( context instanceof Response, @@ -12385,17 +12389,21 @@ describe("a router", () => { }); it("passes context into staticHandler.query", async () => { - let { query } = createStaticHandler(MIDDLEWARE_CONTEXT_ROUTES, { - future: { unstable_middleware: true }, - }); + let { queryAndRender } = createStaticHandler( + MIDDLEWARE_CONTEXT_ROUTES, + { + future: { unstable_middleware: true }, + } + ); - let ctx = await query(createRequest("/parent/child/grandchild"), { - render: (context) => { + let ctx = await queryAndRender( + createRequest("/parent/child/grandchild"), + (context) => { return Promise.resolve( json((context as StaticHandlerContext).loaderData) ); - }, - }); + } + ); invariant(ctx instanceof Response, "Expected Response"); @@ -12416,9 +12424,12 @@ describe("a router", () => { }); it("prefills context in staticHandler.query", async () => { - let { query } = createStaticHandler(MIDDLEWARE_CONTEXT_ROUTES, { - future: { unstable_middleware: true }, - }); + let { queryAndRender } = createStaticHandler( + MIDDLEWARE_CONTEXT_ROUTES, + { + future: { unstable_middleware: true }, + } + ); let middlewareContext = createMiddlewareStore(); let routeMiddlewareContext = getRouteAwareMiddlewareContext( @@ -12427,14 +12438,15 @@ describe("a router", () => { () => {} ); routeMiddlewareContext.set(loaderCountContext, 50); - let ctx = await query(createRequest("/parent/child/grandchild"), { - middlewareContext, - render: (context) => { + let ctx = await queryAndRender( + createRequest("/parent/child/grandchild"), + (context) => { return Promise.resolve( json((context as StaticHandlerContext).loaderData) ); }, - }); + { middlewareContext } + ); invariant(ctx instanceof Response, "Expected Response"); diff --git a/packages/router/router.ts b/packages/router/router.ts index 3e49a3a37d..8dcac7f807 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -356,11 +356,15 @@ export interface StaticHandlerContext { interface StaticHandlerQueryOpts { requestContext?: unknown; +} + +interface StaticHandlerQueryAndRenderOpts { middlewareContext?: InternalMiddlewareContext; - render?: (context: StaticHandlerContext | Response) => Promise; } -interface StaticHandlerQueryRouteOpts extends StaticHandlerQueryOpts { +interface StaticHandlerQueryRouteOpts { + requestContext?: unknown; + middlewareContext?: InternalMiddlewareContext; routeId?: string; } @@ -373,6 +377,11 @@ export interface StaticHandler { request: Request, opts?: StaticHandlerQueryOpts ): Promise; + queryAndRender( + request: Request, + render: (context: StaticHandlerContext | Response) => Promise, + opts?: StaticHandlerQueryAndRenderOpts + ): Promise; queryRoute( request: Request, opts?: StaticHandlerQueryRouteOpts @@ -2395,13 +2404,98 @@ export function createStaticHandler( */ async function query( request: Request, - { requestContext, middlewareContext, render }: StaticHandlerQueryOpts = {} + { requestContext }: StaticHandlerQueryOpts = {} ): Promise { invariant( request.signal, "query() requests must contain an AbortController signal" ); + invariant( + !future.unstable_middleware, + "staticHandler.query() cannot be used with middleware" + ); + + let queryInit = initQueryRequest(request); + + if ("shortCircuitContext" in queryInit) { + return queryInit.shortCircuitContext; + } + + let { location, matches } = queryInit; + + return runQueryHandlers( + request, + location, + matches, + requestContext, + undefined + ); + } + + /** + * The queryAndRender() method is a small extension to query() in which we + * also accept a render() callback allowing our calling context to transform + * the staticHandlerContext int oa singular HTML Response we can bubble back + * up our middleware chain. + */ + async function queryAndRender( + request: Request, + render: (context: StaticHandlerContext | Response) => Promise, + { middlewareContext }: StaticHandlerQueryAndRenderOpts = {} + ): Promise { + invariant( + request.signal, + "query() requests must contain an AbortController signal" + ); + + let queryInit = initQueryRequest(request); + + if ("shortCircuitContext" in queryInit) { + return render(queryInit.shortCircuitContext); + } + + let { location, matches } = queryInit; + + if (!future.unstable_middleware) { + let result = await runQueryHandlers(request, location, matches); + return render(result); + } + + // Since this is a document request, we run middlewares once here for the Request + // so we don't duplicate middleware executions for parallel loaders + invariant( + render != null, + "Using middleware with staticHandler.query() requires passing a render() function" + ); + let ctx = middlewareContext || createMiddlewareStore(); + let result = await callRouteSubPipeline( + request, + matches, + 0, + matches[0].params, + ctx, + async () => { + let staticContext = await runQueryHandlers( + request, + location, + matches, + undefined, + ctx + ); + let response = await render(staticContext); + return response; + } + ); + return result; + } + // Initialize an incoming query() or queryAndRender() call, potentially + // short circuiting if there's nothing to do + function initQueryRequest( + request: Request + ): + | { shortCircuitContext: StaticHandlerContext } + | { location: Location; matches: AgnosticDataRouteMatch[] } { let url = new URL(request.url); let method = request.method.toLowerCase(); let location = createLocation("", createPath(url), null, "default"); @@ -2413,78 +2507,49 @@ export function createStaticHandler( let { matches: methodNotAllowedMatches, route } = getShortCircuitMatches(dataRoutes); return { - basename, - location, - matches: methodNotAllowedMatches, - loaderData: {}, - actionData: null, - errors: { - [route.id]: error, + shortCircuitContext: { + basename, + location, + matches: methodNotAllowedMatches, + loaderData: {}, + actionData: null, + errors: { + [route.id]: error, + }, + statusCode: error.status, + loaderHeaders: {}, + actionHeaders: {}, + activeDeferreds: null, }, - statusCode: error.status, - loaderHeaders: {}, - actionHeaders: {}, - activeDeferreds: null, }; - } else if (!matches) { + } + + if (!matches) { let error = getInternalRouterError(404, { pathname: location.pathname }); let { matches: notFoundMatches, route } = getShortCircuitMatches(dataRoutes); return { - basename, - location, - matches: notFoundMatches, - loaderData: {}, - actionData: null, - errors: { - [route.id]: error, + shortCircuitContext: { + basename, + location, + matches: notFoundMatches, + loaderData: {}, + actionData: null, + errors: { + [route.id]: error, + }, + statusCode: error.status, + loaderHeaders: {}, + actionHeaders: {}, + activeDeferreds: null, }, - statusCode: error.status, - loaderHeaders: {}, - actionHeaders: {}, - activeDeferreds: null, }; } - // Since this is a document request, we run middlewares once here for the Request - // so we don't duplicate middleware executions for parallel loaders - if (future.unstable_middleware) { - invariant( - render != null, - "Using middleware with staticHandler.query() requires passing a render() function" - ); - let ctx = middlewareContext || createMiddlewareStore(); - let result = await callRouteSubPipeline( - request, - matches, - 0, - matches[0].params, - ctx, - async () => { - let staticContext = await runQueryHandlers( - request, - location, - matches!, - undefined, - ctx - ); - let response = await render(staticContext); - return response; - } - ); - return result; - } else { - let result = runQueryHandlers( - request, - location, - matches!, - requestContext, - undefined - ); - return result; - } + return { location, matches }; } + // Run the appropriate handlers for a query() or queryAndRender() call async function runQueryHandlers( request: Request, location: Location, @@ -2973,6 +3038,7 @@ export function createStaticHandler( return { dataRoutes, query, + queryAndRender, queryRoute, }; } From 3f9c7400cd53f9d47972bee4f7d38ad9c9c3d4bb Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 8 Feb 2023 16:32:01 -0500 Subject: [PATCH 24/30] Update createRoutesFromChildren snapshots to reflect middleware --- .../__tests__/createRoutesFromChildren-test.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/react-router/__tests__/createRoutesFromChildren-test.tsx b/packages/react-router/__tests__/createRoutesFromChildren-test.tsx index fafd02c141..280232255e 100644 --- a/packages/react-router/__tests__/createRoutesFromChildren-test.tsx +++ b/packages/react-router/__tests__/createRoutesFromChildren-test.tsx @@ -32,6 +32,7 @@ describe("creating routes from JSX", () => { "id": "0-0", "index": undefined, "loader": undefined, + "middleware": undefined, "path": "home", "shouldRevalidate": undefined, }, @@ -47,6 +48,7 @@ describe("creating routes from JSX", () => { "id": "0-1", "index": undefined, "loader": undefined, + "middleware": undefined, "path": "about", "shouldRevalidate": undefined, }, @@ -66,6 +68,7 @@ describe("creating routes from JSX", () => { "id": "0-2-0", "index": true, "loader": undefined, + "middleware": undefined, "path": undefined, "shouldRevalidate": undefined, }, @@ -81,6 +84,7 @@ describe("creating routes from JSX", () => { "id": "0-2-1", "index": undefined, "loader": undefined, + "middleware": undefined, "path": ":id", "shouldRevalidate": undefined, }, @@ -92,6 +96,7 @@ describe("creating routes from JSX", () => { "id": "0-2", "index": undefined, "loader": undefined, + "middleware": undefined, "path": "users", "shouldRevalidate": undefined, }, @@ -103,6 +108,7 @@ describe("creating routes from JSX", () => { "id": "0", "index": undefined, "loader": undefined, + "middleware": undefined, "path": "/", "shouldRevalidate": undefined, }, @@ -148,6 +154,7 @@ describe("creating routes from JSX", () => { "id": "0-0", "index": undefined, "loader": [Function], + "middleware": undefined, "path": "home", "shouldRevalidate": [Function], }, @@ -167,6 +174,7 @@ describe("creating routes from JSX", () => { "id": "0-1-0", "index": true, "loader": undefined, + "middleware": undefined, "path": undefined, "shouldRevalidate": undefined, }, @@ -178,6 +186,7 @@ describe("creating routes from JSX", () => { "id": "0-1", "index": undefined, "loader": undefined, + "middleware": undefined, "path": "users", "shouldRevalidate": undefined, }, @@ -191,6 +200,7 @@ describe("creating routes from JSX", () => { "id": "0", "index": undefined, "loader": undefined, + "middleware": undefined, "path": "/", "shouldRevalidate": undefined, }, From ff38e9e636d97b8af36906abe2d74daaf28f668a Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 9 Feb 2023 16:14:29 -0500 Subject: [PATCH 25/30] Export UNSAFE_ stuff for use by remix server adaptors --- packages/router/index.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/router/index.ts b/packages/router/index.ts index 06870ed372..dd2c9273c8 100644 --- a/packages/router/index.ts +++ b/packages/router/index.ts @@ -85,9 +85,11 @@ export * from "./router"; /////////////////////////////////////////////////////////////////////////////// /** @internal */ +export type { InternalMiddlewareContext as UNSAFE_InternalMiddlewareContext } from "./utils"; export { DeferredData as UNSAFE_DeferredData, convertRoutesToDataRoutes as UNSAFE_convertRoutesToDataRoutes, getPathContributingMatches as UNSAFE_getPathContributingMatches, createMiddlewareStore as UNSAFE_createMiddlewareStore, + getRouteAwareMiddlewareContext as UNSAFE_getRouteAwareMiddlewareContext, } from "./utils"; From 44b8496da16302ebe3427136f2652256c6dc26cc Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 9 Feb 2023 16:19:47 -0500 Subject: [PATCH 26/30] Bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index c9a2d7b235..82babf0913 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "43 kB" + "none": "44.5 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13 kB" From 4fe7b215a667c55189362496184f1f6e0ff94dd0 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 10 Feb 2023 11:04:52 -0500 Subject: [PATCH 27/30] Update chnageset --- .changeset/hip-geckos-fold.md | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/.changeset/hip-geckos-fold.md b/.changeset/hip-geckos-fold.md index 57190bd59e..9a47b21a50 100644 --- a/.changeset/hip-geckos-fold.md +++ b/.changeset/hip-geckos-fold.md @@ -2,7 +2,11 @@ "@remix-run/router": minor --- -Adds support for "middleware" on routes to give you a common place to run before and after your loaders and actions in a single location higher up in the routing tree. The API we landed on is inspired by the middleware API in [Fresh](https://fresh.deno.dev/docs/concepts/middleware) since it supports the concept of nested routes and also allows you to run logic on the response _after_ the fact. +**Introducing Route Middleware** + +**Proposal**: #9564 + +Adds support for middleware on routes to give you a common place to run before and after your loaders and actions in a single location higher up in the routing tree. The API we landed on is inspired by the middleware API in [Fresh](https://fresh.deno.dev/docs/concepts/middleware) since it supports the concept of nested routes and also allows you to run logic on the response _after_ the fact. This feature is behind a `future.unstable_middleware` flag at the moment, but major API changes are not expected and we believe it's ready for production usage. This flag allows us to make small "breaking" changes if users run into unforeseen issues. @@ -17,13 +21,14 @@ import { import { getSession, commitSession } from "../session"; import { getPosts } from "../posts"; -// Create strongly-typed contexts to use for your middleware data +// 👉 Create strongly-typed contexts to use as keys for your middleware data let userCtx = createMiddlewareContext(null); let sessionCtx = createMiddlewareContext(null); const routes = [ { path: "/", + // 👉 Define middleware on your routes middleware: rootMiddleware, children: [ { @@ -36,6 +41,7 @@ const routes = [ const router = createBrowserRouter(routes, { future: { + // 👉 Enable middleware for your router instance unstable_middleware: true, }, }); @@ -44,16 +50,17 @@ function App() { return ; } +// Middlewares receive a context object with get/set/next methods async function rootMiddleware({ request, context }) { // 🔥 Load common information in one spot in your middleware and make it // available to child middleware/loaders/actions let session = await getSession(request.headers.get("Cookie")); let user = await getUser(session); - middleware.set(userCtx, user); - middleware.set(sessionCtx, session); + context.set(userCtx, user); + context.set(sessionCtx, session); // Call child middleware/loaders/actions - let response = await middleware.next(); + let response = await context.next(); // 🔥 Assign common response headers on the way out response.headers.append("Set-Cookie", await commitSession(session)); @@ -70,4 +77,4 @@ async function childLoader({ context }) { } ``` -⚠️ Please note that middleware is executed on a per-`loader`/`action` basis because they can alter the `Response` from the target `loader`/`action`. This means that if you have 3 `loader`'s being called in parallel on a navigation or revalidation, they will _each_ run any existing root middleware. If these duplicate middleware calls are problematic then you will need to de-dup any problematic logic manually. +⚠️ Please note that middleware is executed on a per-`loader`/`action` basis because even though they may operate on the same Request, they operate on individual `Response` instances from the target `loader`/`action`. This means that if you have 3 `loader`'s being called in parallel on a navigation or revalidation, they will _each_ run any existing ancestor middleware. If these duplicate middleware calls are problematic then you will may need to de-dup middleware side effects manually. From 1cbab31ec4d9b71f3934a648c10c21fa546f24b5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 15 Feb 2023 18:13:52 -0500 Subject: [PATCH 28/30] First stab at middleware docs --- docs/route/middleware.md | 107 +++++++++++++++++++++++++++++++++++++++ docs/route/route.md | 51 +++++++++++++++++++ 2 files changed, 158 insertions(+) create mode 100644 docs/route/middleware.md diff --git a/docs/route/middleware.md b/docs/route/middleware.md new file mode 100644 index 0000000000..80c937c63e --- /dev/null +++ b/docs/route/middleware.md @@ -0,0 +1,107 @@ +--- +title: middleware +new: true +--- + +# `middleware` + +React Router tries to avoid network waterfalls by running loaders in parallel. This can cause some code duplication for logic required across multiple loaders (or actions) such as validating a user session. Middleware is designed to give you a single location to put this type of logic that is shared amongst many loaders/actions. Middleware can be defined on any route and is run _sequentially_ top-down _before_ a loader/action call and then bottom-up _after_ the call. + +Because they are run sequentially, you can easily introduce inadvertent network waterfalls and slow down your page loads and route transitions. Please use carefully! + +This feature only works if using a data router, see [Picking a Router][pickingarouter] + +This feature is currently enabled via a `future.unstable_middleware` flag passed to `createBrowserRouter` + +```tsx [2,6-26,32] +// Context allows strong types on middleware-provided values +let userContext = createMiddlewareContext(); + + { + let user = getUser(request); + + // Provide user object to all child routes + context.set(userContext, user); + + // Continue the Remix request chain running all child middlewares sequentially, + // followed by all matched loaders in parallel. The response from the underlying + // loader is then bubbles back up the middleware chain via the return value. + let response = await context.next(); + + // Set common outgoing headers on all responses + response.headers.set("X-Custom", "Stuff"); + + // Return the altered response + return response; + }} +> + { + // Guaranteed to have a user if this loader runs! + let user = context.get(userContext); + let data = await getProfile(user); + return json(data); + }} + /> +; +``` + +## Arguments + +Middleware receives the same arguments as a `loader`/`action` (`request` and `params`) as well as an additional `context` parameter. `context` behaves a bit like [React Context][react-context] in that you create a context which is the strongly-typed to the value you provide. + +```tsx +let userContext = createMiddlewareContext(); +// context.get(userContext) => Return type is User +// context.set(userContext, user) => Requires `user` be of type User +``` + +When middleware is enabled, this `context` object is also made available to actions and loaders to retrieve values set by middlewares. + +## Logic Flow + +Middleware is designed to solve 4 separate use-cases with a single API to keep the API surface compact: + +- I want to run some logic before a loader +- I want to run some logic after a loader +- I want to run some logic before an action +- I want to run some logic after an action + +To support this we adopted an API inspired by the middleware implementation in [Fresh][fresh-middleware] where the function gives you control over the invocation of child logic, thus allowing you to run logic both before and after the child logic. You can differentiate between loaders and actions based on the `request.method`. + +```tsx +async function middleware({ request, context }) { + // Run me top-down before action/loaders run + // Provide values to descendant middleware + action/loaders + context.set(userContext, getUser(request)); + + // Run descendant middlewares sequentially, followed by the action/loaders + let response = await context.next(); + + // Run me bottom-up after action/loaders run + response.headers.set("X-Custom", "Stuff"); + + // Return the response to ancestor middlewares + return response; +} +``` + +Because middleware has access to the incoming `Request` _and also_ has the ability to mutate the outgoing `Response`, it's important to note that middlewares are executed _per-unique Request/Response combination_. + +In client-side React Router applications, this means that nested routes will execute middlewares _for each loader_ because each loader returns a unique `Response` that could be altered independently by the middleware. + +When navigating to `/a/b`, the following represents the parallel data loading chains: + +``` +a middleware -> a loader -> a middleware +a middleware -> b middleware -> b loader -> b middleware -> a middleware +``` + +So you should be aware that while middleware will reduce some code duplication across your actions/loaders, you may need to leverage a mechanism to dedup external API calls made from within a middleware. + +[pickingarouter]: ../routers/picking-a-router +[react-context]: https://reactjs.org/docs/context.html +[fresh-middleware]: https://fresh.deno.dev/docs/concepts/middleware diff --git a/docs/route/route.md b/docs/route/route.md index b59a98220b..9b1fd22e4a 100644 --- a/docs/route/route.md +++ b/docs/route/route.md @@ -291,6 +291,56 @@ The route action is called when a submission is sent to the route from a [Form][ Please see the [action][action] documentation for more details. +## `middleware` + +React Router tries to avoid network waterfalls by running loaders in parallel. This can cause some code duplication for logic required across multiple loaders (or actions) such as validating a user session. Middleware is designed to give you a single location to put this type of logic that is shared amongst many loaders/actions. Middleware can be defined on any route and is run _both_ top-down _before_ a loader/action call and then bottom-up _after_ the call. + +For example: + +```tsx [2,6-26,32] +// Context allow strong types on middleware-provided values +let userContext = createMiddlewareContext(); + + { + let user = getUser(request); + if (!user) { + // Require login for all child routes of /account + throw redirect("/login"); + } + + // Provide user object to all child routes + context.set(userContext, user); + + // Continue the Remix request chain running all child middlewares sequentially, + // followed by all matched loaders in parallel. The response from the underlying + // loader is then bubbles back up the middleware chain via the return value. + let response = await context.next(); + + // Set common outgoing headers on all responses + response.headers.set("X-Custom", "Stuff"); + + // Return the altered response + return response; + }} +> + { + // Guaranteed to have a user if this loader runs! + let user = context.get(userContext); + let data = await getProfile(user); + return json(data); + }} + /> +; +``` + +If you are not using a data router like [`createBrowserRouter`][createbrowserrouter], this will do nothing + +Please see the [middleware][middleware] documentation for more details. + ## `element` The element to render when the route matches the URL. @@ -334,6 +384,7 @@ Any application-specific data. Please see the [useMatches][usematches] documenta [useloaderdata]: ../hooks/use-loader-data [loader]: ./loader [action]: ./action +[middleware]: ./middleware [errorelement]: ./error-element [form]: ../components/form [fetcher]: ../hooks/use-fetcher From 0b233aaf979588d14137084483b66446980b3d54 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 15 Feb 2023 18:14:58 -0500 Subject: [PATCH 29/30] Remove section from changeset on deduping since it's in docs now --- .changeset/hip-geckos-fold.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/.changeset/hip-geckos-fold.md b/.changeset/hip-geckos-fold.md index 9a47b21a50..2441f0ca23 100644 --- a/.changeset/hip-geckos-fold.md +++ b/.changeset/hip-geckos-fold.md @@ -76,5 +76,3 @@ async function childLoader({ context }) { return redirect(`/posts/${post.id}`); } ``` - -⚠️ Please note that middleware is executed on a per-`loader`/`action` basis because even though they may operate on the same Request, they operate on individual `Response` instances from the target `loader`/`action`. This means that if you have 3 `loader`'s being called in parallel on a navigation or revalidation, they will _each_ run any existing ancestor middleware. If these duplicate middleware calls are problematic then you will may need to de-dup middleware side effects manually. From 1e8a428ba85ab1d345b160f30d7d90501002a0e9 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 15 Feb 2023 18:30:10 -0500 Subject: [PATCH 30/30] Add short circuiting tests --- package.json | 2 +- packages/router/__tests__/router-test.ts | 91 ++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 82babf0913..369536d265 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "44.5 kB" + "none": "44.6 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13 kB" diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index db181209bf..79ad60c55a 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -12588,6 +12588,97 @@ describe("a router", () => { expect(currentRouter.state.errors).toBe(null); }); }); + + describe("short circuiting", () => { + it("short circuits a pipeline if you throw a Redirect from a middleware", async () => { + let middleware = jest.fn(({ request }) => { + if (request.url.endsWith("/a")) { + throw redirect("/b"); + } + }); + let aLoader = jest.fn((arg) => "❌"); + let bLoader = jest.fn((arg) => "✅"); + + currentRouter = createRouter({ + routes: [ + { + path: "/", + middleware, + children: [ + { + path: "a", + loader: aLoader, + }, + { + path: "b", + loader: bLoader, + }, + ], + }, + ], + history: createMemoryHistory(), + future: { unstable_middleware: true }, + }).initialize(); + + await currentRouter.navigate("/a"); + + expect(currentRouter.state.location.pathname).toBe("/b"); + + expect(middleware).toHaveBeenCalledTimes(2); + expect(middleware.mock.calls[0][0].request.url).toEqual( + "http://localhost/a" + ); + expect(middleware.mock.calls[1][0].request.url).toEqual( + "http://localhost/b" + ); + + expect(aLoader).toHaveBeenCalledTimes(0); + expect(bLoader).toHaveBeenCalledTimes(1); + expect(bLoader.mock.calls[0][0].request.url).toEqual( + "http://localhost/b" + ); + }); + + it("short circuits a pipeline if you throw an Error from a middleware", async () => { + let middleware = jest.fn(({ request }) => { + if (request.url.endsWith("/a")) { + throw new Error("💥"); + } + }); + let aLoader = jest.fn((arg) => "✅"); + + currentRouter = createRouter({ + routes: [ + { + path: "/", + middleware, + children: [ + { + path: "a", + loader: aLoader, + }, + ], + }, + ], + history: createMemoryHistory(), + future: { unstable_middleware: true }, + }).initialize(); + + await currentRouter.navigate("/a"); + + expect(currentRouter.state.location.pathname).toBe("/a"); + expect(currentRouter.state.loaderData).toEqual({}); + expect(currentRouter.state.errors).toEqual({ + "0": new Error("💥"), + }); + + expect(middleware).toHaveBeenCalledTimes(1); + expect(middleware.mock.calls[0][0].request.url).toEqual( + "http://localhost/a" + ); + expect(aLoader).toHaveBeenCalledTimes(0); + }); + }); }); describe("ssr", () => {