diff --git a/integration/catch-boundary-data-test.ts b/integration/catch-boundary-data-test.ts new file mode 100644 index 00000000000..6514619e719 --- /dev/null +++ b/integration/catch-boundary-data-test.ts @@ -0,0 +1,193 @@ +import { createAppFixture, createFixture, js } from "./helpers/create-fixture"; +import type { Fixture, AppFixture } from "./helpers/create-fixture"; + +let fixture: Fixture; +let app: AppFixture; + +let ROOT_BOUNDARY_TEXT = "ROOT_TEXT"; +let LAYOUT_BOUNDARY_TEXT = "LAYOUT_BOUNDARY_TEXT"; +let OWN_BOUNDARY_TEXT = "OWN_BOUNDARY_TEXT"; + +let NO_BOUNDARY_LOADER = "/no/loader"; +let HAS_BOUNDARY_LAYOUT_NESTED_LOADER = "/yes/loader-layout-boundary"; +let HAS_BOUNDARY_NESTED_LOADER = "/yes/loader-self-boundary"; + +let ROOT_DATA = "root data"; +let LAYOUT_DATA = "root data"; + +beforeAll(async () => { + fixture = await createFixture({ + files: { + "app/root.jsx": js` + import { Outlet, Scripts, useMatches, useLoaderData } from "remix"; + export let loader = () => "${ROOT_DATA}"; + export default function Root() { + let data = useLoaderData(); + return ( + + + +
{data}
+ + + + + ); + } + export function CatchBoundary() { + let matches = useMatches(); + let { data } = matches.find(match => match.id === "root"); + + return ( + + + +
${ROOT_BOUNDARY_TEXT}
+
{data}
+ + + + ); + } + `, + + "app/routes/index.jsx": js` + import { Link } from "remix"; + export default function Index() { + return ( +
+ ${NO_BOUNDARY_LOADER} + ${HAS_BOUNDARY_LAYOUT_NESTED_LOADER} + ${HAS_BOUNDARY_NESTED_LOADER} +
+ ); + } + `, + + [`app/routes${NO_BOUNDARY_LOADER}.jsx`]: js` + export function loader() { + throw new Response("", { status: 401 }); + } + export default function Index() { + return
; + } + `, + + [`app/routes${HAS_BOUNDARY_LAYOUT_NESTED_LOADER}.jsx`]: js` + import { useMatches } from "remix"; + export function loader() { + return "${LAYOUT_DATA}"; + } + export default function Layout() { + return
; + } + export function CatchBoundary() { + let matches = useMatches(); + let { data } = matches.find(match => match.id === "routes${HAS_BOUNDARY_LAYOUT_NESTED_LOADER}"); + + return ( +
+
${LAYOUT_BOUNDARY_TEXT}
+
{data}
+
+ ); + } + `, + + [`app/routes${HAS_BOUNDARY_LAYOUT_NESTED_LOADER}/index.jsx`]: js` + export function loader() { + throw new Response("", { status: 401 }); + } + export default function Index() { + return
; + } + `, + + [`app/routes${HAS_BOUNDARY_NESTED_LOADER}.jsx`]: js` + import { Outlet, useLoaderData } from "remix"; + export function loader() { + return "${LAYOUT_DATA}"; + } + export default function Layout() { + let data = useLoaderData(); + return ( +
+
{data}
+ +
+ ); + } + `, + + [`app/routes${HAS_BOUNDARY_NESTED_LOADER}/index.jsx`]: js` + export function loader() { + throw new Response("", { status: 401 }); + } + export default function Index() { + return
; + } + export function CatchBoundary() { + return ( +
${OWN_BOUNDARY_TEXT}
+ ); + } + `, + }, + }); + + app = await createAppFixture(fixture); +}); + +afterAll(async () => app.close()); + +it("renders root boundary with data avaliable", async () => { + let res = await fixture.requestDocument(NO_BOUNDARY_LOADER); + expect(res.status).toBe(401); + let html = await res.text(); + expect(html).toMatch(ROOT_BOUNDARY_TEXT); + expect(html).toMatch(ROOT_DATA); +}); + +it("renders root boundary with data avaliable on transition", async () => { + await app.goto("/"); + await app.clickLink(NO_BOUNDARY_LOADER); + let html = await app.getHtml(); + expect(html).toMatch(ROOT_BOUNDARY_TEXT); + expect(html).toMatch(ROOT_DATA); +}); + +it("renders layout boundary with data avaliable", async () => { + let res = await fixture.requestDocument(HAS_BOUNDARY_LAYOUT_NESTED_LOADER); + expect(res.status).toBe(401); + let html = await res.text(); + expect(html).toMatch(ROOT_DATA); + expect(html).toMatch(LAYOUT_BOUNDARY_TEXT); + expect(html).toMatch(LAYOUT_DATA); +}); + +it("renders layout boundary with data avaliable on transition", async () => { + await app.goto("/"); + await app.clickLink(HAS_BOUNDARY_LAYOUT_NESTED_LOADER); + let html = await app.getHtml(); + expect(html).toMatch(ROOT_DATA); + expect(html).toMatch(LAYOUT_BOUNDARY_TEXT); + expect(html).toMatch(LAYOUT_DATA); +}); + +it("renders self boundary with layout data avaliable", async () => { + let res = await fixture.requestDocument(HAS_BOUNDARY_NESTED_LOADER); + expect(res.status).toBe(401); + let html = await res.text(); + expect(html).toMatch(ROOT_DATA); + expect(html).toMatch(LAYOUT_DATA); + expect(html).toMatch(OWN_BOUNDARY_TEXT); +}); + +it("renders self boundary with layout data avaliable on transition", async () => { + await app.goto("/"); + await app.clickLink(HAS_BOUNDARY_NESTED_LOADER); + let html = await app.getHtml(); + expect(html).toMatch(ROOT_DATA); + expect(html).toMatch(LAYOUT_DATA); + expect(html).toMatch(OWN_BOUNDARY_TEXT); +}); diff --git a/integration/catch-boundary-test.ts b/integration/catch-boundary-test.ts index 1d8663152fb..967850f3599 100644 --- a/integration/catch-boundary-test.ts +++ b/integration/catch-boundary-test.ts @@ -10,8 +10,10 @@ describe("CatchBoundary", () => { let HAS_BOUNDARY_LOADER = "/yes/loader"; let HAS_BOUNDARY_ACTION = "/yes/action"; + let HAS_BOUNDARY_NO_LOADER_OR_ACTION = "/yes/no-loader-or-action"; let NO_BOUNDARY_ACTION = "/no/action"; let NO_BOUNDARY_LOADER = "/no/loader"; + let NO_BOUNDARY_NO_LOADER_OR_ACTION = "/no/no-loader-or-action"; let NOT_FOUND_HREF = "/not/found"; @@ -54,6 +56,8 @@ describe("CatchBoundary", () => {
+ ) + } + `, + + "app/routes/fetcher-no-boundary.jsx": js` + import { useFetcher } from "remix"; + export default function() { + let fetcher = useFetcher(); + + return ( +
+ +
+ ) + } + `, + [`app/routes${HAS_BOUNDARY_ACTION}.jsx`]: js` import { Form } from "remix"; export async function action() { @@ -102,6 +139,21 @@ describe("CatchBoundary", () => { } `, + [`app/routes${HAS_BOUNDARY_NO_LOADER_OR_ACTION}.jsx`]: js` + export function CatchBoundary() { + return
${OWN_BOUNDARY_TEXT}
+ } + export default function Index() { + return
+ } + `, + + [`app/routes${NO_BOUNDARY_NO_LOADER_OR_ACTION}.jsx`]: js` + export default function Index() { + return
+ } + `, + [`app/routes${HAS_BOUNDARY_LOADER}.jsx`]: js` export function loader() { throw new Response("", { status: 401 }) @@ -159,8 +211,7 @@ describe("CatchBoundary", () => { expect(await res.text()).toMatch(OWN_BOUNDARY_TEXT); }); - // FIXME: this is broken, the request returns but the page doesn't update - test.skip("own boundary, action, client transition from other route", async () => { + test("own boundary, action, client transition from other route", async () => { await app.goto("/"); await app.clickSubmitButton(HAS_BOUNDARY_ACTION); expect(await app.getHtml()).toMatch(OWN_BOUNDARY_TEXT); @@ -214,4 +265,43 @@ describe("CatchBoundary", () => { await app.clickLink(NO_BOUNDARY_LOADER); expect(await app.getHtml()).toMatch(ROOT_BOUNDARY_TEXT); }); + + it("renders root boundary in document POST without action requests", async () => { + let res = await fixture.requestDocument(NO_BOUNDARY_NO_LOADER_OR_ACTION, { + method: "post", + }); + expect(res.status).toBe(405); + expect(await res.text()).toMatch(ROOT_BOUNDARY_TEXT); + }); + + it("renders root boundary in action script transitions without action from other routes", async () => { + await app.goto("/"); + await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION); + expect(await app.getHtml()).toMatch(ROOT_BOUNDARY_TEXT); + }); + + it("renders own boundary in document POST without action requests", async () => { + let res = await fixture.requestDocument(HAS_BOUNDARY_NO_LOADER_OR_ACTION, { + method: "post", + }); + expect(res.status).toBe(405); + expect(await res.text()).toMatch(OWN_BOUNDARY_TEXT); + }); + + it("renders own boundary in action script transitions without action from other routes", async () => { + await app.goto("/"); + await app.clickSubmitButton(HAS_BOUNDARY_NO_LOADER_OR_ACTION); + expect(await app.getHtml()).toMatch(OWN_BOUNDARY_TEXT); + }); + + it("renders own boundary in fetcher action submission without action from other routes", async () => { + await app.goto("/fetcher-boundary"); + await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION); + expect(await app.getHtml()).toMatch(OWN_BOUNDARY_TEXT); + }); + it("renders root boundary in fetcher action submission without action from other routes", async () => { + await app.goto("/fetcher-no-boundary"); + await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION); + expect(await app.getHtml()).toMatch(ROOT_BOUNDARY_TEXT); + }); }); diff --git a/packages/remix-react/routes.tsx b/packages/remix-react/routes.tsx index 9f8b142cc46..eb6e61a8bef 100644 --- a/packages/remix-react/routes.tsx +++ b/packages/remix-react/routes.tsx @@ -65,7 +65,7 @@ export type RouteDataFunction = { export interface ClientRoute extends Route { loader?: RouteDataFunction; - action?: RouteDataFunction; + action: RouteDataFunction; shouldReload?: ShouldReloadFunction; ErrorBoundary?: any; CatchBoundary?: any; @@ -90,7 +90,7 @@ export function createClientRoute( index: entryRoute.index, module: entryRoute.module, loader: createLoader(entryRoute, routeModulesCache), - action: createAction(entryRoute), + action: createAction(entryRoute, routeModulesCache), shouldReload: createShouldReload(entryRoute, routeModulesCache), ErrorBoundary: entryRoute.hasErrorBoundary, CatchBoundary: entryRoute.hasCatchBoundary, @@ -175,10 +175,15 @@ function createLoader(route: EntryRoute, routeModules: RouteModules) { return loader; } -function createAction(route: EntryRoute) { - if (!route.hasAction) return undefined; - +function createAction(route: EntryRoute, routeModules: RouteModules) { let action: ClientRoute["action"] = async ({ url, signal, submission }) => { + if (!route.hasAction) { + console.error( + `Route "${route.id}" does not have an action, but you are trying ` + + `to submit to it. To fix this, please add an \`action\` function to the route` + ); + } + let result = await fetchData(url, route.id, signal, submission); if (result instanceof Error) { @@ -188,6 +193,8 @@ function createAction(route: EntryRoute) { let redirect = await checkRedirect(result); if (redirect) return redirect; + await loadRouteModuleWithBlockingLinks(route, routeModules); + if (isCatchResponse(result)) { throw new CatchValue( result.status, diff --git a/packages/remix-react/transition.ts b/packages/remix-react/transition.ts index af850879440..c2ead81538f 100644 --- a/packages/remix-react/transition.ts +++ b/packages/remix-react/transition.ts @@ -736,11 +736,12 @@ export function createTransitionManager(init: TransitionManagerInit) { maybeActionErrorResult ); - let [catchVal, catchBoundaryId] = await findCatchAndBoundaryId( - results, - state.matches, - maybeActionCatchResult - ); + let [catchVal, catchBoundaryId] = + (await findCatchAndBoundaryId( + results, + state.matches, + maybeActionCatchResult + )) || []; let doneFetcher: FetcherStates["Done"] = { state: "idle", @@ -951,6 +952,7 @@ export function createTransitionManager(init: TransitionManagerInit) { key: string, result: DataResult ) { + // TODO: revisit this if submission is correct after review if (isCatchResult(result)) { let catchBoundaryId = findNearestCatchBoundary(match, state.matches); state.fetchers.delete(key); @@ -1062,18 +1064,10 @@ export function createTransitionManager(init: TransitionManagerInit) { return; } + let catchVal, catchBoundaryId; if (isCatchResult(result)) { - let [catchVal, catchBoundaryId] = await findCatchAndBoundaryId( - [result], - matches, - result - ); - update({ - transition: IDLE_TRANSITION, - catch: catchVal, - catchBoundaryId, - }); - return; + [catchVal, catchBoundaryId] = + (await findCatchAndBoundaryId([result], matches, result)) || []; } let loadTransition: TransitionStates["LoadingAction"] = { @@ -1093,7 +1087,9 @@ export function createTransitionManager(init: TransitionManagerInit) { matches, submission, leafMatch.route.id, - result + result, + catchVal, + catchBoundaryId ); } @@ -1230,7 +1226,9 @@ export function createTransitionManager(init: TransitionManagerInit) { matches: ClientMatch[], submission?: Submission, submissionRouteId?: string, - actionResult?: DataResult + actionResult?: DataResult, + catchVal?: CatchData, + catchBoundaryId?: string | null ) { let maybeActionErrorResult = actionResult && isErrorResult(actionResult) ? actionResult : undefined; @@ -1251,7 +1249,9 @@ export function createTransitionManager(init: TransitionManagerInit) { maybeActionErrorResult, maybeActionCatchResult, submission, - submissionRouteId + submissionRouteId, + undefined, + catchBoundaryId ); if (controller.signal.aborted) { @@ -1295,11 +1295,11 @@ export function createTransitionManager(init: TransitionManagerInit) { maybeActionErrorResult ); - let [catchVal, catchBoundaryId] = await findCatchAndBoundaryId( + [catchVal, catchBoundaryId] = (await findCatchAndBoundaryId( results, matches, maybeActionErrorResult - ); + )) || [catchVal, catchBoundaryId]; markFetchRedirectsDone(); @@ -1392,7 +1392,8 @@ async function callLoaders( actionCatchResult?: DataCatchResult, submission?: Submission, submissionRouteId?: string, - fetcher?: Fetcher + fetcher?: Fetcher, + catchBoundaryId?: string | null ): Promise { let matchesToLoad = filterMatchesToLoad( state, @@ -1402,7 +1403,8 @@ async function callLoaders( actionCatchResult, submission, submissionRouteId, - fetcher + fetcher, + catchBoundaryId ); return Promise.all( @@ -1426,13 +1428,6 @@ async function callAction( match: ClientMatch, signal: AbortSignal ): Promise { - if (!match.route.action) { - throw new Error( - `Route "${match.route.id}" does not have an action, but you are trying ` + - `to submit to it. To fix this, please add an \`action\` function to the route` - ); - } - try { let value = await match.route.action({ url: createUrl(submission.action), @@ -1454,17 +1449,24 @@ function filterMatchesToLoad( actionCatchResult?: DataCatchResult, submission?: Submission, submissionRouteId?: string, - fetcher?: Fetcher + fetcher?: Fetcher, + catchBoundaryId?: string | null ): ClientMatch[] { // Filter out all routes below the problematic route as they aren't going // to render so we don't need to load them. - if (submissionRouteId && (actionCatchResult || actionErrorResult)) { + if ( + catchBoundaryId || + (submissionRouteId && (actionCatchResult || actionErrorResult)) + ) { let foundProblematicRoute = false; matches = matches.filter((match) => { if (foundProblematicRoute) { return false; } - if (match.route.id === submissionRouteId) { + if ( + match.route.id === submissionRouteId || + match.route.id === catchBoundaryId + ) { foundProblematicRoute = true; return false; } @@ -1566,7 +1568,7 @@ async function findCatchAndBoundaryId( results: DataResult[], matches: ClientMatch[], actionCatchResult?: DataCatchResult -): Promise<[CatchData, string | null] | [undefined, undefined]> { +): Promise<[CatchData, string | null] | null> { let loaderCatchResult: DataCatchResult | undefined; for (let result of results) { @@ -1595,7 +1597,7 @@ async function findCatchAndBoundaryId( return [await extractCatchData(loaderCatchResult.value), boundaryId]; } - return [undefined, undefined]; + return null; } function findErrorAndBoundaryId( diff --git a/packages/remix-server-runtime/__tests__/server-test.ts b/packages/remix-server-runtime/__tests__/server-test.ts index c894f7075e8..021f2cf2fe4 100644 --- a/packages/remix-server-runtime/__tests__/server-test.ts +++ b/packages/remix-server-runtime/__tests__/server-test.ts @@ -518,28 +518,6 @@ describe("shared server runtime", () => { expect(testAction.mock.calls.length).toBe(0); }); - test("data request that does not match action surfaces error for boundary", async () => { - let build = mockServerBuild({ - root: { - default: {}, - }, - "routes/index": { - parentId: "root", - index: true, - }, - }); - let handler = createRequestHandler(build, {}, ServerMode.Test); - - let request = new Request(`${baseUrl}/?index&_data=routes/index`, { - method: "post", - }); - - let result = await handler(request); - expect(result.status).toBe(500); - expect(result.headers.get("X-Remix-Error")).toBe("yes"); - expect((await result.json()).message).toBeTruthy(); - }); - test("data request calls action", async () => { let rootLoader = jest.fn(() => { return "root"; diff --git a/packages/remix-server-runtime/data.ts b/packages/remix-server-runtime/data.ts index 60df3fef7c0..674f3dbc1f3 100644 --- a/packages/remix-server-runtime/data.ts +++ b/packages/remix-server-runtime/data.ts @@ -25,11 +25,9 @@ export async function callRouteAction({ let action = match.route.module.action; if (!action) { - throw new Error( - `You made a ${request.method} request to ${request.url} but did not provide ` + - `an \`action\` for route "${match.route.id}", so there is no way to handle the ` + - `request.` - ); + let response = new Response(null, { status: 405 }); + response.headers.set("X-Remix-Catch", "yes"); + return response; } let result;