Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Properly handle in-flight network errors on _data requests #6783

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Properly handle in-flight network errors on _data requests #6783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b89efb4
Properly handle network erorrs on _data requests
brophdawg11 Jul 6, 2023
4321b36
Add unit tests for header mutual exclusivity
brophdawg11 Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/handle-network-errors.md
View file
Open in desktop
@@ -0,0 +1,6 @@
---
"@remix-run/react": patch
"@remix-run/server-runtime": patch
---

Properly handle `?_data` HTTP/Network errors that don't reach the Remix server and ensure they bubble to the `ErrorBoundary`
37 changes: 26 additions & 11 deletions integration/error-boundary-v2-test.ts
View file
Open in desktop
Expand Up @@ -164,20 +164,24 @@ test.describe("V2 Singular ErrorBoundary (future.v2_errorBoundary)", () => {
test.describe("with JavaScript", () => {
test.use({ javaScriptEnabled: true });
runBoundaryTests();

test("Network errors that never reach the Remix server", async ({
page,
}) => {
// Cause a ?_data request to trigger an HTTP error that never reaches the
// Remix server, and ensure we properly handle it at the ErrorBoundary
await page.route(
"**/parent/child-with-boundary?_data=routes%2Fparent.child-with-boundary",
(route) => route.fulfill({ status: 500, body: "CDN Error!" })
);
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent");
await app.clickLink("/parent/child-with-boundary");
await waitForAndAssert(page, app, "#child-error", "CDN Error!");
});
});

function runBoundaryTests() {
// Shorthand util to wait for an element to appear before asserting it
async function waitForAndAssert(
page: Page,
app: PlaywrightFixture,
selector: string,
match: string
) {
await page.waitForSelector(selector);
expect(await app.getHtml(selector)).toMatch(match);
}

test("No errors", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent");
Expand Down Expand Up @@ -238,3 +242,14 @@ test.describe("V2 Singular ErrorBoundary (future.v2_errorBoundary)", () => {
});
}
});

// Shorthand util to wait for an element to appear before asserting it
async function waitForAndAssert(
page: Page,
app: PlaywrightFixture,
selector: string,
match: string
) {
await page.waitForSelector(selector);
expect(await app.getHtml(selector)).toMatch(match);
}
36 changes: 36 additions & 0 deletions packages/remix-react/data.ts
View file
Open in desktop
Expand Up @@ -18,6 +18,25 @@ export function isErrorResponse(response: any): response is Response {
return response.headers.get("X-Remix-Error") != null;
}

export function isNetworkErrorResponse(response: any): response is Response {
// If we reach the Remix server, we can safely identify response types via the
// X-Remix-Error/X-Remix-Catch headers. However, if we never reach the Remix
// server, and instead receive a 4xx/5xx from somewhere in between (like
// Cloudflare), then we get a false negative n the isErrorResponse check and
// we incorrectly assume that the user returns the 4xx/5xx response and
// consider it successful. To alleviate this, we add X-Remix-Response to any
// non-Error/non-Catch responses coming back from the server. If we don't
// see this, we can conclude that a 4xx/5xx response never actually reached
// the Remix server and we can bubble it up as an error.
return (
isResponse(response) &&
response.status >= 400 &&
response.headers.get("X-Remix-Error") == null &&
response.headers.get("X-Remix-Catch") == null &&
response.headers.get("X-Remix-Response") == null
);
}

export function isRedirectResponse(response: Response): boolean {
return response.headers.get("X-Remix-Redirect") != null;
}
Expand All @@ -26,6 +45,16 @@ export function isDeferredResponse(response: Response): boolean {
return !!response.headers.get("Content-Type")?.match(/text\/remix-deferred/);
}

function isResponse(value: any): value is Response {
return (
value != null &&
typeof value.status === "number" &&
typeof value.statusText === "string" &&
typeof value.headers === "object" &&
typeof value.body !== "undefined"
);
}

export async function fetchData(
request: Request,
routeId: string,
Expand Down Expand Up @@ -85,6 +114,13 @@ export async function fetchData(
return error;
}

if (isNetworkErrorResponse(response)) {
let text = await response.text();
let error = new Error(text);
error.stack = undefined;
return error;
}

return response;
}

Expand Down
174 changes: 172 additions & 2 deletions packages/remix-server-runtime/__tests__/data-test.ts
View file
Open in desktop
@@ -1,4 +1,5 @@
import type { ServerBuild } from "../build";
import { defer } from "../responses";
import { createRequestHandler } from "../server";

describe("loaders", () => {
Expand Down Expand Up @@ -39,7 +40,133 @@ describe("loaders", () => {
expect(await res.json()).toMatchInlineSnapshot(`"?foo=bar"`);
});

it("sets header for throw responses", async () => {
it("sets X-Remix-Response header for returned 2xx response", async () => {
let routeId = "routes/random";
let build = {
routes: {
[routeId]: {
id: routeId,
path: "/random",
module: {
async loader() {
return new Response("text", {
status: 200,
headers: { "Content-Type": "text/plain" },
});
},
},
},
},
entry: {
module: {
handleError() {},
},
},
future: {},
} as unknown as ServerBuild;

let handler = createRequestHandler(build);

let request = new Request(
"http://example.com/random?_data=routes/random&foo=bar",
{
headers: {
"Content-Type": "application/json",
},
}
);

let res = await handler(request);
expect(res.headers.get("X-Remix-Response")).toBeTruthy();
expect(res.headers.get("X-Remix-Error")).toBeNull();
expect(res.headers.get("X-Remix-Catch")).toBeNull();
expect(res.headers.get("X-Remix-Redirect")).toBeNull();
});

it("sets X-Remix-Response header for returned 2xx defer response", async () => {
let routeId = "routes/random";
let build = {
routes: {
[routeId]: {
id: routeId,
path: "/random",
module: {
async loader() {
return defer({ lazy: Promise.resolve("hey!") });
},
},
},
},
entry: {
module: {
handleError() {},
},
},
future: {},
} as unknown as ServerBuild;

let handler = createRequestHandler(build);

let request = new Request(
"http://example.com/random?_data=routes/random&foo=bar",
{
headers: {
"Content-Type": "application/json",
},
}
);

let res = await handler(request);
expect(res.headers.get("X-Remix-Response")).toBeTruthy();
expect(res.headers.get("X-Remix-Error")).toBeNull();
expect(res.headers.get("X-Remix-Catch")).toBeNull();
expect(res.headers.get("X-Remix-Redirect")).toBeNull();
});

it("sets X-Remix-Redirect header for returned 3xx redirect", async () => {
let routeId = "routes/random";
let build = {
routes: {
[routeId]: {
id: routeId,
path: "/random",
module: {
async loader() {
return new Response("text", {
status: 302,
headers: { Location: "https://remix.run" },
});
},
},
},
},
entry: {
module: {
handleError() {},
},
},
future: {},
} as unknown as ServerBuild;

let handler = createRequestHandler(build);

let request = new Request(
"http://example.com/random?_data=routes/random&foo=bar",
{
headers: {
"Content-Type": "application/json",
},
}
);

let res = await handler(request);
expect(res.headers.get("X-Remix-Redirect")).toBeTruthy();
expect(res.headers.get("X-Remix-Error")).toBeNull();
expect(res.headers.get("X-Remix-Catch")).toBeNull();
expect(res.headers.get("X-Remix-Response")).toBeNull();
});

it("sets X-Remix-Catch header for throw responses", async () => {
let loader = async ({ request }) => {
throw new Response("null", {
headers: {
Expand Down Expand Up @@ -75,7 +202,50 @@ describe("loaders", () => {
);

let res = await handler(request);
expect(await res.headers.get("X-Remix-Catch")).toBeTruthy();
expect(res.headers.get("X-Remix-Catch")).toBeTruthy();
expect(res.headers.get("X-Remix-Error")).toBeNull();
expect(res.headers.get("X-Remix-Redirect")).toBeNull();
expect(res.headers.get("X-Remix-Response")).toBeNull();
});

it("sets X-Remix-Error header for throw error", async () => {
let routeId = "routes/random";
let build = {
routes: {
[routeId]: {
id: routeId,
path: "/random",
module: {
async loader() {
throw new Error("broke!");
},
},
},
},
entry: {
module: {
handleError() {},
},
},
future: {},
} as unknown as ServerBuild;

let handler = createRequestHandler(build);

let request = new Request(
"http://example.com/random?_data=routes/random&foo=bar",
{
headers: {
"Content-Type": "application/json",
},
}
);

let res = await handler(request);
expect(res.headers.get("X-Remix-Error")).toBeTruthy();
expect(res.headers.get("X-Remix-Catch")).toBeNull();
expect(res.headers.get("X-Remix-Redirect")).toBeNull();
expect(res.headers.get("X-Remix-Response")).toBeNull();
});

it("removes index from request.url", async () => {
Expand Down
6 changes: 6 additions & 0 deletions packages/remix-server-runtime/server.ts
View file
Open in desktop
Expand Up @@ -166,10 +166,16 @@ async function handleDataRequestRR(
let init = deferredData.init || {};
let headers = new Headers(init.headers);
headers.set("Content-Type", "text/remix-deferred");
// Mark successful responses with a header so we can identify in-flight
// network errors that are missing this header
headers.set("X-Remix-Response", "yes");
init.headers = headers;
return new Response(body, init);
}

// Mark all successful responses with a header so we can identify in-flight
// network errors that are missing this header
response.headers.set("X-Remix-Response", "yes");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having to remember to do this in each success code path,. should we instead do this higher up? I kept it here to colocate it with the setting of the error/catch headers but it feels potentially error-prone in the future:

function requestHandler() {
  ...
  if (url.searchParams.has("_data")) {
    response = await handleDataRequestRR(...);

    if (!response.headers.has('X-Remix-Error') && !response.headers.has('X-Remix-Catch')) {
      response.headers.set('X-Remix-Response', 'yes');
    }
    ...
  }
  ...
}

return response;
} catch (error: unknown) {
if (isResponse(error)) {
Expand Down