From faf262e36cccc71b37c1c1af893b95f474d7a9ca Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 11 Mar 2024 16:56:59 -0400 Subject: [PATCH] Lift redirects to the root for single fetch loaders --- integration/client-data-test.ts | 6 +- integration/defer-test.ts | 5 +- integration/error-data-request-test.ts | 26 ++--- integration/error-sanitization-test.ts | 124 ++++++++++++++---------- integration/loader-test.ts | 6 +- packages/remix-react/single-fetch.tsx | 23 +++-- packages/remix-server-runtime/index.ts | 4 + packages/remix-server-runtime/server.ts | 49 ++++------ 8 files changed, 134 insertions(+), 109 deletions(-) diff --git a/integration/client-data-test.ts b/integration/client-data-test.ts index 4823e2abafa..2269fd41fcb 100644 --- a/integration/client-data-test.ts +++ b/integration/client-data-test.ts @@ -831,13 +831,15 @@ test.describe("Client Data", () => { ); let app = new PlaywrightFixture(appFixture, page); - await app.goto("/parent/child"); + await app.goto("/parent/child", false); let html = await app.getHtml("main"); expect(html).toMatch("Parent Server Loader

"); expect(html).toMatch("Child Server Error"); expect(html).not.toMatch("Should not see me"); // Ensure we hydrate and remain on the boundary - await new Promise((r) => setTimeout(r, 100)); + await page.waitForSelector( + ":has-text('Parent Server Loader (mutated by client)')" + ); html = await app.getHtml("main"); expect(html).toMatch("Parent Server Loader (mutated by client)

"); expect(html).toMatch("Child Server Error"); diff --git a/integration/defer-test.ts b/integration/defer-test.ts index ca15faf6f82..487be7f8b03 100644 --- a/integration/defer-test.ts +++ b/integration/defer-test.ts @@ -2304,6 +2304,7 @@ test.describe("single fetch", () => { // Exported for use by the server runtime so we can abort the // turbo-stream encode() call export const streamTimeout = 250; + const renderTimeout = streamTimeout + 250; export default function handleRequest( request: Request, @@ -2365,7 +2366,7 @@ test.describe("single fetch", () => { } ); - setTimeout(abort, streamTimeout); + setTimeout(abort, renderTimeout); }); } @@ -2407,7 +2408,7 @@ test.describe("single fetch", () => { } ); - setTimeout(abort, streamTimeout); + setTimeout(abort, renderTimeout); }); } `, diff --git a/integration/error-data-request-test.ts b/integration/error-data-request-test.ts index b19e8cea4f2..907c4fb83b3 100644 --- a/integration/error-data-request-test.ts +++ b/integration/error-data-request-test.ts @@ -292,11 +292,13 @@ test.describe("single fetch", () => { expect(status).toBe(200); expect(headers.has("X-Remix-Error")).toBe(false); expect(data).toEqual({ - root: { - data: null, - }, - "routes/_index": { - data: null, + results: { + root: { + data: null, + }, + "routes/_index": { + data: null, + }, }, }); }); @@ -339,12 +341,14 @@ test.describe("single fetch", () => { expect(status).toBe(404); expect(headers.has("X-Remix-Error")).toBe(false); expect(data).toEqual({ - root: { - error: new ErrorResponseImpl( - 404, - "Not Found", - 'Error: No route matches URL "/i/match/nothing"' - ), + results: { + root: { + error: new ErrorResponseImpl( + 404, + "Not Found", + 'Error: No route matches URL "/i/match/nothing"' + ), + }, }, }); assertLoggedErrorInstance('No route matches URL "/i/match/nothing"'); diff --git a/integration/error-sanitization-test.ts b/integration/error-sanitization-test.ts index 40cae6ab043..18a01d40e10 100644 --- a/integration/error-sanitization-test.ts +++ b/integration/error-sanitization-test.ts @@ -757,11 +757,13 @@ test.describe("single fetch", () => { test("returns data without errors", async () => { let { data } = await fixture.requestSingleFetchData("/_root.data"); expect(data).toEqual({ - root: { - data: null, - }, - "routes/_index": { - data: "LOADER", + results: { + root: { + data: null, + }, + "routes/_index": { + data: "LOADER", + }, }, }); }); @@ -771,11 +773,13 @@ test.describe("single fetch", () => { "/_root.data?loader" ); expect(data).toEqual({ - root: { - data: null, - }, - "routes/_index": { - error: new Error("Unexpected Server Error"), + results: { + root: { + data: null, + }, + "routes/_index": { + error: new Error("Unexpected Server Error"), + }, }, }); expect(errorLogs.length).toBe(1); @@ -786,7 +790,9 @@ test.describe("single fetch", () => { test("returns deferred data without errors", async () => { let { data } = await fixture.requestSingleFetchData("/defer.data"); // @ts-expect-error - expect(await data["routes/defer"].data.lazy).toEqual("RESOLVED"); + expect(await data.results["routes/defer"].data.lazy).toEqual( + "RESOLVED" + ); }); test("sanitizes loader errors in deferred data requests", async () => { @@ -795,7 +801,7 @@ test.describe("single fetch", () => { ); try { // @ts-expect-error - await data["routes/defer"].data.lazy; + await data.results["routes/defer"].data.lazy; expect(true).toBe(false); } catch (e) { expect((e as Error).message).toBe("Unexpected Server Error"); @@ -820,12 +826,14 @@ test.describe("single fetch", () => { "/not-a-route.data" ); expect(data).toEqual({ - root: { - error: new ErrorResponseImpl( - 404, - "Not Found", - 'Error: No route matches URL "/not-a-route"' - ), + results: { + root: { + error: new ErrorResponseImpl( + 404, + "Not Found", + 'Error: No route matches URL "/not-a-route"' + ), + }, }, }); expect(errorLogs).toEqual([ @@ -930,11 +938,13 @@ test.describe("single fetch", () => { test("returns data without errors", async () => { let { data } = await fixture.requestSingleFetchData("/_root.data"); expect(data).toEqual({ - root: { - data: null, - }, - "routes/_index": { - data: "LOADER", + results: { + root: { + data: null, + }, + "routes/_index": { + data: "LOADER", + }, }, }); }); @@ -944,11 +954,13 @@ test.describe("single fetch", () => { "/_root.data?loader" ); expect(data).toEqual({ - root: { - data: null, - }, - "routes/_index": { - error: new Error("Loader Error"), + results: { + root: { + data: null, + }, + "routes/_index": { + error: new Error("Loader Error"), + }, }, }); expect(errorLogs.length).toBe(1); @@ -959,7 +971,9 @@ test.describe("single fetch", () => { test("returns deferred data without errors", async () => { let { data } = await fixture.requestSingleFetchData("/defer.data"); // @ts-expect-error - expect(await data["routes/defer"].data.lazy).toEqual("RESOLVED"); + expect(await data.results["routes/defer"].data.lazy).toEqual( + "RESOLVED" + ); }); test("does not sanitize loader errors in deferred data requests", async () => { @@ -968,7 +982,7 @@ test.describe("single fetch", () => { ); try { // @ts-expect-error - await data["routes/defer"].data.lazy; + await data.results["routes/defer"].data.lazy; expect(true).toBe(false); } catch (e) { expect((e as Error).message).toBe("REJECTED"); @@ -994,12 +1008,14 @@ test.describe("single fetch", () => { "/not-a-route.data" ); expect(data).toEqual({ - root: { - error: new ErrorResponseImpl( - 404, - "Not Found", - 'Error: No route matches URL "/not-a-route"' - ), + results: { + root: { + error: new ErrorResponseImpl( + 404, + "Not Found", + 'Error: No route matches URL "/not-a-route"' + ), + }, }, }); expect(errorLogs).toEqual([ @@ -1198,11 +1214,13 @@ test.describe("single fetch", () => { test("returns data without errors", async () => { let { data } = await fixture.requestSingleFetchData("/_root.data"); expect(data).toEqual({ - root: { - data: null, - }, - "routes/_index": { - data: "LOADER", + results: { + root: { + data: null, + }, + "routes/_index": { + data: "LOADER", + }, }, }); }); @@ -1212,8 +1230,10 @@ test.describe("single fetch", () => { "/_root.data?loader" ); expect(data).toEqual({ - root: { data: null }, - "routes/_index": { error: new Error("Unexpected Server Error") }, + results: { + root: { data: null }, + "routes/_index": { error: new Error("Unexpected Server Error") }, + }, }); expect(errorLogs[0][0]).toEqual("App Specific Error Logging:"); expect(errorLogs[1][0]).toEqual( @@ -1227,7 +1247,7 @@ test.describe("single fetch", () => { test("returns deferred data without errors", async () => { let { data } = await fixture.requestSingleFetchData("/defer.data"); // @ts-expect-error - expect(await data["routes/defer"].data.lazy).toBe("RESOLVED"); + expect(await data.results["routes/defer"].data.lazy).toBe("RESOLVED"); }); test("sanitizes loader errors in deferred data requests", async () => { @@ -1236,7 +1256,7 @@ test.describe("single fetch", () => { ); try { // @ts-expect-error - await data["routes/defer"].data.lazy; + await data.results["routes/defer"].data.lazy; expect(true).toBe(false); } catch (e) { expect((e as Error).message).toBe("Unexpected Server Error"); @@ -1266,12 +1286,14 @@ test.describe("single fetch", () => { "/not-a-route.data" ); expect(data).toEqual({ - root: { - error: new ErrorResponseImpl( - 404, - "Not Found", - 'Error: No route matches URL "/not-a-route"' - ), + results: { + root: { + error: new ErrorResponseImpl( + 404, + "Not Found", + 'Error: No route matches URL "/not-a-route"' + ), + }, }, }); expect(errorLogs[0][0]).toEqual("App Specific Error Logging:"); diff --git a/integration/loader-test.ts b/integration/loader-test.ts index 86e64def002..07867b474d8 100644 --- a/integration/loader-test.ts +++ b/integration/loader-test.ts @@ -195,8 +195,10 @@ test.describe("single fetch", () => { test("returns responses for single fetch routes", async () => { let { data } = await fixture.requestSingleFetchData("/_root.data"); expect(data).toEqual({ - root: { data: ROOT_DATA }, - "routes/_index": { data: INDEX_DATA }, + results: { + root: { data: ROOT_DATA }, + "routes/_index": { data: INDEX_DATA }, + }, }); }); }); diff --git a/packages/remix-react/single-fetch.tsx b/packages/remix-react/single-fetch.tsx index 68c54a78de2..cc5c19dae13 100644 --- a/packages/remix-react/single-fetch.tsx +++ b/packages/remix-react/single-fetch.tsx @@ -7,6 +7,10 @@ import { UNSAFE_ErrorResponseImpl as ErrorResponseImpl, redirect, } from "@remix-run/router"; +import type { + UNSAFE_SingleFetchResult as SingleFetchResult, + UNSAFE_SingleFetchResults as SingleFetchResults, +} from "@remix-run/server-runtime"; import type { DataRouteObject, unstable_DataStrategyFunctionArgs as DataStrategyFunctionArgs, @@ -18,15 +22,6 @@ import type { AssetsManifest, EntryContext } from "./entry"; import type { RouteModules } from "./routeModules"; import invariant from "./invariant"; -// IMPORTANT! Keep in sync with the types in @remix-run/server-runtime -type SingleFetchResult = - | { data: unknown } - | { error: unknown } - | { redirect: string; status: number; revalidate: boolean; reload: boolean }; -type SingleFetchResults = { - [key: string]: SingleFetchResult; -}; - interface StreamTransferProps { context: EntryContext; identifier: number; @@ -158,9 +153,13 @@ export function getSingleFetchDataStrategy( singleFetchPromise = makeSingleFetchCall(); } let results = await singleFetchPromise; - return results[routeId] !== undefined - ? unwrapSingleFetchResult(results[routeId], routeId) - : null; + if ("redirect" in results) { + return unwrapSingleFetchResult(results, routeId); + } else { + return results.results[routeId] !== undefined + ? unwrapSingleFetchResult(results.results[routeId], routeId) + : null; + } }; } diff --git a/packages/remix-server-runtime/index.ts b/packages/remix-server-runtime/index.ts index 2d813bf522e..5217e33e1e1 100644 --- a/packages/remix-server-runtime/index.ts +++ b/packages/remix-server-runtime/index.ts @@ -5,6 +5,10 @@ export { parseMultipartFormData as unstable_parseMultipartFormData, } from "./formData"; export { defer, json, redirect, redirectDocument } from "./responses"; +export type { + SingleFetchResult as UNSAFE_SingleFetchResult, + SingleFetchResults as UNSAFE_SingleFetchResults, +} from "./server"; export { createRequestHandler } from "./server"; export { createSession, diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index 1f35931d59e..47a4b595496 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -312,14 +312,23 @@ async function handleDataRequest( } } -// IMPORTANT! Keep in sync with the types in @remix-run/react -type SingleFetchResult = +type SingleFetchRedirectResult = { + redirect: string; + status: number; + revalidate: boolean; + reload: boolean; +}; +export type SingleFetchResult = | { data: unknown } | { error: unknown } - | { redirect: string; status: number; revalidate: boolean; reload: boolean }; -type SingleFetchResults = { - [key: string]: SingleFetchResult; + | SingleFetchRedirectResult; + +export type SingleFetchDataResults = { + results: { [key: string]: SingleFetchResult }; }; +export type SingleFetchResults = + | SingleFetchDataResults + | SingleFetchRedirectResult; async function handleSingleFetchRequest( serverMode: ServerMode, @@ -344,7 +353,6 @@ async function handleSingleFetchRequest( request, handlerUrl, staticHandler, - matches, loadContext, handleError, serverMode, @@ -438,7 +446,6 @@ async function singleFetchLoaders( request: Request, handlerUrl: URL, staticHandler: StaticHandler, - matches: RouteMatch[] | null, loadContext: AppLoadContext, handleError: (err: unknown) => void, serverMode: ServerMode, @@ -458,24 +465,8 @@ async function singleFetchLoaders( skipLoaderErrorBubbling: true, }); if (isResponse(result)) { - let redirect = getSingleFetchRedirect(result); - - // We don't really know which loader this came from, include for all routes - // TODO: This feels like a hack - change internal query behavior here? - let results = loadRouteIds - ? loadRouteIds.reduce( - (acc, routeId) => Object.assign(acc, { [routeId]: redirect }), - {} - ) - : matches! - .filter((m) => m.route.module.loader) - .reduce( - (acc, m) => Object.assign(acc, { [m.route.id]: redirect }), - {} - ); - return { - result: results, + result: getSingleFetchRedirect(result), headers: result.headers, status: 200, // Don't want the `fetch` call to follow the redirect }; @@ -496,7 +487,7 @@ async function singleFetchLoaders( // Aggregate results based on the matches we intended to load since we get // `null` values back in `context.loaderData` for routes we didn't load - let results: SingleFetchResults = {}; + let results: SingleFetchDataResults = { results: {} }; let loadedMatches = loadRouteIds ? context.matches.filter( (m) => m.route.loader && loadRouteIds!.includes(m.route.id) @@ -506,9 +497,9 @@ async function singleFetchLoaders( let data = context.loaderData?.[m.route.id]; let error = context.errors?.[m.route.id]; if (error !== undefined) { - results[m.route.id] = { error }; + results.results[m.route.id] = { error }; } else if (data !== undefined) { - results[m.route.id] = { data }; + results.results[m.route.id] = { data }; } }); @@ -520,7 +511,7 @@ async function singleFetchLoaders( } catch (error: unknown) { handleError(error); return { - result: { root: { error } }, + result: { results: { root: { error } } }, headers: new Headers(), status: 500, }; @@ -785,7 +776,7 @@ function unwrapResponse(response: Response) { : response.text(); } -function getSingleFetchRedirect(response: Response): SingleFetchResult { +function getSingleFetchRedirect(response: Response): SingleFetchRedirectResult { return { redirect: response.headers.get("Location")!, status: response.status,