diff --git a/.changeset/data-strategy.md b/.changeset/data-strategy.md new file mode 100644 index 0000000000..1a70093bcf --- /dev/null +++ b/.changeset/data-strategy.md @@ -0,0 +1,8 @@ +--- +"@remix-run/router": minor +--- + +Add a new `unstable_dataStrategy` configuration option + +- This option allows Data Router applications to take control over the approach for executing route loaders and actions +- The default implementation is today's behavior, to fetch all loaders in parallel, but this option allows users to implement more advanced data flows including Remix single-fetch, middleware/context APIs, automatic loader caching, and more diff --git a/.changeset/skip-action-revalidation.md b/.changeset/skip-action-revalidation.md new file mode 100644 index 0000000000..379d4a7c13 --- /dev/null +++ b/.changeset/skip-action-revalidation.md @@ -0,0 +1,11 @@ +--- +"@remix-run/router": minor +--- + +Add a new `future.unstable_skipActionRevalidation` future flag + +- Currently, active loaders revalidate after any action, regardless of the result +- With this flag enabled, actions that return/throw a 4xx/5xx response status will no longer automatically revalidate +- This should reduce load on your server since it's rare that a 4xx/5xx should actually mutate any data +- If you need to revalidate after a 4xx/5xx result with this flag enabled, you can still do that via returning `true` from `shouldRevalidate` +- `shouldRevalidate` now also receives a new `unstable_actionStatus` argument alongside `actionResult` so you can make decision based on the status of the `action` response without having to encode it into the action data diff --git a/.changeset/static-query-flags.md b/.changeset/static-query-flags.md new file mode 100644 index 0000000000..5bca02eca7 --- /dev/null +++ b/.changeset/static-query-flags.md @@ -0,0 +1,8 @@ +--- +"@remix-run/router": minor +--- + +Added 2 new options to the `staticHandler.query` method for use in Remix's Single Fetch implementation: + +- `loadRouteIds`: An optional array of route IDs to load if you wish to load a subset of the matched routes (useful for fine-grained revalidation) +- `skipLoaderErrorBubbling`: Disable error bubbling on loader executions for single-fetch scenarios where the client-side router will handle the bubbling diff --git a/.gitignore b/.gitignore index 1387c7fdff..7b661bd88a 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ +.DS_Store npm-debug.log /docs/api/ diff --git a/decisions/0003-data-strategy.md b/decisions/0003-data-strategy.md new file mode 100644 index 0000000000..dafb3b4fef --- /dev/null +++ b/decisions/0003-data-strategy.md @@ -0,0 +1,420 @@ +# Data Strategy + +Date: 2024-01-31 + +Status: accepted + +## Context + +In order to implement "Single Fetch" in Remix ([Issue][single-fetch-issue], [RFC][single-fetch-rfc]), we need to expose some level of control over the internal data fetching behaviors of the `@remix-run/router`. This way, while React Router will run loaders in parallel by default, Remix can opt-into making a single fetch call to the server for all loaders. + +## Decisions + +### `dataStrategy` + +To achieve the above, we propose to add an optional `dataStrategy` config which can be passed in by the application. The idea is that `dataStrategy` will accept an array of `matches` to load and will return a parallel array of results for those matches. + +```js +function dataStrategy(arg: DataStrategyFunctionArgs): DataResult[]; + +interface DataStrategyFunctionArgs + extends DataFunctionArgs { + matches: AgnosticDataStrategyMatch[]; +} + +interface DataFunctionArgs { + request: Request; + params: Params; + context?: Context; +} +``` + +There's a [comment][responsibilities-comment] here from Jacob which does a good job of outlining the current responsibilities, but basically React Router in it's current state handles 4 aspects when it comes to executing loaders for a given URL - `dataStrategy` is largely intended to handle step 3: + +1. Match routes for URL +2. Determine what routes to load (via `shouldRevalidate`) +3. Call `loader` functions in parallel +4. Decode Responses + +### Inputs + +The primary input is `matches`, since the user needs to know what routes match and eed to have loaders executed. We also wanted to provide a way for the user to call the "default" internal behavior so they could easily change from parallel to sequential without having to re-invent the wheel and manually call loaders, decode responses, etc. The first idea for this API was to pass a `defaultStrategy(match)` parameter so they could call that per-match: + +```js +function dataStrategy({ matches }) { + // Call in parallel + return Promise.all(matches.map(m => defaultStrategy((m)))); + + // Call sequentially + let results = [] + for (let match of matches) { + results.push(await defaultStrategy(match)) + } + return results; +} +``` + +⚠️ `defaultStrategy` was eliminated in favor of `match.resolve`. + +We also originally intended to expose a `type: 'loader' | 'action`' field as a way to presumably let them call `match.route.loader`/`match.route.action` directly - but we have since decided against that with the `match.resolve` API. + +⚠️ `type` was eliminated in favor of `match.resolve`. + +`dataStrategy` is control _when_ handlers are called, not _how_. RR is in charge of calling them with the right parameters. + +### Outputs + +Originally, we planned on making the `DataResult` API public, which is a union of the different types of results (`SuccessResult`, `ErrorResult`, `RedirectResult`, `DeferResult`). However, as we kept evolving and did some internal refactoring to separate calling loaders from decoding results - we realized that all we really need is a simpler `HandlerResult`: + +```ts +interface HandlerResult { + type: ResultType.success | ResultType.error; + result: any; +} +``` + +If the user returns us one of those per-match, we can internally convert it to a `DataResult`. + +- If `result` is a `Response` then we can handle unwrapping the data and processing any redirects (may produce a `SuccessResult`, `ErrorResult`, or `RedirectResult`) +- If `result` is a `DeferredData` instance, convert to `DeferResult` +- If result is anything else we don't touch the data, it's either a `SuccessResult` or `ErrorResult` based on `type` + - This is important because it's lets the end user opt into a different decoding strategy of their choice. If they return us a Response, we decode it. If not, we don't touch it. + +### Decoding Responses + +Initially, we intended for `dataStrategy` to handle (3), and considered an optional `decodeResponse` API for (4) - but we decided that the decoding of responses was a small enough undertaking using standard Fetch APIs (i.e., `res.json`) that it didn't warrant a custom property - and they could just call those APIs directly. The `defaultStrategy` parameter would handle performing 3 the normal way that RR would. + +⚠️ `decodeResponse` is made obsolete by `HandlerResult` + +### Handling `route.lazy` + +There's a nuanced step we missed in our sequential steps above. If a route was +using `route.lazy`, we may need to load the route before we can execute the `loader`. There's two options here: + +1. We pre-execute all `route.lazy` methods before calling `dataStrategy` +2. We let `dataStrategy` execute them accordingly + +(1) has a pretty glaring perf issue in that it blocks _any_ loaders from running until _all_ `route.lazy`'s have resolved. So if route A is super small but has a slow loader, and route B is large but has a fast loader: + +``` +|-- route a lazy --> |-- route a loader --------------->| +|-- route b lazy ------------------------>|-- route b loader --> | +``` + +This is no bueno. Instead, we want option (2) where the users can run these sequentially per-route - and "loading the route" is just part of the "loading the data" step + +``` +|-- route a lazy -->|-- route a loader ---------------> | +|-- route b lazy ------------------------>|-- route b loader -->| +``` + +Therefore, we're introducing the concept of a `DataStrategyMatch` which is just like a `RouteMatch` but the `match.route` field is a `Promise`. We'll kick off the executions of route.lazy and then you can wait for them to complete prior to calling the loader: + +```js +function dataStrategy({ matches, defaultStrategy }) { + return Promise.all( + matches.map((m) => match.route.then((route) => route.loader(/* ... */))) + ); +} +``` + +There are also statically defined properties that live outside of lazy, so those are extended right onto `match.route`. This allows you to define loaders statically and run them in parallel with `route.lazy`: + +```js +function dataStrategy({ matches, defaultStrategy }) { + // matches[0].route => Promise + // matches[0].route.id => string + // matches[0].route.index => boolean + // matches[0].route.path => string +} +``` + +⚠️ This match.route as a function API was removed in favor of `match.resolve` + +### Handling `shouldRevalidate` behavior + +We considered how to handle `shouldRevalidate` behavior. There's sort of 2 basic approaches: + +1. We pre-filter and only hand the user `matchesToLoad` +2. We hand the user all matches and let them filter + - This would probably also require a new `defaultShouldRevalidate(match) => boolean` parameter passed to `dataStrategy` + +I _think_ (1) is preferred to keep the API at a minimum and avoid leaking into _other_ ways to opt-out of revalidation. We already have an API for that so let's lean into it. + +Additionally, another big con of (2) is that if we want to let them make revalidation decisions inside `dataStrategy` - we need to expose all of the informaiton required for that (`currentUrl`, `currentParams`, `nextUrl`, `nextParams`, `submission` info, `actionResult`, etc.) - the API becomes a mess. + +Therefore we are aiming to stick with one and let `shouldRevalidate` be the only way to opt-out of revalidation. + +### Handling actions and fetchers + +Thus far, we've been mostly concerned with how to handle navigational loaders where they are multiple matched routes and loaders to run. But what about actions and fetchers where we only run a handler for a single leaf match? The quick answer to this is to just send a single-length array with the match in question: + +```js +// loaders +let matchesToLoad = getMatchesToLoad(request, matches); +let results = await dataStrategy({ + request, + params, + matches: matchesToLoad, + type: "loader", + defaultStrategy, +}); + +// action +let actionMatch = getTargetMatch(request, matches); +let actionResults = await dataStrategy({ + request, + params, + matches: [actionMatch], + type: "action", + defaultStrategy, +}); +let actionResult = actionResults[0]; + +// fetcher loader/action +let fetcherMatch = getTargetMatch(request, matches); +let fetcherResults = await dataStrategy({ + request, + params, + matches: [fetcherMatch], + type: "loader", // or "action" + defaultStrategy, +}); +let fetcherResult = fetcherResults[0]; +``` + +This way, the user's implementation can just always operate on the `matches` array and it'll work for all use cases. + +```js +// Sample strategy to run sequentially +async function dataStrategy({ request, params, matches, type }) { + let results = []; + for (let match of matches) { + let result = await match.route[type]({ request, params }); + result.push(result); + } + return results; +} +``` + +### What about middlewares? + +As we thought more and more about this API, it became clear that the concept of "process data for a route" (step 3 above) was not necessarily limited to the `loader`/`action` and that there are data-related APIs on the horizon such as `middleware` and `context` that would also fall under the `dataStrategy` umbrella! In fact, a well-implemented `dataStrategy` could alleviate the need for first-class APIs - even if only initially. Early adopters could use `dataStrategy` to implement their own middlewares and we could see which patterns rise to the top and adopt them as first class `route.middleware` or whatever. + +So how would middleware work? The general idea is that middleware runs sequentially top-down prior to the loaders running. And if you bring `context` into the equation - they also run top down and middlewares/loaders/actions receive the context from their level and above in the tree - but they do not "see" any context from below them in the tree. + +A user-land implementation turns out not to be too bad assuming routes define `middleware`/`context` on `handle`: + +```js +// Assume routes look like this: +let route = { + id: "parent", + path: "/parent", + loader: () => {}, + handle: { + // context can provide multiple keyed contexts + context: { + parent: () => ({ id: "parent" }), + }, + // middleware receives context as an argument + middleware(context) { + context.parent.whatever = "PARENT MIDDLEWARE"; + }, + }, +}; + +async function dataStrategy({ request, params, matches, type }) { + // Run context/middleware sequentially + let contexts = {}; + for (let match of matches) { + if (m.route.handle?.context) { + for (let [id, ctx] of Object.entries(m.route.handle.context)) { + contexts[key] = ctx(); + } + } + if (m.route.handle?.middleware) { + m.route.handle.middleware(context); + } + } + + // Run loaders in parallel (or run the solo action) + return Promise.all( + matches.map(async (m, i) => { + // Only expose contexts from this level and above + let context = matches.slice(0, i + 1).reduce((acc, m) => { + Object.keys(m.route.handle?.context).forEach((k) => { + acc[k] = contexts[k]; + }); + return acc; + }, {}); + try { + return { + type: ResultType.data, + data: await m.route[type]?.({ request, params, context });, + }; + } catch (error) { + return { + type: ResultType.error, + error, + }; + } + }) + ); +} +``` + +❌ Nope - this doesn't actually work! + +Remember above where we decided to _pre-filter_ the matches based on `shouldRevalidate`? That breaks any concept of middleware since even if we don't intend to load a route, we need to run middleware on all parents before the loader. So we _must_ expose at least the `matches` at or above that level in the tree - and more likely _all_ matches to `dataStrategy` if it's to be able to implement middleware. + +And then, once we expose _multiple_ matches - we need to tell the user if they're supposed to actually run the handlers on those matches or only on a leaf/target match. + +I think there's a few options here: + +**Option 1 - `routeMatches` and `handlerMatches`** + +We could add a second array of the "full" set of matches for the route and then middleware would operate on that set, and handlers would operate on the filtered set (renamed to `handlerMatches`) here. This still preserves the pre-filtering and keeps `shouldRevalidate` logic out of `dataStrategy`. + +```js +async function dataStrategy({ request, params, routeMatches, handlerMatches, type }) { + // Run context/middleware sequentially + let contexts = {}; + for (let match of routeMatches) { ... } + + // Run loaders in parallel + return Promise.all( + handlerMatches.map(async (m, i) => { ... }) + ); +} +``` + +**Option 2 - new field on `DataStrategyMatch`** + +Since we're already introducing a concept of a `DataStrategyMatch` to handle `route.lazy`, we could lean into that and expose something on those matches that indicate if they need to have their handler run or not? + +```js +// Inside React Router, assume navigate from /a/ -> /b and we don't need to +// re-run the root loader +let dataStrategyMatches = [{ + route: { id: 'root', loader() {}, ... } + runHandler: false // determined via shouldRevalidate +}, { + route: { id: 'b', loader() {}, ... } + runHandler: true // determined via shouldRevalidate +}] +``` + +Then, the user could use this to differentiate between middlewares and handlers: + +```js +async function dataStrategy({ request, params, matches, type }) { + // Run context/middleware sequentially + let contexts = {}; + for (let match of matches) { ... } + + // Run loaders in parallel + let matchesToLoad = matches.filter(m => m.runHandler); + return Promise.all( + matchesToLoad.map(async (m, i) => { ... }) + ); +} +``` + +**Option 3 - new function on `DataStrategyMatch`** + +Extending on the idea above - it all started to feel super leaky and full of implementation-details. Why are users manually filtering? Or manually passing parameters to loaders/actions? Using a `type` field to know which to call? Waiting on a `match.route` Promise before calling the loader? + +That's wayyyy to many rough edges for us to document and users to get wrong (rightfully so!). + +Why can't we just do it all? Let's wrap _all_ of that up into a single `match.resolve()` function that: + +- Waits for `route.lazy` to resolve (if needed) +- No-ops if the route isn't supposed to revalidate + - Open question here if we return the _current_ data from these no-ops or return `undefined`? + - We decided _not_ to expose this data for now since we don't have a good use case +- Knows whether to call the `loader` or the `action` +- Allows users to pass _additional_ params to loaders/actions for middleware/context use cases. + +```js +// Simplest case - call all loaders in parallel just like current behavior +function dataStrategy({ matches }) { + // No more type, defaultStrategy, or match.route promise APIs! + return Promise.all(matches.map(match => { + // resolve `route.lazy` if needed and call loader/action + return m.resolve(); + }); +} + +// More advanced case - call loader sequentially passing a context through +async function dataStrategy({ matches }) { + let ctx = {}; + let results = []; + for (let match of matches) { + // You can pass a "handlerOverride" function to resolve giving you control + // over how/if to call the handler. The argument passed to `handler` will + // be passed as the second argument to your `loader`/`action`: + // function loader({ request }, ctx) {...} + let result = await m.resolve((handler) => { + return handler(ctx); + }); + results.push(result); + }); + return results; +} + +// More performant case leveraging a middleware type abstraction which lets loaders +// still run in parallel after sequential middlewares: +function dataStrategy({ matches }) { + // Can implement middleware as above since you now get all matches + let context = runMiddlewares(matches); + + // Call all loaders in parallel (no params to pass) but you _can_ pass you + // own argument to `resolve` and it will come in as `loader({ request }, handlerArg)` + // So you can send middleware context through to loaders/actions + return Promise.all(matches.map(match => { + return m.resolve(context); + }); + + // Note we don't do any filtering above - if a match doesn't need to load, + // `match.resolve` is no-op. Just like `serverLoader` is a no-op in `clientLoader` + // when it doesn't need to run +} + +// Advanced case - single-fetch type approach +// More advanced case - call loader sequentially passing a context through +async function dataStrategy({ matches }) { + let singleFetchData = await makeSingleFetchCall() + // Assume we get back: + // { data: { [routeId]: unknown }, errors: { [routeId]: unknown } } + let results = []; + for (let match of matches) { + // Don't even call the handler since we have the data we need from single fetch + let result = await m.resolve(() => { + if (singleFetchData.errors?.[m.route.id]) { + return { + type: 'error', + result: singleFetchData.errors?.[m.route.id] + } + } + return { + type: 'data', + result: singleFetchData.data?.[m.route.id] + } + }); + results.push(result); + }); + return results; +} +``` + +## Status codes + +Initially, we thought we could just let the `handlerOverride`return or throw and then internally we could convert the returned/thrown valuer into a `HandlerResult`. However, this didn't work for the `unstable_skipActionRevalidation` behavior we wanted to implement with Single Fetch. + +If users returned normal Response's it would be fine, since we could decode the response internally and also know the status. However, if user's wanted to do custom response decoding (i.e., use `turbo-stream` like we did in single fetch) then there was no way to return/throw data _and the status code from the response_ without introducing something like the `ErrorResponse` API which holds a status and data. We decided to make `HandlerResult` public API and put an optional `status` field on it. + +This means that if you just call resolve with no `handlerOverride` you never need to know about `HandlerResult`. If you do pass a `handlerOverride`, then you need to return a proper HandlerResult with `type:"data"|"error"`. + +[single-fetch-issue]: https://github.com/remix-run/remix/issues/7641 +[single-fetch-rfc]: https://github.com/remix-run/remix/discussions/7640 +[responsibilities-comment]: https://github.com/remix-run/remix/issues/7641#issuecomment-1836635069 diff --git a/docs/route/should-revalidate.md b/docs/route/should-revalidate.md index 38389275a3..04c58a330c 100644 --- a/docs/route/should-revalidate.md +++ b/docs/route/should-revalidate.md @@ -25,6 +25,7 @@ interface ShouldRevalidateFunctionArgs { formData?: Submission["formData"]; json?: Submission["json"]; actionResult?: any; + unstable_actionStatus?: number; defaultShouldRevalidate: boolean; } ``` diff --git a/package.json b/package.json index 35e7d49de0..8aa9b98cad 100644 --- a/package.json +++ b/package.json @@ -116,19 +116,19 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "50.4 kB" + "none": "52.4 kB" }, "packages/react-router/dist/react-router.production.min.js": { - "none": "14.73 kB" + "none": "14.8 kB" }, "packages/react-router/dist/umd/react-router.production.min.js": { "none": "17.2 kB" }, "packages/react-router-dom/dist/react-router-dom.production.min.js": { - "none": "17.0 kB" + "none": "17.1 kB" }, "packages/react-router-dom/dist/umd/react-router-dom.production.min.js": { - "none": "23.2 kB" + "none": "23.5 kB" } } } diff --git a/packages/react-router-dom-v5-compat/index.ts b/packages/react-router-dom-v5-compat/index.ts index ae07125d13..bf967ed5f1 100644 --- a/packages/react-router-dom-v5-compat/index.ts +++ b/packages/react-router-dom-v5-compat/index.ts @@ -51,6 +51,9 @@ export type { ActionFunctionArgs, AwaitProps, BrowserRouterProps, + unstable_DataStrategyFunction, + unstable_DataStrategyFunctionArgs, + unstable_DataStrategyMatch, DataRouteMatch, DataRouteObject, ErrorResponse, @@ -109,6 +112,7 @@ export type { UIMatch, Blocker, BlockerFunction, + unstable_HandlerResult, } from "./react-router-dom"; export { AbortedDeferredError, diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index b30ed811ae..1d4675396e 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -36,6 +36,9 @@ import { } from "react-router"; import type { BrowserHistory, + unstable_DataStrategyFunction, + unstable_DataStrategyFunctionArgs, + unstable_DataStrategyMatch, Fetcher, FormEncType, FormMethod, @@ -83,6 +86,9 @@ import { //////////////////////////////////////////////////////////////////////////////// export type { + unstable_DataStrategyFunction, + unstable_DataStrategyFunctionArgs, + unstable_DataStrategyMatch, FormEncType, FormMethod, GetScrollRestorationKeyFunction, @@ -91,7 +97,7 @@ export type { URLSearchParamsInit, V7_FormMethod, }; -export { createSearchParams }; +export { createSearchParams, ErrorResponseImpl as UNSAFE_ErrorResponseImpl }; // Note: Keep in sync with react-router exports! export type { @@ -143,6 +149,7 @@ export type { ShouldRevalidateFunctionArgs, To, UIMatch, + unstable_HandlerResult, } from "react-router"; export { AbortedDeferredError, @@ -248,6 +255,7 @@ interface DOMRouterOpts { basename?: string; future?: Partial>; hydrationData?: HydrationState; + unstable_dataStrategy?: unstable_DataStrategyFunction; window?: Window; } @@ -265,6 +273,7 @@ export function createBrowserRouter( hydrationData: opts?.hydrationData || parseHydrationData(), routes, mapRouteProperties, + unstable_dataStrategy: opts?.unstable_dataStrategy, window: opts?.window, }).initialize(); } @@ -283,6 +292,7 @@ export function createHashRouter( hydrationData: opts?.hydrationData || parseHydrationData(), routes, mapRouteProperties, + unstable_dataStrategy: opts?.unstable_dataStrategy, window: opts?.window, }).initialize(); } diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index 0e07d98d35..9fcb91b173 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -315,6 +315,7 @@ export function createStaticRouter( v7_partialHydration: opts.future?.v7_partialHydration === true, v7_prependBasename: false, v7_relativeSplatPath: opts.future?.v7_relativeSplatPath === true, + unstable_skipActionErrorRevalidation: false, }; }, get state() { diff --git a/packages/react-router-native/index.tsx b/packages/react-router-native/index.tsx index f88de93b3d..562a298540 100644 --- a/packages/react-router-native/index.tsx +++ b/packages/react-router-native/index.tsx @@ -27,6 +27,9 @@ export type { BlockerFunction, DataRouteMatch, DataRouteObject, + unstable_DataStrategyFunction, + unstable_DataStrategyFunctionArgs, + unstable_DataStrategyMatch, ErrorResponse, Fetcher, FutureConfig, @@ -68,6 +71,7 @@ export type { ShouldRevalidateFunctionArgs, To, UIMatch, + unstable_HandlerResult, } from "react-router"; export { AbortedDeferredError, diff --git a/packages/react-router/__tests__/data-memory-router-test.tsx b/packages/react-router/__tests__/data-memory-router-test.tsx index 7946c8b95c..4a4826dd2c 100644 --- a/packages/react-router/__tests__/data-memory-router-test.tsx +++ b/packages/react-router/__tests__/data-memory-router-test.tsx @@ -30,6 +30,7 @@ import { useRouteLoaderData, } from "react-router"; +import urlDataStrategy from "../../router/__tests__/utils/urlDataStrategy"; import { createDeferred } from "../../router/__tests__/utils/utils"; import MemoryNavigate from "./utils/MemoryNavigate"; import getHtml from "./utils/getHtml"; @@ -2019,6 +2020,7 @@ describe("createMemoryRouter", () => { { path: "/", Component() { + // eslint-disable-next-line no-throw-literal throw null; }, ErrorBoundary() { @@ -3165,4 +3167,110 @@ describe("createMemoryRouter", () => { `); }); }); + + describe("router dataStrategy", () => { + it("executes route loaders on navigation", async () => { + let barDefer = createDeferred(); + let router = createMemoryRouter( + createRoutesFromElements( + }> + } /> + barDefer.promise} + element={} + /> + + ), + { initialEntries: ["/foo"], unstable_dataStrategy: urlDataStrategy } + ); + let { container } = render(); + + function Layout() { + let navigation = useNavigation(); + return ( +
+ Link to Bar +

{navigation.state}

+ +
+ ); + } + + function Foo() { + return

Foo

; + } + function Bar() { + let data = useLoaderData() as URLSearchParams; + return

{data?.get("message")}

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

+ idle +

+

+ Foo +

+
+
" + `); + + fireEvent.click(screen.getByText("Link to Bar")); + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+
+ + Link to Bar + +

+ loading +

+

+ Foo +

+
+
" + `); + + // barDefer.resolve({ message: "Bar Loader" }); + barDefer.resolve( + new Response( + new URLSearchParams([["message", "Bar Loader"]]).toString(), + { + headers: { + "Content-Type": "application/x-www-form-urlencoded", + }, + } + ) + ); + await waitFor(() => screen.getByText("idle")); + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+
+ + Link to Bar + +

+ idle +

+

+ Bar Loader +

+
+
" + `); + }); + }); }); diff --git a/packages/react-router/index.ts b/packages/react-router/index.ts index fb7b787797..b6d85af431 100644 --- a/packages/react-router/index.ts +++ b/packages/react-router/index.ts @@ -4,6 +4,9 @@ import type { ActionFunctionArgs, Blocker, BlockerFunction, + unstable_DataStrategyFunction, + unstable_DataStrategyFunctionArgs, + unstable_DataStrategyMatch, ErrorResponse, Fetcher, HydrationState, @@ -28,6 +31,7 @@ import type { ShouldRevalidateFunctionArgs, To, UIMatch, + unstable_HandlerResult, } from "@remix-run/router"; import { AbortedDeferredError, @@ -130,6 +134,9 @@ export type { AwaitProps, DataRouteMatch, DataRouteObject, + unstable_DataStrategyFunction, + unstable_DataStrategyFunctionArgs, + unstable_DataStrategyMatch, ErrorResponse, Fetcher, FutureConfig, @@ -173,6 +180,7 @@ export type { UIMatch, Blocker, BlockerFunction, + unstable_HandlerResult, }; export { AbortedDeferredError, @@ -288,6 +296,7 @@ export function createMemoryRouter( hydrationData?: HydrationState; initialEntries?: InitialEntry[]; initialIndex?: number; + unstable_dataStrategy?: unstable_DataStrategyFunction; } ): RemixRouter { return createRouter({ @@ -303,6 +312,7 @@ export function createMemoryRouter( hydrationData: opts?.hydrationData, routes, mapRouteProperties, + unstable_dataStrategy: opts?.unstable_dataStrategy, }).initialize(); } diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 0feda4720f..37697ee647 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -697,7 +697,7 @@ export function _renderMatches( let errors = dataRouterState?.errors; if (errors != null) { let errorIndex = renderedMatches.findIndex( - (m) => m.route.id && errors?.[m.route.id] + (m) => m.route.id && errors?.[m.route.id] !== undefined ); invariant( errorIndex >= 0, diff --git a/packages/router/__tests__/data-strategy-test.ts b/packages/router/__tests__/data-strategy-test.ts new file mode 100644 index 0000000000..6cdff873bc --- /dev/null +++ b/packages/router/__tests__/data-strategy-test.ts @@ -0,0 +1,1244 @@ +import type { DataStrategyFunction, DataStrategyMatch } from "../utils"; +import { json } from "../utils"; +import { createDeferred, setup } from "./utils/data-router-setup"; +import { createFormData, tick } from "./utils/utils"; + +describe("router dataStrategy", () => { + function mockDataStrategy(fn: DataStrategyFunction) { + return jest.fn< + ReturnType, + Parameters + >(fn); + } + + describe("loaders", () => { + it("should allow a custom implementation to passthrough to default behavior", async () => { + let dataStrategy = mockDataStrategy(({ matches }) => + Promise.all(matches.map((m) => m.resolve())) + ); + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "json", + path: "/test", + loader: true, + children: [ + { + id: "text", + index: true, + loader: true, + }, + ], + }, + ], + dataStrategy, + }); + + let A = await t.navigate("/test"); + + // Should be called in parallel + expect(A.loaders.json.stub).toHaveBeenCalledTimes(1); + expect(A.loaders.text.stub).toHaveBeenCalledTimes(1); + + await A.loaders.json.resolve(json({ message: "hello json" })); + await A.loaders.text.resolve(new Response("hello text")); + + expect(t.router.state.loaderData).toEqual({ + json: { message: "hello json" }, + text: "hello text", + }); + expect(dataStrategy).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + params: {}, + matches: [ + expect.objectContaining({ + route: expect.objectContaining({ + id: "json", + }), + }), + expect.objectContaining({ + route: expect.objectContaining({ + id: "text", + }), + }), + ], + }) + ); + }); + + it("should allow a custom implementation to passthrough to default behavior and lazy", async () => { + let dataStrategy = mockDataStrategy(({ matches }) => + Promise.all(matches.map((m) => m.resolve())) + ); + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "json", + path: "/test", + lazy: true, + children: [ + { + id: "text", + index: true, + lazy: true, + }, + ], + }, + ], + dataStrategy, + }); + + let A = await t.navigate("/test"); + await A.lazy.json.resolve({ + loader: () => ({ message: "hello json" }), + }); + await A.lazy.text.resolve({ + loader: () => "hello text", + }); + expect(t.router.state.loaderData).toEqual({ + json: { message: "hello json" }, + text: "hello text", + }); + expect(dataStrategy).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + matches: [ + expect.objectContaining({ + route: expect.objectContaining({ + id: "json", + }), + }), + expect.objectContaining({ + route: expect.objectContaining({ + id: "text", + }), + }), + ], + }) + ); + }); + + it("should allow custom implementations to override default behavior", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "test", + path: "/test", + loader: true, + }, + ], + async dataStrategy({ matches }) { + return Promise.all( + matches.map((m) => + m.resolve(async (handler) => { + let result = await handler(); + return { + type: "data", + result: `Route ID "${m.route.id}" returned "${result}"`, + }; + }) + ) + ); + }, + }); + + let A = await t.navigate("/test"); + await A.loaders.test.resolve("TEST"); + + expect(t.router.state.loaderData).toMatchObject({ + test: 'Route ID "test" returned "TEST"', + }); + }); + + it("should allow custom implementations to override default behavior with lazy", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "test", + path: "/test", + lazy: true, + }, + ], + async dataStrategy({ matches }) { + return Promise.all( + matches.map((m) => + m.resolve(async (handler) => { + let result = await handler(); + return { + type: "data", + result: `Route ID "${m.route.id}" returned "${result}"`, + }; + }) + ) + ); + }, + }); + + let A = await t.navigate("/test"); + await A.lazy.test.resolve({ loader: () => "TEST" }); + + expect(t.router.state.loaderData).toMatchObject({ + test: 'Route ID "test" returned "TEST"', + }); + }); + + it("handles errors at the proper boundary", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + path: "/parent", + children: [ + { + id: "child", + path: "child", + hasErrorBoundary: true, + children: [ + { + id: "test", + index: true, + loader: true, + }, + ], + }, + ], + }, + ], + dataStrategy({ matches }) { + return Promise.all(matches.map(async (match) => match.resolve())); + }, + }); + + let A = await t.navigate("/parent/child"); + await A.loaders.test.reject(new Error("ERROR")); + + expect(t.router.state.loaderData.test).toBeUndefined(); + expect(t.router.state.errors?.child.message).toBe("ERROR"); + }); + + it("handles errors at the proper boundary with a custom implementation", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + path: "/parent", + children: [ + { + id: "child", + path: "child", + hasErrorBoundary: true, + children: [ + { + id: "test", + index: true, + loader: true, + }, + ], + }, + ], + }, + ], + dataStrategy({ matches }) { + return Promise.all( + matches.map((match) => { + return match.resolve(async (handler) => { + return { + type: "data", + result: await handler(), + }; + }); + }) + ); + }, + }); + + let A = await t.navigate("/parent/child"); + await A.loaders.test.reject(new Error("ERROR")); + + expect(t.router.state.loaderData.test).toBeUndefined(); + expect(t.router.state.errors?.child.message).toBe("ERROR"); + }); + + it("bubbles to the root if dataStrategy throws", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "parent", + path: "/parent", + loader: true, + children: [ + { + id: "child", + path: "child", + loader: true, + }, + ], + }, + ], + dataStrategy({ matches }) { + throw new Error("Uh oh"); + }, + }); + + let A = await t.navigate("/parent/child"); + await A.loaders.parent.resolve("PARENT"); + + expect(t.router.state).toMatchObject({ + actionData: null, + errors: { + parent: new Error("Uh oh"), + }, + loaderData: {}, + navigation: { + state: "idle", + }, + }); + }); + + it("throws an error if an implementation does not call resolve", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "parent", + path: "/parent", + loader: true, + hasErrorBoundary: true, + children: [ + { + id: "child", + path: "child", + lazy: true, + }, + ], + }, + ], + // @ts-expect-error + dataStrategy({ matches }) { + return Promise.all( + matches.map(async (match) => { + if (match.route.id === "child") { + // noop to cause error + return "forgot to load child"; + } + return match.resolve(); + }) + ); + }, + }); + + let A = await t.navigate("/parent/child"); + await A.loaders.parent.resolve("PARENT"); + + expect(t.router.state).toMatchObject({ + actionData: null, + errors: { + parent: new Error( + '`match.resolve()` was not called for route id "child". ' + + "You must call `match.resolve()` on every match passed " + + "to `dataStrategy` to ensure all routes are properly loaded." + ), + }, + loaderData: { + child: undefined, + parent: undefined, + }, + navigation: { + state: "idle", + }, + }); + }); + + it("indicates which routes need to load via match.shouldLoad", async () => { + let dataStrategy = jest.fn< + ReturnType, + Parameters + >(({ matches }) => { + return Promise.all(matches.map((m) => m.resolve())); + }); + let t = setup({ + routes: [ + { + id: "root", + path: "/", + loader: true, + children: [ + { + id: "parent", + path: "parent", + loader: true, + action: true, + children: [ + { + id: "child", + path: "child", + lazy: true, + }, + ], + }, + ], + }, + ], + dataStrategy, + hydrationData: { + // don't call dataStrategy on hydration + loaderData: { root: null }, + }, + }); + + let A = await t.navigate("/"); + expect(dataStrategy.mock.calls[0][0].matches).toEqual([ + expect.objectContaining({ + shouldLoad: true, + route: expect.objectContaining({ id: "root" }), + }), + ]); + await A.loaders.root.resolve("ROOT"); + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT", + }); + + let B = await t.navigate("/parent"); + expect(dataStrategy.mock.calls[1][0].matches).toEqual([ + expect.objectContaining({ + shouldLoad: false, + route: expect.objectContaining({ id: "root" }), + }), + expect.objectContaining({ + shouldLoad: true, + route: expect.objectContaining({ id: "parent" }), + }), + ]); + await B.loaders.parent.resolve("PARENT"); + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT", + parent: "PARENT", + }); + + let C = await t.navigate("/parent/child"); + expect(dataStrategy.mock.calls[2][0].matches).toEqual([ + expect.objectContaining({ + shouldLoad: false, + route: expect.objectContaining({ id: "root" }), + }), + expect.objectContaining({ + shouldLoad: false, + route: expect.objectContaining({ id: "parent" }), + }), + expect.objectContaining({ + shouldLoad: true, + route: expect.objectContaining({ id: "child" }), + }), + ]); + await C.lazy.child.resolve({ + action: () => "CHILD ACTION", + loader: () => "CHILD", + shouldRevalidate: () => false, + }); + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT", + parent: "PARENT", + child: "CHILD", + }); + + await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({}), + }); + await tick(); + expect(dataStrategy.mock.calls[3][0].matches).toEqual([ + expect.objectContaining({ + shouldLoad: false, + route: expect.objectContaining({ id: "root" }), + }), + expect.objectContaining({ + shouldLoad: false, + route: expect.objectContaining({ id: "parent" }), + }), + expect.objectContaining({ + shouldLoad: true, // action + route: expect.objectContaining({ id: "child" }), + }), + ]); + expect(dataStrategy.mock.calls[4][0].matches).toEqual([ + expect.objectContaining({ + shouldLoad: true, + route: expect.objectContaining({ id: "root" }), + }), + expect.objectContaining({ + shouldLoad: true, + route: expect.objectContaining({ id: "parent" }), + }), + expect.objectContaining({ + shouldLoad: false, // shouldRevalidate=false + route: expect.objectContaining({ id: "child" }), + }), + ]); + expect(t.router.state.actionData).toMatchObject({ + child: "CHILD ACTION", + }); + expect(t.router.state.loaderData).toMatchObject({ + root: "ROOT", + parent: "PARENT", + child: "CHILD", + }); + }); + }); + + describe("actions", () => { + it("should allow a custom implementation to passthrough to default behavior", async () => { + let dataStrategy = mockDataStrategy(({ matches }) => + Promise.all(matches.map((m) => m.resolve())) + ); + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "json", + path: "/test", + action: true, + }, + ], + dataStrategy, + }); + + let A = await t.navigate("/test", { + formMethod: "post", + formData: createFormData({}), + }); + + await A.actions.json.resolve(json({ message: "hello json" })); + + expect(t.router.state.actionData).toEqual({ + json: { message: "hello json" }, + }); + expect(dataStrategy).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + matches: [ + expect.objectContaining({ + route: expect.objectContaining({ + id: "json", + }), + }), + ], + }) + ); + }); + + it("should allow a custom implementation to passthrough to default behavior and lazy", async () => { + let dataStrategy = mockDataStrategy(({ matches }) => + Promise.all(matches.map((m) => m.resolve())) + ); + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "json", + path: "/test", + lazy: true, + }, + ], + dataStrategy, + }); + + let A = await t.navigate("/test", { + formMethod: "post", + formData: createFormData({}), + }); + await A.lazy.json.resolve({ + action: () => ({ message: "hello json" }), + }); + expect(t.router.state.actionData).toEqual({ + json: { message: "hello json" }, + }); + expect(dataStrategy).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + matches: [ + expect.objectContaining({ + route: expect.objectContaining({ + id: "json", + }), + }), + ], + }) + ); + }); + }); + + describe("fetchers", () => { + describe("loaders", () => { + it("should allow a custom implementation to passthrough to default behavior", async () => { + let dataStrategy = mockDataStrategy(({ matches }) => + Promise.all(matches.map((m) => m.resolve())) + ); + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "json", + path: "/test", + loader: true, + }, + ], + dataStrategy, + }); + + let key = "key"; + let A = await t.fetch("/test", key); + + await A.loaders.json.resolve(json({ message: "hello json" })); + + expect(t.router.state.fetchers.get(key)?.data.message).toBe( + "hello json" + ); + + expect(dataStrategy).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + matches: [ + expect.objectContaining({ + route: expect.objectContaining({ + id: "json", + }), + }), + ], + }) + ); + }); + + it("should allow a custom implementation to passthrough to default behavior and lazy", async () => { + let dataStrategy = mockDataStrategy(({ matches }) => + Promise.all(matches.map((m) => m.resolve())) + ); + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "json", + path: "/test", + lazy: true, + }, + ], + dataStrategy, + }); + + let key = "key"; + let A = await t.fetch("/test", key); + await A.lazy.json.resolve({ + loader: () => ({ message: "hello json" }), + }); + expect(t.router.state.fetchers.get(key)?.data.message).toBe( + "hello json" + ); + expect(dataStrategy).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + matches: [ + expect.objectContaining({ + route: expect.objectContaining({ + id: "json", + }), + }), + ], + }) + ); + }); + }); + + describe("actions", () => { + it("should allow a custom implementation to passthrough to default behavior", async () => { + let dataStrategy = mockDataStrategy(({ matches }) => + Promise.all(matches.map((m) => m.resolve())) + ); + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "json", + path: "/test", + action: true, + }, + ], + dataStrategy, + }); + + let key = "key"; + let A = await t.fetch("/test", key, { + formMethod: "post", + formData: createFormData({}), + }); + + await A.actions.json.resolve(json({ message: "hello json" })); + + expect(t.router.state.fetchers.get(key)?.data.message).toBe( + "hello json" + ); + + expect(dataStrategy).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + matches: expect.arrayContaining([ + expect.objectContaining({ + route: expect.objectContaining({ + id: "json", + }), + }), + ]), + }) + ); + }); + + it("should allow a custom implementation to passthrough to default behavior and lazy", async () => { + let dataStrategy = mockDataStrategy(({ matches }) => + Promise.all(matches.map((m) => m.resolve())) + ); + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "json", + path: "/test", + lazy: true, + }, + ], + dataStrategy, + }); + + let key = "key"; + let A = await t.fetch("/test", key, { + formMethod: "post", + formData: createFormData({}), + }); + await A.lazy.json.resolve({ + action: () => ({ message: "hello json" }), + }); + + expect(t.router.state.fetchers.get(key)?.data.message).toBe( + "hello json" + ); + + expect(dataStrategy).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + matches: expect.arrayContaining([ + expect.objectContaining({ + route: expect.objectContaining({ + id: "json", + }), + }), + ]), + }) + ); + }); + }); + }); + + describe("use-cases", () => { + it("permits users to take control over response decoding", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "json", + path: "/test", + loader: true, + children: [ + { + id: "reverse", + index: true, + loader: true, + }, + ], + }, + ], + async dataStrategy({ matches }) { + return Promise.all( + matches.map(async (m) => { + return await m.resolve(async (handler) => { + let result = await handler(); + if ( + result instanceof Response && + result.headers.get("Content-Type") === "application/reverse" + ) { + let str = await result.text(); + return { + type: "data", + result: { + original: str, + reversed: str.split("").reverse().join(""), + }, + }; + } + // This will be a JSON response we expect to be decoded the normal way + return { type: "data", result }; + }); + }) + ); + }, + }); + + let A = await t.navigate("/test"); + await A.loaders.json.resolve(json({ message: "hello json" })); + await A.loaders.reverse.resolve( + new Response("hello text", { + headers: { "Content-Type": "application/reverse" }, + }) + ); + + expect(t.router.state.loaderData).toEqual({ + json: { message: "hello json" }, + reverse: { + original: "hello text", + reversed: "txet olleh", + }, + }); + }); + + it("allows a single-fetch type approach", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "parent", + path: "/parent", + loader: true, + children: [ + { + id: "child", + path: "child", + loader: true, + }, + ], + }, + ], + async dataStrategy({ matches }) { + // Hold a deferred for each route we need to load + let routeDeferreds: Map< + string, + ReturnType + > = new Map(); + + // Use resolve's to create and await a deferred for each + // route that needs to load + let matchPromises = matches.map((m) => + m.resolve(() => { + // Don't call handler, just create a deferred we can resolve from + // the single fetch response and return it's promise + let dfd = createDeferred(); + routeDeferreds.set(m.route.id, dfd); + return dfd.promise; + }) + ); + + // Mocked single fetch call response for the routes that need loading + let result = { + loaderData: { + parent: "PARENT", + child: "CHILD", + }, + errors: null, + }; + + // Resolve the deferred's above and return the mapped match promises + routeDeferreds.forEach((dfd, routeId) => + dfd.resolve({ type: "data", result: result.loaderData[routeId] }) + ); + return Promise.all(matchPromises); + }, + }); + + await t.navigate("/parent/child"); + + // We don't even have to resolve the loader here because it'll never + // be called in this test + await tick(); + + expect(t.router.state.loaderData).toMatchObject({ + parent: "PARENT", + child: "CHILD", + }); + }); + + it("allows middleware/context implementations", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "parent", + path: "/parent", + loader: true, + handle: { + context: { + parent: () => ({ id: "parent" }), + }, + middleware(context) { + context.parent.whatever = "PARENT MIDDLEWARE"; + }, + }, + children: [ + { + id: "child", + path: "child", + loader: true, + handle: { + context: { + child: () => ({ id: "child" }), + }, + middleware(context) { + context.child.whatever = "CHILD MIDDLEWARE"; + }, + }, + }, + ], + }, + ], + async dataStrategy({ matches }) { + // Run context/middleware sequentially + let context = matches.reduce((acc, m) => { + if (m.route.handle?.context) { + let matchContext = Object.entries(m.route.handle.context).reduce( + (acc, [key, value]) => + Object.assign(acc, { + // @ts-expect-error + [key]: value(), + }), + {} + ); + Object.assign(acc, matchContext); + } + if (m.route.handle?.middleware) { + m.route.handle.middleware(acc); + } + return acc; + }, {}); + + // Run loaders in parallel only exposing contexts from above + return Promise.all( + matches.map((m, i) => + m.resolve(async (handler) => { + // Only provide context values up to this level in the matches + let handlerCtx = matches.slice(0, i + 1).reduce((acc, m) => { + Object.keys(m.route.handle?.context).forEach((k) => { + acc[k] = context[k]; + }); + return acc; + }, {}); + return { type: "data", result: await handler(handlerCtx) }; + }) + ) + ); + }, + }); + + let A = await t.navigate("/parent/child"); + + // Loaders are called with context from their level and above, and + // context reflects any values set by middleware + expect(A.loaders.parent.stub).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + params: expect.any(Object), + }), + { + parent: { + id: "parent", + whatever: "PARENT MIDDLEWARE", + }, + } + ); + + expect(A.loaders.child.stub).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + params: expect.any(Object), + }), + { + parent: { + id: "parent", + whatever: "PARENT MIDDLEWARE", + }, + child: { + id: "child", + whatever: "CHILD MIDDLEWARE", + }, + } + ); + + await A.loaders.parent.resolve("PARENT LOADER"); + expect(t.router.state.navigation.state).toBe("loading"); + + await A.loaders.child.resolve("CHILD LOADER"); + expect(t.router.state.navigation.state).toBe("idle"); + + expect(t.router.state.loaderData).toMatchObject({ + parent: "PARENT LOADER", + child: "CHILD LOADER", + }); + }); + + it("allows middleware/context implementations when some routes don't need to revalidate", async () => { + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "parent", + path: "/parent", + loader: true, + handle: { + context: { + parent: () => ({ id: "parent" }), + }, + middleware(context) { + context.parent.whatever = "PARENT MIDDLEWARE"; + }, + }, + children: [ + { + id: "child", + path: "child", + loader: true, + handle: { + context: { + child: () => ({ id: "child" }), + }, + middleware(context) { + context.child.whatever = "CHILD MIDDLEWARE"; + }, + }, + }, + ], + }, + ], + async dataStrategy({ matches }) { + // Run context/middleware sequentially + let context = matches.reduce((acc, m) => { + if (m.route.handle?.context) { + let matchContext = Object.entries(m.route.handle.context).reduce( + (acc, [key, value]) => + Object.assign(acc, { + // @ts-expect-error + [key]: value(), + }), + {} + ); + Object.assign(acc, matchContext); + } + if (m.route.handle?.middleware) { + m.route.handle.middleware(acc); + } + return acc; + }, {}); + + // Run loaders in parallel only exposing contexts from above + return Promise.all( + matches.map((m, i) => + m.resolve(async (callHandler) => { + // Only provide context values up to this level in the matches + let handlerCtx = matches.slice(0, i + 1).reduce((acc, m) => { + Object.keys(m.route.handle?.context).forEach((k) => { + acc[k] = context[k]; + }); + return acc; + }, {}); + return { type: "data", result: await callHandler(handlerCtx) }; + }) + ) + ); + }, + }); + + let A = await t.navigate("/parent"); + await A.loaders.parent.resolve("PARENT"); + expect(t.router.state.navigation.state).toBe("idle"); + expect(t.router.state.loaderData).toMatchObject({ + parent: "PARENT", + }); + + let B = await t.navigate("/parent/child"); + + // Loaders are called with context from their level and above, and + // context reflects any values set by middleware + expect(B.loaders.child.stub).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.any(Request), + params: expect.any(Object), + }), + { + parent: { + id: "parent", + whatever: "PARENT MIDDLEWARE", + }, + child: { + id: "child", + whatever: "CHILD MIDDLEWARE", + }, + } + ); + + await B.loaders.child.resolve("CHILD"); + expect(t.router.state.navigation.state).toBe("idle"); + + expect(t.router.state.loaderData).toMatchObject({ + parent: "PARENT", + child: "CHILD", + }); + }); + + it("allows automatic caching of loader results", async () => { + let cache: Record = {}; + let t = setup({ + routes: [ + { + path: "/", + }, + { + id: "parent", + path: "/parent", + loader: true, + handle: { + cacheKey: (url: string) => new URL(url).pathname, + }, + children: [ + { + id: "child", + path: "child", + loader: true, + action: true, + }, + ], + }, + ], + async dataStrategy({ request, matches }) { + const getCacheKey = (m: DataStrategyMatch) => + m.route.handle?.cacheKey + ? [m.route.id, m.route.handle.cacheKey(request.url)].join("-") + : null; + + return Promise.all( + matches.map(async (m) => { + return m.resolve(async (handler) => { + if (request.method !== "GET") { + // invalidate on actions + cache = {}; + return { type: "data", result: await handler() }; + } + + let key = getCacheKey(m); + if (key && cache[key]) { + return { type: "data", result: cache[key] }; + } + + let handlerResult = await handler(); + if (key) { + cache[key] = handlerResult; + } + + return { type: "data", result: handlerResult }; + }); + }) + ); + }, + }); + + let A = await t.navigate("/parent/child"); + await A.loaders.parent.resolve("PARENT"); + await A.loaders.child.resolve("CHILD"); + + expect(t.router.state).toMatchObject({ + navigation: { state: "idle" }, + loaderData: { + parent: "PARENT", + child: "CHILD", + }, + }); + + // Changing search params should force revalidation, but pathname-based + // cache will serve the old data + let B = await t.navigate("/parent/child?a=b"); + await B.loaders.child.resolve("CHILD*"); + + expect(t.router.state).toMatchObject({ + navigation: { state: "idle" }, + loaderData: { + parent: "PARENT", + child: "CHILD*", + }, + }); + + // Useless resolution - handler was never called for parent + await B.loaders.parent.resolve("PARENT*"); + + expect(t.router.state).toMatchObject({ + navigation: { state: "idle" }, + loaderData: { + parent: "PARENT", + child: "CHILD*", + }, + }); + + // Action to invalidate the cache + let C = await t.navigate("/parent/child?a=b", { + formMethod: "post", + formData: createFormData({}), + }); + await C.actions.child.resolve("ACTION"); + await C.loaders.parent.resolve("PARENT**"); + await C.loaders.child.resolve("CHILD**"); + + expect(t.router.state).toMatchObject({ + navigation: { state: "idle" }, + actionData: { + child: "ACTION", + }, + loaderData: { + parent: "PARENT**", + child: "CHILD**", + }, + }); + }); + }); +}); diff --git a/packages/router/__tests__/fetchers-test.ts b/packages/router/__tests__/fetchers-test.ts index 07ddfc356c..9969c249a2 100644 --- a/packages/router/__tests__/fetchers-test.ts +++ b/packages/router/__tests__/fetchers-test.ts @@ -2122,6 +2122,7 @@ describe("fetchers", () => { }, "nextUrl": "http://localhost/two/three", "text": undefined, + "unstable_actionStatus": undefined, } `); @@ -2850,6 +2851,7 @@ describe("fetchers", () => { let key = "KEY"; await t.fetch("/parent"); + await tick(); expect(t.router.state.errors).toMatchInlineSnapshot(` { "parent": ErrorResponseImpl { @@ -2885,6 +2887,7 @@ describe("fetchers", () => { let key = "KEY"; await t.fetch("/parent?index"); + await tick(); expect(t.router.state.errors).toMatchInlineSnapshot(` { "parent": ErrorResponseImpl { diff --git a/packages/router/__tests__/redirects-test.ts b/packages/router/__tests__/redirects-test.ts index fe949d632c..27d1d46a9a 100644 --- a/packages/router/__tests__/redirects-test.ts +++ b/packages/router/__tests__/redirects-test.ts @@ -487,6 +487,51 @@ describe("redirects", () => { expect(t.window.location.replace).not.toHaveBeenCalled(); }); + it("automatically replaces in the history stack if you redirect to the same location (root relative)", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent"); + await A.loaders.parent.resolve("PARENT"); + + let B = await t.navigate("/parent", { + formMethod: "post", + formData: createFormData({}), + }); + let C = await B.actions.parent.redirectReturn("/parent"); + await C.loaders.parent.resolve("PARENT*"); + + expect(t.router.state.location).toMatchObject({ pathname: "/parent" }); + + // Because we redirected to the current location it defaults to a replace + await t.router.navigate(-1); + expect(t.router.state.location).toMatchObject({ pathname: "/" }); + }); + + it("automatically replaces in the history stack if you redirect to the same location (absolute)", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent"); + await A.loaders.parent.resolve("PARENT"); + + let B = await t.navigate("/parent", { + formMethod: "post", + formData: createFormData({}), + }); + let C = await B.actions.parent.redirectReturn( + "http://localhost/parent", + undefined, + undefined, + ["parent"] + ); + await C.loaders.parent.resolve("PARENT*"); + + expect(t.router.state.location).toMatchObject({ pathname: "/parent" }); + + // Because we redirected to the current location it defaults to a replace + await t.router.navigate(-1); + expect(t.router.state.location).toMatchObject({ pathname: "/" }); + }); + it("preserves action revalidation across multiple redirects", async () => { let t = setup({ initialEntries: ["/action"], diff --git a/packages/router/__tests__/should-revalidate-test.ts b/packages/router/__tests__/should-revalidate-test.ts index 63801d4edb..be7622b5a6 100644 --- a/packages/router/__tests__/should-revalidate-test.ts +++ b/packages/router/__tests__/should-revalidate-test.ts @@ -1,5 +1,6 @@ import { createMemoryHistory, createRouter, redirect } from "../index"; import type { ShouldRevalidateFunctionArgs } from "../utils"; +import { ErrorResponseImpl } from "../utils"; import { urlMatch } from "./utils/custom-matchers"; import { cleanup } from "./utils/data-router-setup"; import { createFormData, tick } from "./utils/utils"; @@ -321,6 +322,61 @@ describe("shouldRevalidate", () => { router.dispose(); }); + it("includes submissions and acton status on actions that return Responses", async () => { + let shouldRevalidate = jest.fn(() => true); + + let history = createMemoryHistory({ initialEntries: ["/child"] }); + let router = createRouter({ + history, + routes: [ + { + path: "/", + id: "root", + loader: () => "ROOT", + shouldRevalidate, + children: [ + { + path: "child", + id: "child", + loader: () => "CHILD", + action: () => new Response("ACTION", { status: 201 }), + }, + ], + }, + ], + }); + router.initialize(); + + // Initial load - no existing data, should always call loader and should + // not give use ability to opt-out + await tick(); + router.navigate("/child", { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + await tick(); + expect(shouldRevalidate.mock.calls.length).toBe(1); + // @ts-expect-error + let arg = shouldRevalidate.mock.calls[0][0]; + let expectedArg: ShouldRevalidateFunctionArgs = { + currentParams: {}, + currentUrl: expect.urlMatch("http://localhost/child"), + nextParams: {}, + nextUrl: expect.urlMatch("http://localhost/child"), + defaultShouldRevalidate: true, + formMethod: "post", + formAction: "/child", + formEncType: "application/x-www-form-urlencoded", + actionResult: "ACTION", + unstable_actionStatus: 201, + }; + expect(arg).toMatchObject(expectedArg); + // @ts-expect-error + expect(Object.fromEntries(arg.formData)).toEqual({ key: "value" }); + + router.dispose(); + }); + it("includes json submissions", async () => { let shouldRevalidate = jest.fn(() => true); @@ -664,6 +720,7 @@ describe("shouldRevalidate", () => { "nextParams": {}, "nextUrl": "http://localhost/", "text": undefined, + "unstable_actionStatus": undefined, } `); expect(Object.fromEntries(arg.formData)).toEqual({ key: "value" }); @@ -727,6 +784,7 @@ describe("shouldRevalidate", () => { "nextParams": {}, "nextUrl": "http://localhost/", "text": undefined, + "unstable_actionStatus": undefined, } `); @@ -991,4 +1049,272 @@ describe("shouldRevalidate", () => { router.dispose(); }); + + describe("skipActionRevalidation", () => { + it("revalidates after actions returning 4xx/5xx responses by default", async () => { + let count = -1; + let responses = [ + new Response("DATA 400", { status: 400 }), + new Response("DATA 500", { status: 500 }), + ]; + let rootLoader = jest.fn(() => "ROOT " + count); + + let history = createMemoryHistory(); + let router = createRouter({ + history, + routes: [ + { + id: "root", + path: "/", + loader: async () => rootLoader(), + children: [ + { + id: "index", + index: true, + hasErrorBoundary: true, + action: () => responses[++count], + }, + ], + }, + ], + hydrationData: { + loaderData: { + root: "ROOT", + }, + }, + }); + router.initialize(); + + router.navigate("/?index", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + }); + await tick(); + expect(rootLoader.mock.calls.length).toBe(1); + expect(router.state).toMatchObject({ + location: { pathname: "/" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT 0" }, + actionData: { index: "DATA 400" }, + errors: null, + }); + + router.navigate("/?index", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + }); + await tick(); + expect(rootLoader.mock.calls.length).toBe(2); + expect(router.state).toMatchObject({ + location: { pathname: "/" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT 1" }, + actionData: { index: "DATA 500" }, + errors: null, + }); + + router.dispose(); + }); + + it("revalidates after actions throwing 4xx/5xx responses by default", async () => { + let count = -1; + let responses = [ + new Response("ERROR 400", { status: 400 }), + new Response("ERROR 500", { status: 500 }), + ]; + let rootLoader = jest.fn(() => "ROOT " + count); + + let history = createMemoryHistory(); + let router = createRouter({ + history, + routes: [ + { + id: "root", + path: "/", + loader: async () => rootLoader(), + children: [ + { + id: "index", + index: true, + hasErrorBoundary: true, + action: () => { + throw responses[++count]; + }, + }, + ], + }, + ], + hydrationData: { + loaderData: { + root: "ROOT", + }, + }, + }); + router.initialize(); + + router.navigate("/?index", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + }); + await tick(); + expect(rootLoader.mock.calls.length).toBe(1); + expect(router.state).toMatchObject({ + location: { pathname: "/" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT 0" }, + actionData: null, + errors: { index: new ErrorResponseImpl(400, "", "ERROR 400") }, + }); + + router.navigate("/?index", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + }); + await tick(); + expect(rootLoader.mock.calls.length).toBe(2); + expect(router.state).toMatchObject({ + location: { pathname: "/" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT 1" }, + actionData: null, + errors: { index: new ErrorResponseImpl(500, "", "ERROR 500") }, + }); + + router.dispose(); + }); + + it("does not revalidates after actions returning 4xx/5xx responses with flag", async () => { + let count = -1; + let responses = [ + new Response("DATA 400", { status: 400 }), + new Response("DATA 500", { status: 500 }), + ]; + let rootLoader = jest.fn(() => "ROOT " + count); + + let history = createMemoryHistory(); + let router = createRouter({ + history, + routes: [ + { + id: "root", + path: "/", + loader: async () => rootLoader(), + children: [ + { + id: "index", + index: true, + hasErrorBoundary: true, + action: () => responses[++count], + }, + ], + }, + ], + hydrationData: { + loaderData: { + root: "ROOT", + }, + }, + future: { unstable_skipActionErrorRevalidation: true }, + }); + router.initialize(); + + router.navigate("/?index", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + }); + await tick(); + expect(rootLoader.mock.calls.length).toBe(0); + expect(router.state).toMatchObject({ + location: { pathname: "/" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT" }, + actionData: { index: "DATA 400" }, + errors: null, + }); + + router.navigate("/?index", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + }); + await tick(); + expect(rootLoader.mock.calls.length).toBe(0); + expect(router.state).toMatchObject({ + location: { pathname: "/" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT" }, + actionData: { index: "DATA 500" }, + errors: null, + }); + + router.dispose(); + }); + + it("does not revalidate after actions throwing 4xx/5xx responses with flag", async () => { + let count = -1; + let responses = [ + new Response("ERROR 400", { status: 400 }), + new Response("ERROR 500", { status: 500 }), + ]; + let rootLoader = jest.fn(() => "ROOT " + count); + + let history = createMemoryHistory(); + let router = createRouter({ + history, + routes: [ + { + id: "root", + path: "/", + loader: async () => rootLoader(), + children: [ + { + id: "index", + index: true, + hasErrorBoundary: true, + action: () => { + throw responses[++count]; + }, + }, + ], + }, + ], + hydrationData: { + loaderData: { + root: "ROOT", + }, + }, + future: { unstable_skipActionErrorRevalidation: true }, + }); + router.initialize(); + + router.navigate("/?index", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + }); + await tick(); + expect(rootLoader.mock.calls.length).toBe(0); + expect(router.state).toMatchObject({ + location: { pathname: "/" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT" }, + actionData: null, + errors: { index: new ErrorResponseImpl(400, "", "ERROR 400") }, + }); + + router.navigate("/?index", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + }); + await tick(); + expect(rootLoader.mock.calls.length).toBe(0); + expect(router.state).toMatchObject({ + location: { pathname: "/" }, + navigation: { state: "idle" }, + loaderData: { root: "ROOT" }, + actionData: null, + errors: { index: new ErrorResponseImpl(500, "", "ERROR 500") }, + }); + + router.dispose(); + }); + }); }); diff --git a/packages/router/__tests__/ssr-test.ts b/packages/router/__tests__/ssr-test.ts index daea5eb978..bf0d54c6b6 100644 --- a/packages/router/__tests__/ssr-test.ts +++ b/packages/router/__tests__/ssr-test.ts @@ -2,6 +2,7 @@ * @jest-environment node */ +import urlDataStrategy from "./utils/urlDataStrategy"; import type { StaticHandler, StaticHandlerContext } from "../router"; import { UNSAFE_DEFERRED_SYMBOL, @@ -135,6 +136,14 @@ describe("ssr", () => { path: "/redirect", loader: () => redirect("/"), }, + { + id: "custom", + path: "/custom", + loader: () => + new Response(new URLSearchParams([["foo", "bar"]]).toString(), { + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + }), + }, ]; // Regardless of if the URL is internal or external - all absolute URL @@ -679,6 +688,33 @@ describe("ssr", () => { }); }); + it("should skip bubbling loader errors when skipLoaderErrorBubbling is passed", async () => { + let routes = [ + { + id: "root", + path: "/", + hasErrorBoundary: true, + children: [ + { + id: "child", + path: "child", + loader: () => Promise.reject("CHILD"), + }, + ], + }, + ]; + + let { query } = createStaticHandler(routes); + let context; + + context = await query(createRequest("/child"), { + skipLoaderErrorBubbling: true, + }); + expect(context.errors).toEqual({ + child: "CHILD", + }); + }); + it("should handle aborted load requests", async () => { let dfd = createDeferred(); let controller = new AbortController(); @@ -972,6 +1008,52 @@ describe("ssr", () => { // Can't re-read body here since it's the same request as the root }); + it("should send proper arguments to loaders after an action errors", async () => { + let actionStub = jest.fn(() => Promise.reject("ACTION ERROR")); + let rootLoaderStub = jest.fn(() => "ROOT"); + let childLoaderStub = jest.fn(() => "CHILD"); + let { query } = createStaticHandler([ + { + id: "root", + path: "/", + loader: rootLoaderStub, + children: [ + { + id: "child", + path: "child", + action: actionStub, + loader: childLoaderStub, + hasErrorBoundary: true, + }, + ], + }, + ]); + await query( + createSubmitRequest("/child", { + headers: { + test: "value", + }, + }) + ); + + // @ts-expect-error + let actionRequest = actionStub.mock.calls[0][0]?.request; + expect(actionRequest.method).toBe("POST"); + expect(actionRequest.url).toBe("http://localhost/child"); + expect(actionRequest.headers.get("Content-Type")).toBe( + "application/x-www-form-urlencoded;charset=UTF-8" + ); + expect((await actionRequest.formData()).get("key")).toBe("value"); + + // @ts-expect-error + let rootLoaderRequest = rootLoaderStub.mock.calls[0][0]?.request; + expect(rootLoaderRequest.method).toBe("GET"); + expect(rootLoaderRequest.url).toBe("http://localhost/child"); + expect(rootLoaderRequest.headers.get("test")).toBe("value"); + expect(await rootLoaderRequest.text()).toBe(""); + expect(childLoaderStub).not.toHaveBeenCalled(); + }); + it("should support a requestContext passed to loaders and actions", async () => { let requestContext = { sessionId: "12345" }; let rootStub = jest.fn(() => "ROOT"); @@ -1008,6 +1090,56 @@ describe("ssr", () => { expect(arg(childStub).context.sessionId).toBe("12345"); }); + it("should support a loadRouteIds parameter for granular loads", async () => { + let rootStub = jest.fn(() => "ROOT"); + let childStub = jest.fn(() => "CHILD"); + let actionStub = jest.fn(() => "CHILD ACTION"); + let { query } = createStaticHandler([ + { + id: "root", + path: "/", + loader: rootStub, + children: [ + { + id: "child", + path: "child", + action: actionStub, + loader: childStub, + }, + ], + }, + ]); + + let ctx = await query(createRequest("/child"), { + loadRouteIds: ["child"], + }); + expect(rootStub).not.toHaveBeenCalled(); + expect(childStub).toHaveBeenCalled(); + expect(ctx).toMatchObject({ + loaderData: { + child: "CHILD", + }, + }); + + actionStub.mockClear(); + rootStub.mockClear(); + childStub.mockClear(); + + ctx = await query(createSubmitRequest("/child"), { + loadRouteIds: ["child"], + }); + expect(rootStub).not.toHaveBeenCalled(); + expect(childStub).toHaveBeenCalled(); + expect(ctx).toMatchObject({ + actionData: { + child: "CHILD ACTION", + }, + loaderData: { + child: "CHILD", + }, + }); + }); + describe("deferred", () => { let { query } = createStaticHandler(SSR_ROUTES); @@ -1514,6 +1646,28 @@ describe("ssr", () => { }); }); }); + + describe("router dataStrategy", () => { + it("should support document load navigations with custom dataStrategy", async () => { + let { query } = createStaticHandler(SSR_ROUTES, { + unstable_dataStrategy: urlDataStrategy, + }); + + let context = await query(createRequest("/custom")); + expect(context).toMatchObject({ + actionData: null, + loaderData: { + custom: expect.any(URLSearchParams), + }, + errors: null, + location: { pathname: "/custom" }, + matches: [{ route: { id: "custom" } }], + }); + expect( + (context as StaticHandlerContext).loaderData.custom.get("foo") + ).toEqual("bar"); + }); + }); }); describe("singular route requests", () => { @@ -2524,5 +2678,18 @@ describe("ssr", () => { /* eslint-enable jest/no-conditional-expect */ }); + + describe("router dataStrategy", () => { + it("should match routes automatically if no routeId is provided", async () => { + let { queryRoute } = createStaticHandler(SSR_ROUTES, { + unstable_dataStrategy: urlDataStrategy, + }); + let data; + + data = await queryRoute(createRequest("/custom")); + expect(data).toBeInstanceOf(URLSearchParams); + expect((data as URLSearchParams).get("foo")).toBe("bar"); + }); + }); }); }); diff --git a/packages/router/__tests__/utils/data-router-setup.ts b/packages/router/__tests__/utils/data-router-setup.ts index 40d73cb8ff..cc77d8f290 100644 --- a/packages/router/__tests__/utils/data-router-setup.ts +++ b/packages/router/__tests__/utils/data-router-setup.ts @@ -23,6 +23,7 @@ import { invariant } from "../../history"; import type { AgnosticIndexRouteObject, AgnosticNonIndexRouteObject, + DataStrategyFunction, } from "../../utils"; import { stripBasename } from "../../utils"; @@ -33,7 +34,7 @@ import { isRedirect, tick } from "./utils"; // by our test harness export type TestIndexRouteObject = Pick< AgnosticIndexRouteObject, - "id" | "index" | "path" | "shouldRevalidate" + "id" | "index" | "path" | "shouldRevalidate" | "handle" > & { lazy?: boolean; loader?: boolean; @@ -43,7 +44,7 @@ export type TestIndexRouteObject = Pick< export type TestNonIndexRouteObject = Pick< AgnosticNonIndexRouteObject, - "id" | "index" | "path" | "shouldRevalidate" + "id" | "index" | "path" | "shouldRevalidate" | "handle" > & { lazy?: boolean; loader?: boolean; @@ -142,6 +143,8 @@ type SetupOpts = { initialEntries?: InitialEntry[]; initialIndex?: number; hydrationData?: HydrationState; + future?: FutureConfig; + dataStrategy?: DataStrategyFunction; future?: Partial; }; @@ -177,6 +180,7 @@ export function setup({ initialIndex, hydrationData, future, + dataStrategy, }: SetupOpts) { let guid = 0; // Global "active" helpers, keyed by navType:guid:loaderOrAction:routeId. @@ -234,7 +238,7 @@ export function setup({ }; } if (r.loader) { - enhancedRoute.loader = (args) => { + enhancedRoute.loader = (...args) => { let navigationId = activeLoaderType === "fetch" ? activeLoaderFetchId @@ -242,13 +246,13 @@ export function setup({ let helperKey = `${activeLoaderType}:${navigationId}:loader:${enhancedRoute.id}`; let helpers = activeHelpers.get(helperKey); invariant(helpers, `No helpers found for: ${helperKey}`); - helpers.stub(args); - helpers._signal = args.request.signal; + helpers.stub(...args); + helpers._signal = args[0].request.signal; return helpers.dfd.promise; }; } if (r.action) { - enhancedRoute.action = (args) => { + enhancedRoute.action = (...args) => { let type = activeActionType; let navigationId = activeActionType === "fetch" @@ -257,8 +261,8 @@ export function setup({ let helperKey = `${activeActionType}:${navigationId}:action:${enhancedRoute.id}`; let helpers = activeHelpers.get(helperKey); invariant(helpers, `No helpers found for: ${helperKey}`); - helpers.stub(args); - helpers._signal = args.request.signal; + helpers.stub(...args); + helpers._signal = args[0].request.signal; return helpers.dfd.promise.then( (result) => { // After a successful non-redirect action, ensure we call the right @@ -318,6 +322,7 @@ export function setup({ hydrationData, future, window: testWindow, + unstable_dataStrategy: dataStrategy, }).initialize(); function getRouteHelpers( diff --git a/packages/router/__tests__/utils/urlDataStrategy.ts b/packages/router/__tests__/utils/urlDataStrategy.ts new file mode 100644 index 0000000000..5acf3687d8 --- /dev/null +++ b/packages/router/__tests__/utils/urlDataStrategy.ts @@ -0,0 +1,27 @@ +import type { + DataStrategyFunction, + DataStrategyFunctionArgs, +} from "@remix-run/router"; +import { invariant } from "./utils"; + +export default async function urlDataStrategy({ + matches, +}: DataStrategyFunctionArgs): ReturnType { + return Promise.all( + matches.map((match) => + match.resolve(async (handler) => { + let response = await handler(); + invariant(response instanceof Response, "Expected a response"); + let contentType = response.headers.get("Content-Type"); + invariant( + contentType === "application/x-www-form-urlencoded", + "Invalid Response" + ); + return { + type: "data", + result: new URLSearchParams(await response.text()), + }; + }) + ) + ); +} diff --git a/packages/router/index.ts b/packages/router/index.ts index 060360d34a..8a07c5e27b 100644 --- a/packages/router/index.ts +++ b/packages/router/index.ts @@ -9,9 +9,13 @@ export type { AgnosticNonIndexRouteObject, AgnosticRouteMatch, AgnosticRouteObject, + DataStrategyFunction as unstable_DataStrategyFunction, + DataStrategyFunctionArgs as unstable_DataStrategyFunctionArgs, + DataStrategyMatch as unstable_DataStrategyMatch, ErrorResponse, FormEncType, FormMethod, + HandlerResult as unstable_HandlerResult, HTMLFormMethod, JsonFunction, LazyRouteFunction, diff --git a/packages/router/router.ts b/packages/router/router.ts index 951aa1b992..cf693cc3b9 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -8,11 +8,13 @@ import { warning, } from "./history"; import type { - ActionFunction, AgnosticDataRouteMatch, AgnosticDataRouteObject, + DataStrategyMatch, AgnosticRouteObject, DataResult, + DataStrategyFunction, + DataStrategyFunctionArgs, DeferredData, DeferredResult, DetectErrorBoundaryFunction, @@ -20,8 +22,8 @@ import type { FormEncType, FormMethod, HTMLFormMethod, + HandlerResult, ImmutableRouteKey, - LoaderFunction, MapRoutePropertiesFunction, MutationFormMethod, RedirectResult, @@ -357,6 +359,7 @@ export interface FutureConfig { v7_partialHydration: boolean; v7_prependBasename: boolean; v7_relativeSplatPath: boolean; + unstable_skipActionErrorRevalidation: boolean; } /** @@ -374,6 +377,7 @@ export interface RouterInit { future?: Partial; hydrationData?: HydrationState; window?: Window; + unstable_dataStrategy?: DataStrategyFunction; } /** @@ -400,7 +404,11 @@ export interface StaticHandler { dataRoutes: AgnosticDataRouteObject[]; query( request: Request, - opts?: { requestContext?: unknown } + opts?: { + loadRouteIds?: string[]; + requestContext?: unknown; + skipLoaderErrorBubbling?: boolean; + } ): Promise; queryRoute( request: Request, @@ -616,18 +624,14 @@ interface ShortCircuitable { shortCircuited?: boolean; } +type PendingActionResult = [string, SuccessResult | ErrorResult]; + interface HandleActionResult extends ShortCircuitable { /** - * Error thrown from the current action, keyed by the route containing the - * error boundary to render the error. To be committed to the state after - * loaders have completed - */ - pendingActionError?: RouteData; - /** - * Data returned from the current action, keyed by the route owning the action. - * To be committed to the state after loaders have completed + * Tuple for the returned or thrown value from the current action. The routeId + * is the action route for success and the bubbled boundary route for errors. */ - pendingActionData?: RouteData; + pendingActionResult?: PendingActionResult; } interface HandleLoadersResult extends ShortCircuitable { @@ -660,16 +664,6 @@ interface RevalidatingFetcher extends FetchLoadMatch { controller: AbortController | null; } -/** - * Wrapper object to allow us to throw any response out from callLoaderOrAction - * for queryRouter while preserving whether or not it was thrown or returned - * from the loader/action - */ -interface QueryRouteResponse { - type: ResultType.data | ResultType.error; - response: Response; -} - const validMutationMethodsArr: MutationFormMethod[] = [ "post", "put", @@ -776,6 +770,7 @@ export function createRouter(init: RouterInit): Router { ); let inFlightDataRoutes: AgnosticDataRouteObject[] | undefined; let basename = init.basename || "/"; + let dataStrategyImpl = init.unstable_dataStrategy || defaultDataStrategy; // Config driven behavior flags let future: FutureConfig = { v7_fetcherPersist: false, @@ -783,6 +778,7 @@ export function createRouter(init: RouterInit): Router { v7_partialHydration: false, v7_prependBasename: false, v7_relativeSplatPath: false, + unstable_skipActionErrorRevalidation: false, ...init.future, }; // Cleanup function for history @@ -835,9 +831,16 @@ export function createRouter(init: RouterInit): Router { let errors = init.hydrationData ? init.hydrationData.errors : null; let isRouteInitialized = (m: AgnosticDataRouteMatch) => { // No loader, nothing to initialize - if (!m.route.loader) return true; + if (!m.route.loader) { + return true; + } // Explicitly opting-in to running on hydration - if (m.route.loader.hydrate === true) return false; + if ( + typeof m.route.loader === "function" && + m.route.loader.hydrate === true + ) { + return false; + } // Otherwise, initialized if hydrated with data or an error return ( (loaderData && loaderData[m.route.id] !== undefined) || @@ -1493,24 +1496,24 @@ export function createRouter(init: RouterInit): Router { pendingNavigationController.signal, opts && opts.submission ); - let pendingActionData: RouteData | undefined; - let pendingError: RouteData | undefined; + let pendingActionResult: PendingActionResult | undefined; if (opts && opts.pendingError) { // If we have a pendingError, it means the user attempted a GET submission // with binary FormData so assign here and skip to handleLoaders. That // way we handle calling loaders above the boundary etc. It's not really // different from an actionError in that sense. - pendingError = { - [findNearestBoundary(matches).route.id]: opts.pendingError, - }; + pendingActionResult = [ + findNearestBoundary(matches).route.id, + { type: ResultType.error, error: opts.pendingError }, + ]; } else if ( opts && opts.submission && isMutationMethod(opts.submission.formMethod) ) { // Call action if we received an action submission - let actionOutput = await handleAction( + let actionResult = await handleAction( request, location, opts.submission, @@ -1518,17 +1521,20 @@ export function createRouter(init: RouterInit): Router { { replace: opts.replace, flushSync } ); - if (actionOutput.shortCircuited) { + if (actionResult.shortCircuited) { return; } - pendingActionData = actionOutput.pendingActionData; - pendingError = actionOutput.pendingActionError; + pendingActionResult = actionResult.pendingActionResult; loadingNavigation = getLoadingNavigation(location, opts.submission); flushSync = false; // Create a GET request for the loaders - request = new Request(request.url, { signal: request.signal }); + request = createClientSideRequest( + init.history, + request.url, + request.signal + ); } // Call loaders @@ -1542,8 +1548,7 @@ export function createRouter(init: RouterInit): Router { opts && opts.replace, opts && opts.initialHydration === true, flushSync, - pendingActionData, - pendingError + pendingActionResult ); if (shortCircuited) { @@ -1557,7 +1562,7 @@ export function createRouter(init: RouterInit): Router { completeNavigation(location, { matches, - ...(pendingActionData ? { actionData: pendingActionData } : {}), + ...getActionDataForCommit(pendingActionResult), loaderData, errors, }); @@ -1592,16 +1597,13 @@ export function createRouter(init: RouterInit): Router { }), }; } else { - result = await callLoaderOrAction( + let results = await callDataStrategy( "action", request, - actionMatch, - matches, - manifest, - mapRouteProperties, - basename, - future.v7_relativeSplatPath + [actionMatch], + matches ); + result = results[0]; if (request.signal.aborted) { return { shortCircuited: true }; @@ -1616,13 +1618,24 @@ export function createRouter(init: RouterInit): Router { // If the user didn't explicity indicate replace behavior, replace if // we redirected to the exact same location we're currently at to avoid // double back-buttons - replace = - result.location === state.location.pathname + state.location.search; + let location = normalizeRedirectLocation( + result.response.headers.get("Location")!, + new URL(request.url), + basename + ); + replace = location === state.location.pathname + state.location.search; } - await startRedirectNavigation(state, result, { submission, replace }); + await startRedirectNavigation(request, result, { + submission, + replace, + }); return { shortCircuited: true }; } + if (isDeferredResult(result)) { + throw getInternalRouterError(400, { type: "defer-action" }); + } + if (isErrorResult(result)) { // Store off the pending error - we use it to determine which loaders // to call and will commit it when we complete the navigation @@ -1637,18 +1650,12 @@ export function createRouter(init: RouterInit): Router { } return { - // Send back an empty object we can use to clear out any prior actionData - pendingActionData: {}, - pendingActionError: { [boundaryMatch.route.id]: result.error }, + pendingActionResult: [boundaryMatch.route.id, result], }; } - if (isDeferredResult(result)) { - throw getInternalRouterError(400, { type: "defer-action" }); - } - return { - pendingActionData: { [actionMatch.route.id]: result.data }, + pendingActionResult: [actionMatch.route.id, result], }; } @@ -1664,8 +1671,7 @@ export function createRouter(init: RouterInit): Router { replace?: boolean, initialHydration?: boolean, flushSync?: boolean, - pendingActionData?: RouteData, - pendingError?: RouteData + pendingActionResult?: PendingActionResult ): Promise { // Figure out the right navigation we want to use for data loading let loadingNavigation = @@ -1686,6 +1692,7 @@ export function createRouter(init: RouterInit): Router { activeSubmission, location, future.v7_partialHydration && initialHydration === true, + future.unstable_skipActionErrorRevalidation, isRevalidationRequired, cancelledDeferredRoutes, cancelledFetcherLoads, @@ -1694,8 +1701,7 @@ export function createRouter(init: RouterInit): Router { fetchRedirectIds, routesToUse, basename, - pendingActionData, - pendingError + pendingActionResult ); // Cancel pending deferreds for no-longer-matched routes or routes we're @@ -1718,8 +1724,11 @@ export function createRouter(init: RouterInit): Router { matches, loaderData: {}, // Commit pending error if we're short circuiting - errors: pendingError || null, - ...(pendingActionData ? { actionData: pendingActionData } : {}), + errors: + pendingActionResult && isErrorResult(pendingActionResult[1]) + ? { [pendingActionResult[0]]: pendingActionResult[1].error } + : null, + ...getActionDataForCommit(pendingActionResult), ...(updatedFetchers ? { fetchers: new Map(state.fetchers) } : {}), }, { flushSync } @@ -1745,15 +1754,27 @@ export function createRouter(init: RouterInit): Router { ); state.fetchers.set(rf.key, revalidatingFetcher); }); - let actionData = pendingActionData || state.actionData; + + let actionData: Record | null | undefined; + if (pendingActionResult && !isErrorResult(pendingActionResult[1])) { + // This is cast to `any` currently because `RouteData`uses any and it + // would be a breaking change to use any. + // TODO: v7 - change `RouteData` to use `unknown` instead of `any` + actionData = { + [pendingActionResult[0]]: pendingActionResult[1].data as any, + }; + } else if (state.actionData) { + if (Object.keys(state.actionData).length === 0) { + actionData = null; + } else { + actionData = state.actionData; + } + } + updateState( { navigation: loadingNavigation, - ...(actionData - ? Object.keys(actionData).length === 0 - ? { actionData: null } - : { actionData } - : {}), + ...(actionData !== undefined ? { actionData } : {}), ...(revalidatingFetchers.length > 0 ? { fetchers: new Map(state.fetchers) } : {}), @@ -1786,7 +1807,7 @@ export function createRouter(init: RouterInit): Router { ); } - let { results, loaderResults, fetcherResults } = + let { loaderResults, fetcherResults } = await callLoadersAndMaybeResolveData( state.matches, matches, @@ -1811,7 +1832,7 @@ export function createRouter(init: RouterInit): Router { revalidatingFetchers.forEach((rf) => fetchControllers.delete(rf.key)); // If any loaders returned a redirect Response, start a new REPLACE navigation - let redirect = findRedirect(results); + let redirect = findRedirect([...loaderResults, ...fetcherResults]); if (redirect) { if (redirect.idx >= matchesToLoad.length) { // If this redirect came from a fetcher make sure we mark it in @@ -1821,7 +1842,9 @@ export function createRouter(init: RouterInit): Router { revalidatingFetchers[redirect.idx - matchesToLoad.length].key; fetchRedirectIds.add(fetcherKey); } - await startRedirectNavigation(state, redirect.result, { replace }); + await startRedirectNavigation(request, redirect.result, { + replace, + }); return { shortCircuited: true }; } @@ -1831,7 +1854,7 @@ export function createRouter(init: RouterInit): Router { matches, matchesToLoad, loaderResults, - pendingError, + pendingActionResult, revalidatingFetchers, fetcherResults, activeDeferreds @@ -1995,16 +2018,13 @@ export function createRouter(init: RouterInit): Router { fetchControllers.set(key, abortController); let originatingLoadId = incrementingLoadId; - let actionResult = await callLoaderOrAction( + let actionResults = await callDataStrategy( "action", fetchRequest, - match, - requestMatches, - manifest, - mapRouteProperties, - basename, - future.v7_relativeSplatPath + [match], + requestMatches ); + let actionResult = actionResults[0]; if (fetchRequest.signal.aborted) { // We can delete this so long as we weren't aborted by our own fetcher @@ -2037,7 +2057,7 @@ export function createRouter(init: RouterInit): Router { } else { fetchRedirectIds.add(key); updateFetcherState(key, getLoadingFetcher(submission)); - return startRedirectNavigation(state, actionResult, { + return startRedirectNavigation(fetchRequest, actionResult, { fetcherSubmission: submission, }); } @@ -2083,6 +2103,7 @@ export function createRouter(init: RouterInit): Router { submission, nextLocation, false, + future.unstable_skipActionErrorRevalidation, isRevalidationRequired, cancelledDeferredRoutes, cancelledFetcherLoads, @@ -2091,8 +2112,7 @@ export function createRouter(init: RouterInit): Router { fetchRedirectIds, routesToUse, basename, - { [match.route.id]: actionResult.data }, - undefined // No need to send through errors since we short circuit above + [match.route.id, actionResult] ); // Put all revalidating fetchers into the loading state, except for the @@ -2126,7 +2146,7 @@ export function createRouter(init: RouterInit): Router { abortPendingFetchRevalidations ); - let { results, loaderResults, fetcherResults } = + let { loaderResults, fetcherResults } = await callLoadersAndMaybeResolveData( state.matches, matches, @@ -2148,7 +2168,7 @@ export function createRouter(init: RouterInit): Router { fetchControllers.delete(key); revalidatingFetchers.forEach((r) => fetchControllers.delete(r.key)); - let redirect = findRedirect(results); + let redirect = findRedirect([...loaderResults, ...fetcherResults]); if (redirect) { if (redirect.idx >= matchesToLoad.length) { // If this redirect came from a fetcher make sure we mark it in @@ -2158,7 +2178,7 @@ export function createRouter(init: RouterInit): Router { revalidatingFetchers[redirect.idx - matchesToLoad.length].key; fetchRedirectIds.add(fetcherKey); } - return startRedirectNavigation(state, redirect.result); + return startRedirectNavigation(revalidationRequest, redirect.result); } // Process and commit output from loaders @@ -2246,16 +2266,13 @@ export function createRouter(init: RouterInit): Router { fetchControllers.set(key, abortController); let originatingLoadId = incrementingLoadId; - let result: DataResult = await callLoaderOrAction( + let results = await callDataStrategy( "loader", fetchRequest, - match, - matches, - manifest, - mapRouteProperties, - basename, - future.v7_relativeSplatPath + [match], + matches ); + let result = results[0]; // Deferred isn't supported for fetcher loads, await everything and treat it // as a normal load. resolveDeferredData will return undefined if this @@ -2293,7 +2310,7 @@ export function createRouter(init: RouterInit): Router { return; } else { fetchRedirectIds.add(key); - await startRedirectNavigation(state, result); + await startRedirectNavigation(fetchRequest, result); return; } } @@ -2330,7 +2347,7 @@ export function createRouter(init: RouterInit): Router { * the history action from the original navigation (PUSH or REPLACE). */ async function startRedirectNavigation( - state: RouterState, + request: Request, redirect: RedirectResult, { submission, @@ -2342,26 +2359,29 @@ export function createRouter(init: RouterInit): Router { replace?: boolean; } = {} ) { - if (redirect.revalidate) { + if (redirect.response.headers.has("X-Remix-Revalidate")) { isRevalidationRequired = true; } - let redirectLocation = createLocation(state.location, redirect.location, { + let location = redirect.response.headers.get("Location"); + invariant(location, "Expected a Location header on the redirect Response"); + location = normalizeRedirectLocation( + location, + new URL(request.url), + basename + ); + let redirectLocation = createLocation(state.location, location, { _isRedirect: true, }); - invariant( - redirectLocation, - "Expected a location on the redirect navigation" - ); if (isBrowser) { let isDocumentReload = false; - if (redirect.reloadDocument) { + if (redirect.response.headers.has("X-Remix-Reload-Document")) { // Hard reload if the response contained X-Remix-Reload-Document isDocumentReload = true; - } else if (ABSOLUTE_URL_REGEX.test(redirect.location)) { - const url = init.history.createURL(redirect.location); + } else if (ABSOLUTE_URL_REGEX.test(location)) { + const url = init.history.createURL(location); isDocumentReload = // Hard reload if it's an absolute URL to a new origin url.origin !== routerWindow.location.origin || @@ -2371,9 +2391,9 @@ export function createRouter(init: RouterInit): Router { if (isDocumentReload) { if (replace) { - routerWindow.location.replace(redirect.location); + routerWindow.location.replace(location); } else { - routerWindow.location.assign(redirect.location); + routerWindow.location.assign(location); } return; } @@ -2404,14 +2424,14 @@ export function createRouter(init: RouterInit): Router { // redirected location let activeSubmission = submission || fetcherSubmission; if ( - redirectPreserveMethodStatusCodes.has(redirect.status) && + redirectPreserveMethodStatusCodes.has(redirect.response.status) && activeSubmission && isMutationMethod(activeSubmission.formMethod) ) { await startNavigation(redirectHistoryAction, redirectLocation, { submission: { ...activeSubmission, - formAction: redirect.location, + formAction: location, }, // Preserve this flag across redirects preventScrollReset: pendingPreventScrollReset, @@ -2433,6 +2453,55 @@ export function createRouter(init: RouterInit): Router { } } + // Utility wrapper for calling dataStrategy client-side without having to + // pass around the manifest, mapRouteProperties, etc. + async function callDataStrategy( + type: "loader" | "action", + request: Request, + matchesToLoad: AgnosticDataRouteMatch[], + matches: AgnosticDataRouteMatch[] + ): Promise { + try { + let results = await callDataStrategyImpl( + dataStrategyImpl, + type, + request, + matchesToLoad, + matches, + manifest, + mapRouteProperties + ); + + return await Promise.all( + results.map((result, i) => { + if (isRedirectHandlerResult(result)) { + let response = result.result as Response; + return { + type: ResultType.redirect, + response: normalizeRelativeRoutingRedirectResponse( + response, + request, + matchesToLoad[i].route.id, + matches, + basename, + future.v7_relativeSplatPath + ), + }; + } + + return convertHandlerResultToDataResult(result); + }) + ); + } catch (e) { + // If the outer dataStrategy method throws, just return the error for all + // matches - and it'll naturally bubble to the root + return matchesToLoad.map(() => ({ + type: ResultType.error, + error: e, + })); + } + } + async function callLoadersAndMaybeResolveData( currentMatches: AgnosticDataRouteMatch[], matches: AgnosticDataRouteMatch[], @@ -2440,45 +2509,33 @@ export function createRouter(init: RouterInit): Router { fetchersToLoad: RevalidatingFetcher[], request: Request ) { - // 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, - manifest, - mapRouteProperties, - basename, - future.v7_relativeSplatPath - ) - ), + let [loaderResults, ...fetcherResults] = await Promise.all([ + matchesToLoad.length + ? callDataStrategy("loader", request, matchesToLoad, matches) + : [], ...fetchersToLoad.map((f) => { if (f.matches && f.match && f.controller) { - return callLoaderOrAction( - "loader", - createClientSideRequest(init.history, f.path, f.controller.signal), - f.match, - f.matches, - manifest, - mapRouteProperties, - basename, - future.v7_relativeSplatPath + let fetcherRequest = createClientSideRequest( + init.history, + f.path, + f.controller.signal ); + return callDataStrategy( + "loader", + fetcherRequest, + [f.match], + f.matches + ).then((r) => r[0]); } else { - let error: ErrorResult = { + return Promise.resolve({ type: ResultType.error, - error: getInternalRouterError(404, { pathname: f.path }), - }; - return error; + error: getInternalRouterError(404, { + pathname: f.path, + }), + }); } }), ]); - let loaderResults = results.slice(0, matchesToLoad.length); - let fetcherResults = results.slice(matchesToLoad.length); await Promise.all([ resolveDeferredResults( @@ -2498,7 +2555,10 @@ export function createRouter(init: RouterInit): Router { ), ]); - return { results, loaderResults, fetcherResults }; + return { + loaderResults, + fetcherResults, + }; } function interruptActiveLoads() { @@ -2866,6 +2926,7 @@ export interface CreateStaticHandlerOptions { * @deprecated Use `mapRouteProperties` instead */ detectErrorBoundary?: DetectErrorBoundaryFunction; + unstable_dataStrategy?: DataStrategyFunction; mapRouteProperties?: MapRoutePropertiesFunction; future?: Partial; } @@ -2881,6 +2942,7 @@ export function createStaticHandler( let manifest: RouteManifest = {}; let basename = (opts ? opts.basename : null) || "/"; + let dataStrategyImpl = opts?.unstable_dataStrategy || defaultDataStrategy; let mapRouteProperties: MapRoutePropertiesFunction; if (opts?.mapRouteProperties) { mapRouteProperties = opts.mapRouteProperties; @@ -2925,10 +2987,27 @@ export function createStaticHandler( * redirect response is returned or thrown from any action/loader. We * propagate that out and return the raw Response so the HTTP server can * return it directly. + * + * - `opts.loadRouteIds` is an optional array of routeIds if you wish to only + * run a subset of route loaders on a GET request + * - `opts.requestContext` is an optional server context that will be passed + * to actions/loaders in the `context` parameter + * - `opts.skipLoaderErrorBubbling` is an optional parameter that will prevent + * the bubbling of loader errors which allows single-fetch-type implementations + * where the client will handle the bubbling and we may need to return data + * for the handling route */ async function query( request: Request, - { requestContext }: { requestContext?: unknown } = {} + { + loadRouteIds, + requestContext, + skipLoaderErrorBubbling, + }: { + loadRouteIds?: string[]; + requestContext?: unknown; + skipLoaderErrorBubbling?: boolean; + } = {} ): Promise { let url = new URL(request.url); let method = request.method; @@ -2974,7 +3053,15 @@ export function createStaticHandler( }; } - let result = await queryImpl(request, location, matches, requestContext); + let result = await queryImpl( + request, + location, + matches, + requestContext, + loadRouteIds || null, + skipLoaderErrorBubbling === true, + null + ); if (isResponse(result)) { return result; } @@ -3004,6 +3091,12 @@ export function createStaticHandler( * serialize the error as they see fit while including the proper response * code. Examples here are 404 and 405 errors that occur prior to reaching * any user-defined loaders. + * + * - `opts.routeId` allows you to specify the specific route handler to call. + * If not provided the handler will determine the proper route by matching + * against `request.url` + * - `opts.requestContext` is an optional server context that will be passed + * to actions/loaders in the `context` parameter */ async function queryRoute( request: Request, @@ -3043,6 +3136,8 @@ export function createStaticHandler( location, matches, requestContext, + null, + false, match ); if (isResponse(result)) { @@ -3079,7 +3174,9 @@ export function createStaticHandler( location: Location, matches: AgnosticDataRouteMatch[], requestContext: unknown, - routeMatch?: AgnosticDataRouteMatch + loadRouteIds: string[] | null, + skipLoaderErrorBubbling: boolean, + routeMatch: AgnosticDataRouteMatch | null ): Promise | Response> { invariant( request.signal, @@ -3093,6 +3190,8 @@ export function createStaticHandler( matches, routeMatch || getTargetMatch(matches, location), requestContext, + loadRouteIds, + skipLoaderErrorBubbling, routeMatch != null ); return result; @@ -3102,6 +3201,8 @@ export function createStaticHandler( request, matches, requestContext, + loadRouteIds, + skipLoaderErrorBubbling, routeMatch ); return isResponse(result) @@ -3112,14 +3213,14 @@ export function createStaticHandler( 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 the user threw/returned a Response in callLoaderOrAction for a + // `queryRoute` call, we throw the `HandlerResult` to bail out early + // and then return or throw the raw Response here accordingly + if (isHandlerResult(e) && isResponse(e.result)) { if (e.type === ResultType.error) { - throw e.response; + throw e.result; } - return e.response; + return e.result; } // Redirects are always returned since they don't propagate to catch // boundaries @@ -3135,6 +3236,8 @@ export function createStaticHandler( matches: AgnosticDataRouteMatch[], actionMatch: AgnosticDataRouteMatch, requestContext: unknown, + loadRouteIds: string[] | null, + skipLoaderErrorBubbling: boolean, isRouteRequest: boolean ): Promise | Response> { let result: DataResult; @@ -3153,17 +3256,15 @@ export function createStaticHandler( error, }; } else { - result = await callLoaderOrAction( + let results = await callDataStrategy( "action", request, - actionMatch, + [actionMatch], matches, - manifest, - mapRouteProperties, - basename, - future.v7_relativeSplatPath, - { isStaticRequest: true, isRouteRequest, requestContext } + isRouteRequest, + requestContext ); + result = results[0]; if (request.signal.aborted) { throwStaticHandlerAbortedError(request, isRouteRequest, future); @@ -3176,9 +3277,9 @@ export function createStaticHandler( // can get back on the "throw all redirect responses" train here should // this ever happen :/ throw new Response(null, { - status: result.status, + status: result.response.status, headers: { - Location: result.location, + Location: result.response.headers.get("Location")!, }, }); } @@ -3215,18 +3316,25 @@ export function createStaticHandler( }; } + // Create a GET request for the loaders + let loaderRequest = new Request(request.url, { + headers: request.headers, + redirect: request.redirect, + signal: request.signal, + }); + if (isErrorResult(result)) { // 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( - request, + loaderRequest, matches, requestContext, - undefined, - { - [boundaryMatch.route.id]: result.error, - } + loadRouteIds, + skipLoaderErrorBubbling, + null, + [boundaryMatch.route.id, result] ); // action status codes take precedence over loader status codes @@ -3242,24 +3350,25 @@ export function createStaticHandler( }; } - // Create a GET request for the loaders - let loaderRequest = new Request(request.url, { - headers: request.headers, - redirect: request.redirect, - signal: request.signal, - }); - let context = await loadRouteData(loaderRequest, matches, requestContext); + let context = await loadRouteData( + loaderRequest, + matches, + requestContext, + loadRouteIds, + skipLoaderErrorBubbling, + null + ); return { ...context, - // action status codes take precedence over loader status codes - ...(result.statusCode ? { statusCode: result.statusCode } : {}), actionData: { [actionMatch.route.id]: result.data, }, - actionHeaders: { - ...(result.headers ? { [actionMatch.route.id]: result.headers } : {}), - }, + // action status codes take precedence over loader status codes + ...(result.statusCode ? { statusCode: result.statusCode } : {}), + actionHeaders: result.headers + ? { [actionMatch.route.id]: result.headers } + : {}, }; } @@ -3267,8 +3376,10 @@ export function createStaticHandler( request: Request, matches: AgnosticDataRouteMatch[], requestContext: unknown, - routeMatch?: AgnosticDataRouteMatch, - pendingActionError?: RouteData + loadRouteIds: string[] | null, + skipLoaderErrorBubbling: boolean, + routeMatch: AgnosticDataRouteMatch | null, + pendingActionResult?: PendingActionResult ): Promise< | Omit< StaticHandlerContext, @@ -3293,14 +3404,19 @@ export function createStaticHandler( let requestMatches = routeMatch ? [routeMatch] - : getLoaderMatchesUntilBoundary( - matches, - Object.keys(pendingActionError || {})[0] - ); + : pendingActionResult && isErrorResult(pendingActionResult[1]) + ? getLoaderMatchesUntilBoundary(matches, pendingActionResult[0]) + : matches; let matchesToLoad = requestMatches.filter( (m) => m.route.loader || m.route.lazy ); + if (loadRouteIds) { + matchesToLoad = matchesToLoad.filter((m) => + loadRouteIds.includes(m.route.id) + ); + } + // Short circuit if we have no loaders to run (query()) if (matchesToLoad.length === 0) { return { @@ -3310,28 +3426,26 @@ export function createStaticHandler( (acc, m) => Object.assign(acc, { [m.route.id]: null }), {} ), - errors: pendingActionError || null, + errors: + pendingActionResult && isErrorResult(pendingActionResult[1]) + ? { + [pendingActionResult[0]]: pendingActionResult[1].error, + } + : null, statusCode: 200, loaderHeaders: {}, activeDeferreds: null, }; } - let results = await Promise.all([ - ...matchesToLoad.map((match) => - callLoaderOrAction( - "loader", - request, - match, - matches, - manifest, - mapRouteProperties, - basename, - future.v7_relativeSplatPath, - { isStaticRequest: true, isRouteRequest, requestContext } - ) - ), - ]); + let results = await callDataStrategy( + "loader", + request, + matchesToLoad, + matches, + isRouteRequest, + requestContext + ); if (request.signal.aborted) { throwStaticHandlerAbortedError(request, isRouteRequest, future); @@ -3343,8 +3457,9 @@ export function createStaticHandler( matches, matchesToLoad, results, - pendingActionError, - activeDeferreds + pendingActionResult, + activeDeferreds, + skipLoaderErrorBubbling ); // Add a null for any non-loader matches for proper revalidation on the client @@ -3367,6 +3482,52 @@ export function createStaticHandler( }; } + // Utility wrapper for calling dataStrategy server-side without having to + // pass around the manifest, mapRouteProperties, etc. + async function callDataStrategy( + type: "loader" | "action", + request: Request, + matchesToLoad: AgnosticDataRouteMatch[], + matches: AgnosticDataRouteMatch[], + isRouteRequest: boolean, + requestContext: unknown + ): Promise { + let results = await callDataStrategyImpl( + dataStrategyImpl, + type, + request, + matchesToLoad, + matches, + manifest, + mapRouteProperties, + requestContext + ); + + return await Promise.all( + results.map((result, i) => { + if (isRedirectHandlerResult(result)) { + let response = result.result as Response; + // Throw redirects and let the server handle them with an HTTP redirect + throw normalizeRelativeRoutingRedirectResponse( + response, + request, + matchesToLoad[i].route.id, + matches, + basename, + future.v7_relativeSplatPath + ); + } + if (isResponse(result.result) && isRouteRequest) { + // For SSR single-route requests, we want to hand Responses back + // directly without unwrapping + throw result; + } + + return convertHandlerResultToDataResult(result); + }) + ); + } + return { dataRoutes, query, @@ -3643,7 +3804,7 @@ function normalizeNavigateOptions( // render so we don't need to load them function getLoaderMatchesUntilBoundary( matches: AgnosticDataRouteMatch[], - boundaryId?: string + boundaryId: string ) { let boundaryMatches = matches; if (boundaryId) { @@ -3662,6 +3823,7 @@ function getMatchesToLoad( submission: Submission | undefined, location: Location, isInitialLoad: boolean, + skipActionErrorRevalidation: boolean, isRevalidationRequired: boolean, cancelledDeferredRoutes: string[], cancelledFetcherLoads: string[], @@ -3670,21 +3832,33 @@ function getMatchesToLoad( fetchRedirectIds: Set, routesToUse: AgnosticDataRouteObject[], basename: string | undefined, - pendingActionData?: RouteData, - pendingError?: RouteData + pendingActionResult?: PendingActionResult ): [AgnosticDataRouteMatch[], RevalidatingFetcher[]] { - let actionResult = pendingError - ? Object.values(pendingError)[0] - : pendingActionData - ? Object.values(pendingActionData)[0] + let actionResult = pendingActionResult + ? isErrorResult(pendingActionResult[1]) + ? pendingActionResult[1].error + : pendingActionResult[1].data : undefined; - let currentUrl = history.createURL(state.location); let nextUrl = history.createURL(location); // Pick navigation matches that are net-new or qualify for revalidation - let boundaryId = pendingError ? Object.keys(pendingError)[0] : undefined; - let boundaryMatches = getLoaderMatchesUntilBoundary(matches, boundaryId); + let boundaryId = + pendingActionResult && isErrorResult(pendingActionResult[1]) + ? pendingActionResult[0] + : undefined; + let boundaryMatches = boundaryId + ? getLoaderMatchesUntilBoundary(matches, boundaryId) + : matches; + + // Don't revalidate loaders by default after action 4xx/5xx responses + // when the flag is enabled. They can still opt-into revalidation via + // `shouldRevalidate` via `actionResult` + let actionStatus = pendingActionResult + ? pendingActionResult[1].statusCode + : undefined; + let shouldSkipRevalidation = + skipActionErrorRevalidation && actionStatus && actionStatus >= 400; let navigationMatches = boundaryMatches.filter((match, index) => { let { route } = match; @@ -3698,7 +3872,7 @@ function getMatchesToLoad( } if (isInitialLoad) { - if (route.loader.hydrate) { + if (typeof route.loader !== "function" || route.loader.hydrate) { return true; } return ( @@ -3730,15 +3904,16 @@ function getMatchesToLoad( nextParams: nextRouteMatch.params, ...submission, actionResult, - defaultShouldRevalidate: - // Forced revalidation due to submission, useRevalidator, or X-Remix-Revalidate - isRevalidationRequired || - // Clicked the same link, resubmitted a GET form - currentUrl.pathname + currentUrl.search === - nextUrl.pathname + nextUrl.search || - // Search params affect all loaders - currentUrl.search !== nextUrl.search || - isNewRouteInstance(currentRouteMatch, nextRouteMatch), + unstable_actionStatus: actionStatus, + defaultShouldRevalidate: shouldSkipRevalidation + ? false + : // Forced revalidation due to submission, useRevalidator, or X-Remix-Revalidate + isRevalidationRequired || + currentUrl.pathname + currentUrl.search === + nextUrl.pathname + nextUrl.search || + // Search params affect all loaders + currentUrl.search !== nextUrl.search || + isNewRouteInstance(currentRouteMatch, nextRouteMatch), }); }); @@ -3808,7 +3983,10 @@ function getMatchesToLoad( nextParams: matches[matches.length - 1].params, ...submission, actionResult, - defaultShouldRevalidate: isRevalidationRequired, + unstable_actionStatus: actionStatus, + defaultShouldRevalidate: shouldSkipRevalidation + ? false + : isRevalidationRequired, }); } @@ -3954,39 +4132,137 @@ async function loadLazyRouteModule( }); } +// Default implementation of `dataStrategy` which fetches all loaders in parallel +function defaultDataStrategy( + opts: DataStrategyFunctionArgs +): ReturnType { + return Promise.all(opts.matches.map((m) => m.resolve())); +} + +async function callDataStrategyImpl( + dataStrategyImpl: DataStrategyFunction, + type: "loader" | "action", + request: Request, + matchesToLoad: AgnosticDataRouteMatch[], + matches: AgnosticDataRouteMatch[], + manifest: RouteManifest, + mapRouteProperties: MapRoutePropertiesFunction, + requestContext?: unknown +): Promise { + let routeIdsToLoad = matchesToLoad.reduce( + (acc, m) => acc.add(m.route.id), + new Set() + ); + let loadedMatches = new Set(); + + // Send all matches here to allow for a middleware-type implementation. + // handler will be a no-op for unneeded routes and we filter those results + // back out below. + let results = await dataStrategyImpl({ + matches: matches.map((match) => { + let shouldLoad = routeIdsToLoad.has(match.route.id); + // `resolve` encapsulates the route.lazy, executing the + // loader/action, and mapping return values/thrown errors to a + // HandlerResult. Users can pass a callback to take fine-grained control + // over the execution of the loader/action + let resolve: DataStrategyMatch["resolve"] = (handlerOverride) => { + loadedMatches.add(match.route.id); + return shouldLoad + ? callLoaderOrAction( + type, + request, + match, + manifest, + mapRouteProperties, + handlerOverride, + requestContext + ) + : Promise.resolve({ type: ResultType.data, result: undefined }); + }; + + return { + ...match, + shouldLoad, + resolve, + }; + }), + request, + params: matches[0].params, + }); + + // Throw if any loadRoute implementations not called since they are what + // ensures a route is fully loaded + matches.forEach((m) => + invariant( + loadedMatches.has(m.route.id), + `\`match.resolve()\` was not called for route id "${m.route.id}". ` + + "You must call `match.resolve()` on every match passed to " + + "`dataStrategy` to ensure all routes are properly loaded." + ) + ); + + // Filter out any middleware-only matches for which we didn't need to run handlers + return results.filter((_, i) => routeIdsToLoad.has(matches[i].route.id)); +} + +// Default logic for calling a loader/action is the user has no specified a dataStrategy async function callLoaderOrAction( type: "loader" | "action", request: Request, match: AgnosticDataRouteMatch, - matches: AgnosticDataRouteMatch[], manifest: RouteManifest, mapRouteProperties: MapRoutePropertiesFunction, - basename: string, - v7_relativeSplatPath: boolean, - opts: { - isStaticRequest?: boolean; - isRouteRequest?: boolean; - requestContext?: unknown; - } = {} -): Promise { - let resultType; - let result; + handlerOverride: Parameters[0], + staticContext?: unknown +): Promise { + let result: HandlerResult; let onReject: (() => void) | undefined; - let runHandler = (handler: ActionFunction | LoaderFunction) => { + let runHandler = ( + handler: AgnosticRouteObject["loader"] | AgnosticRouteObject["action"] + ): Promise => { // Setup a promise we can race against so that abort signals short circuit let reject: () => void; - let abortPromise = new Promise((_, r) => (reject = r)); + // This will never resolve so safe to type it as Promise to + // satisfy the function return value + let abortPromise = new Promise((_, r) => (reject = r)); onReject = () => reject(); request.signal.addEventListener("abort", onReject); - return Promise.race([ - handler({ - request, - params: match.params, - context: opts.requestContext, - }), - abortPromise, - ]); + + let actualHandler = (ctx?: unknown) => { + if (typeof handler !== "function") { + return Promise.reject( + new Error( + `You cannot call the handler for a route which defines a boolean ` + + `"${type}" [routeId: ${match.route.id}]` + ) + ); + } + return handler( + { + request, + params: match.params, + context: staticContext, + }, + ...(ctx !== undefined ? [ctx] : []) + ); + }; + + let handlerPromise: Promise; + if (handlerOverride) { + handlerPromise = handlerOverride((ctx: unknown) => actualHandler(ctx)); + } else { + handlerPromise = (async () => { + try { + let val = await actualHandler(); + return { type: "data", result: val }; + } catch (e) { + return { type: "error", result: e }; + } + })(); + } + + return Promise.race([handlerPromise, abortPromise]); }; try { @@ -3996,7 +4272,7 @@ async function callLoaderOrAction( if (handler) { // Run statically defined handler in parallel with lazy() let handlerError; - let values = await Promise.all([ + let [value] = await Promise.all([ // If the handler throws, don't let it immediately bubble out, // since we need to let the lazy() execution finish so we know if this // route has a boundary that can handle the error @@ -4005,17 +4281,17 @@ async function callLoaderOrAction( }), loadLazyRouteModule(match.route, mapRouteProperties, manifest), ]); - if (handlerError) { + if (handlerError !== undefined) { throw handlerError; } - result = values[0]; + result = value!; } else { // Load lazy route module, then run any returned handler await loadLazyRouteModule(match.route, mapRouteProperties, manifest); handler = match.route[type]; if (handler) { - // Handler still run even if we got interrupted to maintain consistency + // Handler still runs even if we got interrupted to maintain consistency // with un-abortable behavior of handler execution on non-lazy or // previously-lazy-loaded routes result = await runHandler(handler); @@ -4030,7 +4306,7 @@ async function callLoaderOrAction( } else { // lazy() route has no loader to run. Short circuit here so we don't // hit the invariant below that errors on returning undefined. - return { type: ResultType.data, data: undefined }; + return { type: ResultType.data, result: undefined }; } } } else if (!handler) { @@ -4044,85 +4320,31 @@ async function callLoaderOrAction( } invariant( - result !== undefined, + result.result !== undefined, `You defined ${type === "action" ? "an action" : "a loader"} for route ` + `"${match.route.id}" but didn't return anything from your \`${type}\` ` + `function. Please return a value or \`null\`.` ); } catch (e) { - resultType = ResultType.error; - result = e; + // We should already be catching and converting normal handler executions to + // HandlerResults and returning them, so anything that throws here is an + // unexpected error we still need to wrap + return { type: ResultType.error, result: e }; } finally { if (onReject) { request.signal.removeEventListener("abort", onReject); } } - if (isResponse(result)) { - let status = result.status; - - // Process redirects - if (redirectStatusCodes.has(status)) { - let location = result.headers.get("Location"); - invariant( - location, - "Redirects returned/thrown from loaders/actions must have a Location header" - ); - - // Support relative routing in internal redirects - if (!ABSOLUTE_URL_REGEX.test(location)) { - location = normalizeTo( - new URL(request.url), - matches.slice(0, matches.indexOf(match) + 1), - basename, - true, - location, - v7_relativeSplatPath - ); - } else if (!opts.isStaticRequest) { - // Strip off the protocol+origin for same-origin + same-basename absolute - // redirects. If this is a static request, we can let it go back to the - // browser as-is - let currentUrl = new URL(request.url); - let url = location.startsWith("//") - ? new URL(currentUrl.protocol + location) - : new URL(location); - let isSameBasename = stripBasename(url.pathname, basename) != null; - if (url.origin === currentUrl.origin && isSameBasename) { - location = url.pathname + url.search + url.hash; - } - } - - // Don't process redirects in the router during static requests requests. - // Instead, throw the Response and let the server handle it with an HTTP - // redirect. We also update the Location header in place in this flow so - // basename and relative routing is taken into account - if (opts.isStaticRequest) { - result.headers.set("Location", location); - throw result; - } - - return { - type: ResultType.redirect, - status, - location, - revalidate: result.headers.get("X-Remix-Revalidate") !== null, - reloadDocument: result.headers.get("X-Remix-Reload-Document") !== null, - }; - } + return result; +} - // For SSR single-route requests, we want to hand Responses back directly - // without unwrapping. We do this with the QueryRouteResponse wrapper - // interface so we can know whether it was returned or thrown - if (opts.isRouteRequest) { - let queryRouteResponse: QueryRouteResponse = { - type: - resultType === ResultType.error ? ResultType.error : ResultType.data, - response: result, - }; - throw queryRouteResponse; - } +async function convertHandlerResultToDataResult( + handlerResult: HandlerResult +): Promise { + let { result, type, status } = handlerResult; + if (isResponse(result)) { let data: any; try { @@ -4142,10 +4364,11 @@ async function callLoaderOrAction( return { type: ResultType.error, error: e }; } - if (resultType === ResultType.error) { + if (type === ResultType.error) { return { - type: resultType, - error: new ErrorResponseImpl(status, result.statusText, data), + type: ResultType.error, + error: new ErrorResponseImpl(result.status, result.statusText, data), + statusCode: result.status, headers: result.headers, }; } @@ -4158,8 +4381,12 @@ async function callLoaderOrAction( }; } - if (resultType === ResultType.error) { - return { type: resultType, error: result }; + if (type === ResultType.error) { + return { + type: ResultType.error, + error: result, + statusCode: isRouteErrorResponse(result) ? result.status : status, + }; } if (isDeferredData(result)) { @@ -4171,7 +4398,60 @@ async function callLoaderOrAction( }; } - return { type: ResultType.data, data: result }; + return { type: ResultType.data, data: result, statusCode: status }; +} + +// Support relative routing in internal redirects +function normalizeRelativeRoutingRedirectResponse( + response: Response, + request: Request, + routeId: string, + matches: AgnosticDataRouteMatch[], + basename: string, + v7_relativeSplatPath: boolean +) { + let location = response.headers.get("Location"); + invariant( + location, + "Redirects returned/thrown from loaders/actions must have a Location header" + ); + + if (!ABSOLUTE_URL_REGEX.test(location)) { + let trimmedMatches = matches.slice( + 0, + matches.findIndex((m) => m.route.id === routeId) + 1 + ); + location = normalizeTo( + new URL(request.url), + trimmedMatches, + basename, + true, + location, + v7_relativeSplatPath + ); + response.headers.set("Location", location); + } + + return response; +} + +function normalizeRedirectLocation( + location: string, + currentUrl: URL, + basename: string +): string { + if (ABSOLUTE_URL_REGEX.test(location)) { + // Strip off the protocol+origin for same-origin + same-basename absolute redirects + let normalizedLocation = location; + let url = normalizedLocation.startsWith("//") + ? new URL(currentUrl.protocol + normalizedLocation) + : new URL(normalizedLocation); + let isSameBasename = stripBasename(url.pathname, basename) != null; + if (url.origin === currentUrl.origin && isSameBasename) { + return url.pathname + url.search + url.hash; + } + } + return location; } // Utility method for creating the Request instances for loaders/actions during @@ -4239,8 +4519,9 @@ function processRouteLoaderData( matches: AgnosticDataRouteMatch[], matchesToLoad: AgnosticDataRouteMatch[], results: DataResult[], - pendingError: RouteData | undefined, - activeDeferreds: Map + pendingActionResult: PendingActionResult | undefined, + activeDeferreds: Map, + skipLoaderErrorBubbling: boolean ): { loaderData: RouterState["loaderData"]; errors: RouterState["errors"] | null; @@ -4253,6 +4534,10 @@ function processRouteLoaderData( let statusCode: number | undefined; let foundError = false; let loaderHeaders: Record = {}; + let pendingError = + pendingActionResult && isErrorResult(pendingActionResult[1]) + ? pendingActionResult[1].error + : undefined; // Process loader results into state.loaderData/state.errors results.forEach((result, index) => { @@ -4262,23 +4547,27 @@ function processRouteLoaderData( "Cannot handle redirect results in processLoaderData" ); if (isErrorResult(result)) { - // Look upwards from the matched route for the closest ancestor - // error boundary, defaulting to the root match - let boundaryMatch = findNearestBoundary(matches, id); let error = result.error; // If we have a pending action error, we report it at the highest-route // that throws a loader error, and then clear it out to indicate that // it was consumed - if (pendingError) { - error = Object.values(pendingError)[0]; + if (pendingError !== undefined) { + error = pendingError; pendingError = undefined; } errors = errors || {}; - // Prefer higher error values if lower errors bubble to the same boundary - if (errors[boundaryMatch.route.id] == null) { - errors[boundaryMatch.route.id] = error; + if (skipLoaderErrorBubbling) { + errors[id] = error; + } else { + // Look upwards from the matched route for the closest ancestor error + // boundary, defaulting to the root match. Prefer higher error values + // if lower errors bubble to the same boundary + let boundaryMatch = findNearestBoundary(matches, id); + if (errors[boundaryMatch.route.id] == null) { + errors[boundaryMatch.route.id] = error; + } } // Clear our any prior loaderData for the throwing route @@ -4299,21 +4588,28 @@ function processRouteLoaderData( if (isDeferredResult(result)) { activeDeferreds.set(id, result.deferredData); loaderData[id] = result.deferredData.data; + // Error status codes always override success status codes, but if all + // loaders are successful we take the deepest status code. + if ( + result.statusCode != null && + result.statusCode !== 200 && + !foundError + ) { + statusCode = result.statusCode; + } + if (result.headers) { + loaderHeaders[id] = result.headers; + } } else { loaderData[id] = result.data; - } - - // Error status codes always override success status codes, but if all - // loaders are successful we take the deepest status code. - if ( - result.statusCode != null && - result.statusCode !== 200 && - !foundError - ) { - statusCode = result.statusCode; - } - if (result.headers) { - loaderHeaders[id] = result.headers; + // Error status codes always override success status codes, but if all + // loaders are successful we take the deepest status code. + if (result.statusCode && result.statusCode !== 200 && !foundError) { + statusCode = result.statusCode; + } + if (result.headers) { + loaderHeaders[id] = result.headers; + } } } }); @@ -4321,9 +4617,9 @@ function processRouteLoaderData( // If we didn't consume the pending action error (i.e., all loaders // resolved), then consume it here. Also clear out any loaderData for the // throwing route - if (pendingError) { - errors = pendingError; - loaderData[Object.keys(pendingError)[0]] = undefined; + if (pendingError !== undefined && pendingActionResult) { + errors = { [pendingActionResult[0]]: pendingError }; + loaderData[pendingActionResult[0]] = undefined; } return { @@ -4339,7 +4635,7 @@ function processLoaderData( matches: AgnosticDataRouteMatch[], matchesToLoad: AgnosticDataRouteMatch[], results: DataResult[], - pendingError: RouteData | undefined, + pendingActionResult: PendingActionResult | undefined, revalidatingFetchers: RevalidatingFetcher[], fetcherResults: DataResult[], activeDeferreds: Map @@ -4351,8 +4647,9 @@ function processLoaderData( matches, matchesToLoad, results, - pendingError, - activeDeferreds + pendingActionResult, + activeDeferreds, + false // This method is only called client side so we always want to bubble ); // Process results from our revalidating fetchers @@ -4425,6 +4722,24 @@ function mergeLoaderData( return mergedLoaderData; } +function getActionDataForCommit( + pendingActionResult: PendingActionResult | undefined +) { + if (!pendingActionResult) { + return {}; + } + return isErrorResult(pendingActionResult[1]) + ? { + // Clear out prior actionData on errors + actionData: {}, + } + : { + actionData: { + [pendingActionResult[0]]: pendingActionResult[1].data, + }, + }; +} + // Find the nearest error boundary, looking upwards from the leaf route (or the // route specified by routeId) for the closest ancestor error boundary, // defaulting to the root match @@ -4559,6 +4874,22 @@ function isHashChangeOnly(a: Location, b: Location): boolean { return false; } +function isHandlerResult(result: unknown): result is HandlerResult { + return ( + result != null && + typeof result === "object" && + "type" in result && + "result" in result && + (result.type === ResultType.data || result.type === ResultType.error) + ); +} + +function isRedirectHandlerResult(result: HandlerResult) { + return ( + isResponse(result.result) && redirectStatusCodes.has(result.result.status) + ); +} + function isDeferredResult(result: DataResult): result is DeferredResult { return result.type === ResultType.deferred; } @@ -4603,14 +4934,6 @@ function isRedirectResponse(result: any): result is Response { return status >= 300 && status <= 399 && location != null; } -function isQueryRouteResponse(obj: any): obj is QueryRouteResponse { - return ( - obj && - isResponse(obj.response) && - (obj.type === ResultType.data || obj.type === ResultType.error) - ); -} - function isValidMethod(method: string): method is FormMethod | V7_FormMethod { return validRequestMethods.has(method.toLowerCase() as FormMethod); } diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 4bb9f48e1d..561421e777 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -20,7 +20,7 @@ export enum ResultType { */ export interface SuccessResult { type: ResultType.data; - data: any; + data: unknown; statusCode?: number; headers?: Headers; } @@ -40,10 +40,8 @@ export interface DeferredResult { */ export interface RedirectResult { type: ResultType.redirect; - status: number; - location: string; - revalidate: boolean; - reloadDocument?: boolean; + // We keep the raw Response for redirects so we can return it verbatim + response: Response; } /** @@ -51,7 +49,8 @@ export interface RedirectResult { */ export interface ErrorResult { type: ResultType.error; - error: any; + error: unknown; + statusCode?: number; headers?: Headers; } @@ -64,6 +63,15 @@ export type DataResult = | RedirectResult | ErrorResult; +/** + * Result from a loader or action called via dataStrategy + */ +export interface HandlerResult { + type: "data" | "error"; + result: unknown; // data, Error, Response, DeferredData + status?: number; +} + type LowerCaseFormMethod = "get" | "post" | "put" | "patch" | "delete"; type UpperCaseFormMethod = Uppercase; @@ -166,22 +174,26 @@ export interface ActionFunctionArgs */ type DataFunctionValue = Response | NonNullable | null; +type DataFunctionReturnValue = Promise | DataFunctionValue; + /** * Route loader function signature */ export type LoaderFunction = { - (args: LoaderFunctionArgs): - | Promise - | DataFunctionValue; + ( + args: LoaderFunctionArgs, + handlerCtx?: unknown + ): DataFunctionReturnValue; } & { hydrate?: boolean }; /** * Route action function signature */ export interface ActionFunction { - (args: ActionFunctionArgs): - | Promise - | DataFunctionValue; + ( + args: ActionFunctionArgs, + handlerCtx?: unknown + ): DataFunctionReturnValue; } /** @@ -198,6 +210,7 @@ export interface ShouldRevalidateFunctionArgs { text?: Submission["text"]; formData?: Submission["formData"]; json?: Submission["json"]; + unstable_actionStatus?: number; actionResult?: any; defaultShouldRevalidate: boolean; } @@ -223,6 +236,25 @@ export interface DetectErrorBoundaryFunction { (route: AgnosticRouteObject): boolean; } +export interface DataStrategyMatch + extends AgnosticRouteMatch { + shouldLoad: boolean; + resolve: ( + handlerOverride?: ( + handler: (ctx?: unknown) => DataFunctionReturnValue + ) => Promise + ) => Promise; +} + +export interface DataStrategyFunctionArgs + extends DataFunctionArgs { + matches: DataStrategyMatch[]; +} + +export interface DataStrategyFunction { + (args: DataStrategyFunctionArgs): Promise; +} + /** * Function provided by the framework-aware layers to set any framework-specific * properties from framework-agnostic properties @@ -277,8 +309,8 @@ type AgnosticBaseRouteObject = { caseSensitive?: boolean; path?: string; id?: string; - loader?: LoaderFunction; - action?: ActionFunction; + loader?: LoaderFunction | boolean; + action?: ActionFunction | boolean; hasErrorBoundary?: boolean; shouldRevalidate?: ShouldRevalidateFunction; handle?: any;