Skip to content

Commit

Permalink
feat: send 405 not 500 for missing action and surface in CatchBoundary (
Browse files Browse the repository at this point in the history 
remix-run#2342)

fix: load module after action submission if it's not a redirect
fix: when an action "catches", still load next data for next location to render nested boundaries and not be missing parent data
  • Loading branch information
@jacob-ebey @MichaelDeBoey
jacob-ebey authored and MichaelDeBoey committed Mar 21, 2022
1 parent 41a2a42 commit be6cf8d
Show file tree
Hide file tree
Showing 6 changed files with 337 additions and 69 deletions.
193 changes: 193 additions & 0 deletions integration/catch-boundary-data-test.ts
View file
Original file line number Diff line number Diff line change
@@ -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 (
<html>
<head />
<body>
<div>{data}</div>
<Outlet />
<Scripts />
</body>
</html>
);
}
export function CatchBoundary() {
let matches = useMatches();
let { data } = matches.find(match => match.id === "root");
return (
<html>
<head />
<body>
<div>${ROOT_BOUNDARY_TEXT}</div>
<div>{data}</div>
<Scripts />
</body>
</html>
);
}
`,

"app/routes/index.jsx": js`
import { Link } from "remix";
export default function Index() {
return (
<div>
<Link to="${NO_BOUNDARY_LOADER}">${NO_BOUNDARY_LOADER}</Link>
<Link to="${HAS_BOUNDARY_LAYOUT_NESTED_LOADER}">${HAS_BOUNDARY_LAYOUT_NESTED_LOADER}</Link>
<Link to="${HAS_BOUNDARY_NESTED_LOADER}">${HAS_BOUNDARY_NESTED_LOADER}</Link>
</div>
);
}
`,

[`app/routes${NO_BOUNDARY_LOADER}.jsx`]: js`
export function loader() {
throw new Response("", { status: 401 });
}
export default function Index() {
return <div/>;
}
`,

[`app/routes${HAS_BOUNDARY_LAYOUT_NESTED_LOADER}.jsx`]: js`
import { useMatches } from "remix";
export function loader() {
return "${LAYOUT_DATA}";
}
export default function Layout() {
return <div/>;
}
export function CatchBoundary() {
let matches = useMatches();
let { data } = matches.find(match => match.id === "routes${HAS_BOUNDARY_LAYOUT_NESTED_LOADER}");
return (
<div>
<div>${LAYOUT_BOUNDARY_TEXT}</div>
<div>{data}</div>
</div>
);
}
`,

[`app/routes${HAS_BOUNDARY_LAYOUT_NESTED_LOADER}/index.jsx`]: js`
export function loader() {
throw new Response("", { status: 401 });
}
export default function Index() {
return <div/>;
}
`,

[`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 (
<div>
<div>{data}</div>
<Outlet/>
</div>
);
}
`,

[`app/routes${HAS_BOUNDARY_NESTED_LOADER}/index.jsx`]: js`
export function loader() {
throw new Response("", { status: 401 });
}
export default function Index() {
return <div/>;
}
export function CatchBoundary() {
return (
<div>${OWN_BOUNDARY_TEXT}</div>
);
}
`,
},
});

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);
});
94 changes: 92 additions & 2 deletions integration/catch-boundary-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -54,6 +56,8 @@ describe("CatchBoundary", () => {
<Form method="post">
<button formAction="${HAS_BOUNDARY_ACTION}" type="submit" />
<button formAction="${NO_BOUNDARY_ACTION}" type="submit" />
<button formAction="${HAS_BOUNDARY_NO_LOADER_OR_ACTION}" type="submit" />
<button formAction="${NO_BOUNDARY_NO_LOADER_OR_ACTION}" type="submit" />
</Form>
<Link to="${HAS_BOUNDARY_LOADER}">
Expand All @@ -67,6 +71,39 @@ describe("CatchBoundary", () => {
}
`,

"app/routes/fetcher-boundary.jsx": js`
import { useFetcher } from "remix";
export function CatchBoundary() {
return <p>${OWN_BOUNDARY_TEXT}</p>
}
export default function() {
let fetcher = useFetcher();
return (
<div>
<fetcher.Form method="post">
<button formAction="${NO_BOUNDARY_NO_LOADER_OR_ACTION}" type="submit" />
</fetcher.Form>
</div>
)
}
`,

"app/routes/fetcher-no-boundary.jsx": js`
import { useFetcher } from "remix";
export default function() {
let fetcher = useFetcher();
return (
<div>
<fetcher.Form method="post">
<button formAction="${NO_BOUNDARY_NO_LOADER_OR_ACTION}" type="submit" />
</fetcher.Form>
</div>
)
}
`,

[`app/routes${HAS_BOUNDARY_ACTION}.jsx`]: js`
import { Form } from "remix";
export async function action() {
Expand Down Expand Up @@ -102,6 +139,21 @@ describe("CatchBoundary", () => {
}
`,

[`app/routes${HAS_BOUNDARY_NO_LOADER_OR_ACTION}.jsx`]: js`
export function CatchBoundary() {
return <div>${OWN_BOUNDARY_TEXT}</div>
}
export default function Index() {
return <div/>
}
`,

[`app/routes${NO_BOUNDARY_NO_LOADER_OR_ACTION}.jsx`]: js`
export default function Index() {
return <div/>
}
`,

[`app/routes${HAS_BOUNDARY_LOADER}.jsx`]: js`
export function loader() {
throw new Response("", { status: 401 })
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
});
});
17 changes: 12 additions & 5 deletions packages/remix-react/routes.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export type RouteDataFunction = {

export interface ClientRoute extends Route {
loader?: RouteDataFunction;
action?: RouteDataFunction;
action: RouteDataFunction;
shouldReload?: ShouldReloadFunction;
ErrorBoundary?: any;
CatchBoundary?: any;
Expand All @@ -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,
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand Down

0 comments on commit be6cf8d

Please sign in to comment.