diff --git a/.changeset/mean-falcons-love.md b/.changeset/mean-falcons-love.md new file mode 100644 index 00000000000..43b1742b591 --- /dev/null +++ b/.changeset/mean-falcons-love.md @@ -0,0 +1,5 @@ +--- +"@remix-run/react": patch +--- + +[REMOVE] Fix a few edge cases with client data (throw if server handler doesnt exist, and only return initialData on first loader call) diff --git a/integration/client-data-test.ts b/integration/client-data-test.ts index 5e4bb543abd..a4b26c0d97f 100644 --- a/integration/client-data-test.ts +++ b/integration/client-data-test.ts @@ -315,7 +315,6 @@ test.describe("Client Data", () => { childClientLoader: false, childClientLoaderHydrate: false, }), - // Blow away parent.child.tsx with our own deferred version "app/routes/parent.child.tsx": js` import * as React from 'react'; import { defer, json } from '@remix-run/node' @@ -415,7 +414,6 @@ test.describe("Client Data", () => { childClientLoader: false, childClientLoaderHydrate: false, }), - // Blow away parent.child.tsx with our own version "app/routes/parent.child.tsx": js` import * as React from 'react'; import { json } from '@remix-run/node'; @@ -470,7 +468,6 @@ test.describe("Client Data", () => { childClientLoader: false, childClientLoaderHydrate: false, }), - // Blow away parent.child.tsx with our own version without a server loader "app/routes/parent.child.tsx": js` import * as React from 'react'; import { useLoaderData } from '@remix-run/react'; @@ -514,7 +511,6 @@ test.describe("Client Data", () => { childClientLoader: false, childClientLoaderHydrate: false, }), - // Blow away parent.child.tsx with our own version without a server loader "app/routes/parent.child.tsx": js` import * as React from 'react'; import { useLoaderData } from '@remix-run/react'; @@ -545,6 +541,189 @@ test.describe("Client Data", () => { html = await app.getHtml("main"); expect(html).toMatch("Loader Data (clientLoader only)"); }); + + test("throws a 400 if you call serverLoader without a server loader", async ({ + page, + }) => { + appFixture = await createAppFixture( + await createFixture({ + files: { + ...getFiles({ + parentClientLoader: false, + parentClientLoaderHydrate: false, + childClientLoader: false, + childClientLoaderHydrate: false, + }), + "app/routes/parent.child.tsx": js` + import * as React from 'react'; + import { useLoaderData, useRouteError } from '@remix-run/react'; + export async function clientLoader({ serverLoader }) { + return await serverLoader(); + } + export default function Component() { + return

Child

; + } + export function HydrateFallback() { + return

Loading...

; + } + export function ErrorBoundary() { + let error = useRouteError(); + return

{error.status} {error.data}

; + } + `, + }, + }) + ); + let app = new PlaywrightFixture(appFixture, page); + + await app.goto("/parent/child"); + await page.waitForSelector("#child-error"); + let html = await app.getHtml("#child-error"); + expect(html.replace(/\n/g, " ").replace(/ +/g, " ")).toMatch( + "400 Error: You are trying to call serverLoader() on a route that does " + + 'not have a server loader (routeId: "routes/parent.child")' + ); + }); + + test("initial hydration data check functions properly", async ({ + page, + }) => { + appFixture = await createAppFixture( + await createFixture({ + files: { + ...getFiles({ + parentClientLoader: false, + parentClientLoaderHydrate: false, + childClientLoader: false, + childClientLoaderHydrate: false, + }), + "app/routes/parent.child.tsx": js` + import * as React from 'react'; + import { json } from '@remix-run/node'; + import { useLoaderData, useRevalidator } from '@remix-run/react'; + let isFirstCall = true; + export async function loader({ serverLoader }) { + if (isFirstCall) { + isFirstCall = false + return json({ + message: "Child Server Loader Data (1)", + }); + } + return json({ + message: "Child Server Loader Data (2+)", + }); + } + export async function clientLoader({ serverLoader }) { + await new Promise(r => setTimeout(r, 100)); + let serverData = await serverLoader(); + return { + message: serverData.message + " (mutated by client)", + }; + } + clientLoader.hydrate=true; + export default function Component() { + let data = useLoaderData(); + let revalidator = useRevalidator(); + return ( + <> +

{data.message}

+ + + ); + } + export function HydrateFallback() { + return

Loading...

+ } + `, + }, + }) + ); + let app = new PlaywrightFixture(appFixture, page); + + await app.goto("/parent/child"); + await page.waitForSelector("#child-data"); + let html = await app.getHtml(); + expect(html).toMatch("Child Server Loader Data (1) (mutated by client)"); + app.clickElement("button"); + await page.waitForSelector(':has-text("Child Server Loader Data (2+)")'); + html = await app.getHtml("main"); + expect(html).toMatch("Child Server Loader Data (2+) (mutated by client)"); + }); + + test("initial hydration data check functions properly even if serverLoader isn't called on hydration", async ({ + page, + }) => { + appFixture = await createAppFixture( + await createFixture({ + files: { + ...getFiles({ + parentClientLoader: false, + parentClientLoaderHydrate: false, + childClientLoader: false, + childClientLoaderHydrate: false, + }), + "app/routes/parent.child.tsx": js` + import * as React from 'react'; + import { json } from '@remix-run/node'; + import { useLoaderData, useRevalidator } from '@remix-run/react'; + let isFirstCall = true; + export async function loader({ serverLoader }) { + if (isFirstCall) { + isFirstCall = false + return json({ + message: "Child Server Loader Data (1)", + }); + } + return json({ + message: "Child Server Loader Data (2+)", + }); + } + let isFirstClientCall = true; + export async function clientLoader({ serverLoader }) { + await new Promise(r => setTimeout(r, 100)); + if (isFirstClientCall) { + isFirstClientCall = false; + // First time through - don't even call serverLoader + return { + message: "Child Client Loader Data", + }; + } + // Only call the serverLoader on subsequent calls and this + // should *not* return us the initialData any longer + let serverData = await serverLoader(); + return { + message: serverData.message + " (mutated by client)", + }; + } + clientLoader.hydrate=true; + export default function Component() { + let data = useLoaderData(); + let revalidator = useRevalidator(); + return ( + <> +

{data.message}

+ + + ); + } + export function HydrateFallback() { + return

Loading...

+ } + `, + }, + }) + ); + let app = new PlaywrightFixture(appFixture, page); + + await app.goto("/parent/child"); + await page.waitForSelector("#child-data"); + let html = await app.getHtml(); + expect(html).toMatch("Child Client Loader Data"); + app.clickElement("button"); + await page.waitForSelector(':has-text("Child Server Loader Data (2+)")'); + html = await app.getHtml("main"); + expect(html).toMatch("Child Server Loader Data (2+) (mutated by client)"); + }); }); test.describe("clientLoader - lazy route module", () => { @@ -632,6 +811,50 @@ test.describe("Client Data", () => { expect(html).toMatch("Parent Server Loader (mutated by client)"); expect(html).toMatch("Child Server Loader (mutated by client"); }); + + test("throws a 400 if you call serverLoader without a server loader", async ({ + page, + }) => { + appFixture = await createAppFixture( + await createFixture({ + files: { + ...getFiles({ + parentClientLoader: false, + parentClientLoaderHydrate: false, + childClientLoader: false, + childClientLoaderHydrate: false, + }), + "app/routes/parent.child.tsx": js` + import * as React from 'react'; + import { useLoaderData, useRouteError } from '@remix-run/react'; + export async function clientLoader({ serverLoader }) { + return await serverLoader(); + } + export default function Component() { + return

Child

; + } + export function HydrateFallback() { + return

Loading...

; + } + export function ErrorBoundary() { + let error = useRouteError(); + return

{error.status} {error.data}

; + } + `, + }, + }) + ); + let app = new PlaywrightFixture(appFixture, page); + + await app.goto("/"); + await app.clickLink("/parent/child"); + await page.waitForSelector("#child-error"); + let html = await app.getHtml("#child-error"); + expect(html.replace(/\n/g, " ").replace(/ +/g, " ")).toMatch( + "400 Error: You are trying to call serverLoader() on a route that does " + + 'not have a server loader (routeId: "routes/parent.child")' + ); + }); }); test.describe("clientAction - critical route module", () => { @@ -796,6 +1019,51 @@ test.describe("Client Data", () => { expect(html).toMatch("Child Server Loader (mutated by client)"); expect(html).toMatch("Child Server Action (mutated by client)"); }); + + test("throws a 400 if you call serverAction without a server action", async ({ + page, + }) => { + appFixture = await createAppFixture( + await createFixture({ + files: { + ...getFiles({ + parentClientLoader: false, + parentClientLoaderHydrate: false, + childClientLoader: false, + childClientLoaderHydrate: false, + }), + "app/routes/parent.child.tsx": js` + import * as React from 'react'; + import { json } from '@remix-run/node'; + import { Form, useRouteError } from '@remix-run/react'; + export async function clientAction({ serverAction }) { + return await serverAction(); + } + export default function Component() { + return ( +
+ +
+ ); + } + export function ErrorBoundary() { + let error = useRouteError(); + return

{error.status} {error.data}

; + } + `, + }, + }) + ); + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/parent/child"); + app.clickSubmitButton("/parent/child"); + await page.waitForSelector("#child-error"); + let html = await app.getHtml("#child-error"); + expect(html.replace(/\n/g, " ").replace(/ +/g, " ")).toMatch( + "400 Error: You are trying to call serverAction() on a route that does " + + 'not have a server action (routeId: "routes/parent.child")' + ); + }); }); test.describe("clientAction - lazy route module", () => { @@ -968,5 +1236,52 @@ test.describe("Client Data", () => { expect(html).toMatch("Child Server Loader (mutated by client)"); expect(html).toMatch("Child Server Action (mutated by client)"); }); + + test("throws a 400 if you call serverAction without a server action", async ({ + page, + }) => { + appFixture = await createAppFixture( + await createFixture({ + files: { + ...getFiles({ + parentClientLoader: false, + parentClientLoaderHydrate: false, + childClientLoader: false, + childClientLoaderHydrate: false, + }), + "app/routes/parent.child.tsx": js` + import * as React from 'react'; + import { json } from '@remix-run/node'; + import { Form, useRouteError } from '@remix-run/react'; + export async function clientAction({ serverAction }) { + return await serverAction(); + } + export default function Component() { + return ( +
+ +
+ ); + } + export function ErrorBoundary() { + let error = useRouteError(); + return

{error.status} {error.data}

; + } + `, + }, + }) + ); + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.goto("/parent/child"); + await page.waitForSelector("form"); + app.clickSubmitButton("/parent/child"); + await page.waitForSelector("#child-error"); + let html = await app.getHtml("#child-error"); + expect(html.replace(/\n/g, " ").replace(/ +/g, " ")).toMatch( + "400 Error: You are trying to call serverAction() on a route that does " + + 'not have a server action (routeId: "routes/parent.child")' + ); + }); }); }); diff --git a/packages/remix-react/routes.tsx b/packages/remix-react/routes.tsx index edcb2824e4a..fef11b8fc5c 100644 --- a/packages/remix-react/routes.tsx +++ b/packages/remix-react/routes.tsx @@ -135,6 +135,15 @@ export function createClientRoutesWithHMRRevalidationOptOut( ); } +function getNoServerHandlerError(type: "action" | "loader", routeId: string) { + let fn = type === "action" ? "serverAction()" : "serverLoader()"; + let msg = + `You are trying to call ${fn} on a route that does not have a server ` + + `${type} (routeId: "${routeId}")`; + console.error(msg); + throw new ErrorResponse(400, "Bad Request", new Error(msg), true); +} + export function createClientRoutes( manifest: RouteManifest, routeModulesCache: RouteModules, @@ -226,41 +235,42 @@ export function createClientRoutes( let isHydrationRequest = needsRevalidation == null && routeModule.clientLoader != null && - routeModule.clientLoader.hydrate === true; - - dataRoute.loader = ({ request, params }: LoaderFunctionArgs) => { - return prefetchStylesAndCallHandler(async () => { - if (!routeModule.clientLoader) { - // Call the server when no client loader exists - return fetchServerLoader(request); - } - - return routeModule.clientLoader({ - request, - params, - async serverLoader() { - if (isHydrationRequest) { - isHydrationRequest = false; - - // Throw an error if a clientLoader tries to call a serverLoader that doesn't exist - if (initialData === undefined) { - throw new Error( - `You are trying to call serverLoader() on a route that does " + - "not have a server loader (routeId: "${route.id}")` - ); + (routeModule.clientLoader.hydrate === true || !route.hasLoader); + + dataRoute.loader = async ({ request, params }: LoaderFunctionArgs) => { + try { + let result = await prefetchStylesAndCallHandler(async () => { + if (!routeModule.clientLoader) { + // Call the server when no client loader exists + return fetchServerLoader(request); + } + + return routeModule.clientLoader({ + request, + params, + async serverLoader() { + if (!route.hasLoader) { + throw getNoServerHandlerError("loader", route.id); } - // Otherwise, resolve the hydration clientLoader with the pre-loaded server data - return initialData; - } + // On the first call, resolve with the pre-loaded server data + if (isHydrationRequest) { + return initialData; + } - // Call the server loader for client-side navigations - let result = await fetchServerLoader(request); - let unwrapped = await unwrapServerResponse(result); - return unwrapped; - }, + // Call the server loader for client-side navigations + let result = await fetchServerLoader(request); + let unwrapped = await unwrapServerResponse(result); + return unwrapped; + }, + }); }); - }); + return result; + } finally { + // Whether or not the user calls `serverLoader`, we only let this + // stick around as true for one loader call + isHydrationRequest = false; + } }; // Let React Router know whether to run this on hydration @@ -276,6 +286,9 @@ export function createClientRoutes( request, params, async serverAction() { + if (!route.hasAction) { + throw getNoServerHandlerError("action", route.id); + } let result = await fetchServerAction(request); let unwrapped = await unwrapServerResponse(result); return unwrapped; @@ -310,6 +323,9 @@ export function createClientRoutes( clientLoader({ ...args, async serverLoader() { + if (!route.hasLoader) { + throw getNoServerHandlerError("loader", route.id); + } let response = await fetchServerLoader(args.request); let result = await unwrapServerResponse(response); return result; @@ -323,6 +339,9 @@ export function createClientRoutes( clientAction({ ...args, async serverAction() { + if (!route.hasAction) { + throw getNoServerHandlerError("action", route.id); + } let response = await fetchServerAction(args.request); let result = await unwrapServerResponse(response); return result;