Skip to content

Commit

Permalink
feat(remix-server-runtime): RRR 1.3 / 1.4 - handleDocumentRequest (#4385
Browse files Browse the repository at this point in the history 
)

test: update test to be more reliable
chore: be more protective when accessing route module
test: stop compiler test from logging
feat: disambiguate catch/error boundary responses
fix: ensure route modules are loaded regardless of error state
test: make fetcher tests more reliable
feat: Change missing loader from 405 -> 400 error

Co-authored-by: Matt Brophy <[email protected]>
  • Loading branch information
@jacob-ebey @brophdawg11
jacob-ebey and brophdawg11 authored Nov 15, 2022
1 parent 70784af commit 9f3bfe4
Show file tree
Hide file tree
Showing 18 changed files with 1,625 additions and 646 deletions.
26 changes: 26 additions & 0 deletions .changeset/light-avocados-drop.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
"@remix-run/server-runtime": patch
---

fix: Properly categorize internal framework-thrown error Responses as error boundary errors

Previously there was some ambiguity around _"thrown Responses go to the `CatchBoundary`"_.
The `CatchBoundary` exists to give the _user_ a place to handle non-happy path code flows
such that they can throw Response instances from _their own code_ and handle them in a
`CatchBoundary`. However, there are a handful of framework-internal errors that make
sense to have a non-500 status code, and the fact that these were being thrown as Responses
was causing them to go into the CatchBoundary, even though they were not user-thrown.

With this change, anything thrown by the framework itself (`Error` or `Response`) will
go to the `ErrorBoundary`, and any user-thrown `Response` instances will go to the
`CatchBoundary`. Thereis one exception to this rule, which is that framework-detected
404's will continue to go to the `CatchBoundary` since users should have one single
location to handle 404 displays.

The primary affected use cases are scenarios such as:

* HTTP `OPTIONS` requests (405 Unsupported Method )
* `GET` requests to routes without loaders (400 Bad Request)
* `POST` requests to routes without actions (405 Method Not Allowed)
* Missing route id in `_data` parameters (403 Forbidden)
* Non-matching route id included in `_data` parameters (403 Forbidden)
110 changes: 0 additions & 110 deletions integration/catch-boundary-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@ test.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 @@ -64,8 +62,6 @@ test.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 @@ -79,39 +75,6 @@ test.describe("CatchBoundary", () => {
}
`,

"app/routes/fetcher-boundary.jsx": js`
import { useFetcher } from "@remix-run/react";
export function CatchBoundary() {
return <p id="fetcher-boundary">${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-run/react";
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-run/react";
export async function action() {
Expand Down Expand Up @@ -147,21 +110,6 @@ test.describe("CatchBoundary", () => {
}
`,

[`app/routes${HAS_BOUNDARY_NO_LOADER_OR_ACTION}.jsx`]: js`
export function CatchBoundary() {
return <div id="boundary-no-loader-or-action">${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 @@ -250,12 +198,6 @@ test.describe("CatchBoundary", () => {
await page.waitForSelector("#root-boundary");
});

test("invalid request methods", async () => {
let res = await fixture.requestDocument("/", { method: "OPTIONS" });
expect(res.status).toBe(405);
expect(await res.text()).toMatch(ROOT_BOUNDARY_TEXT);
});

test("own boundary, action, document request", async () => {
let params = new URLSearchParams();
let res = await fixture.postDocument(HAS_BOUNDARY_ACTION, params);
Expand Down Expand Up @@ -334,58 +276,6 @@ test.describe("CatchBoundary", () => {
await page.waitForSelector("#root-boundary");
});

test("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);
});

test("renders root boundary in action script transitions without action from other routes", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");
await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION);
await page.waitForSelector("#root-boundary");
});

test("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);
});

test("renders own boundary in action script transitions without action from other routes", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");
await app.clickSubmitButton(HAS_BOUNDARY_NO_LOADER_OR_ACTION);
await page.waitForSelector("#boundary-no-loader-or-action");
});

test("renders own boundary in fetcher action submission without action from other routes", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/fetcher-boundary");
await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION);
await page.waitForSelector("#fetcher-boundary");
});

test("renders root boundary in fetcher action submission without action from other routes", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/fetcher-no-boundary");
await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION);
await page.waitForSelector("#root-boundary");
});

test("uses correct catch boundary on server action errors", async ({
page,
}) => {
Expand Down
13 changes: 13 additions & 0 deletions integration/compiler-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,19 @@ test.describe("compiler", () => {
});

test.describe("serverBareModulesPlugin", () => {
let ogConsole: typeof global.console;
test.beforeEach(() => {
ogConsole = global.console;
// @ts-ignore
global.console = {
log() {},
warn() {},
error() {},
};
});
test.afterEach(() => {
global.console = ogConsole;
});
test("warns when a module isn't installed", async () => {
let buildOutput: string;
let buildStdio = new PassThrough();
Expand Down
Loading

0 comments on commit 9f3bfe4

Please sign in to comment.