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.

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

fix(remix-express,remix-vercel): fix request.signal #3626

Merged
merged 8 commits into from
Aug 29, 2022
61 changes: 61 additions & 0 deletions integration/abort-signal-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { test, expect } from "@playwright/test";

import { PlaywrightFixture } from "./helpers/playwright-fixture";
import type { Fixture, AppFixture } from "./helpers/create-fixture";
import { createAppFixture, createFixture, js } from "./helpers/create-fixture";

let fixture: Fixture;
let appFixture: AppFixture;

test.beforeAll(async () => {
fixture = await createFixture({
files: {
"app/routes/index.jsx": js`
import { json } from "@remix-run/node";
import { useActionData, useLoaderData, Form } from "@remix-run/react";

export async function action ({ request }) {
console.log('action request.signal', request.signal)
// New event loop causes express request to close
await new Promise(r => setTimeout(r, 0));
return json({ aborted: request.signal.aborted });
}

export function loader({ request }) {
console.log('loader request.signal', request.signal)
return json({ aborted: request.signal.aborted });
}

export default function Index() {
let actionData = useActionData();
let data = useLoaderData();
return (
<div>
<p className="action">{actionData ? String(actionData.aborted) : "empty"}</p>
<p className="loader">{String(data.aborted)}</p>
<Form method="post">
<button type="submit">Submit</button>
</Form>
</div>
)
}
`,
},
});

// This creates an interactive app using puppeteer.
appFixture = await createAppFixture(fixture);
});

test.afterAll(async () => appFixture.close());

test("should not abort the request in a new event loop", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");
expect(await app.getHtml(".action")).toMatch("empty");
expect(await app.getHtml(".loader")).toMatch("false");

await app.clickElement('button[type="submit"]');
expect(await app.getHtml(".action")).toMatch("false");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prior to the fix this would be true

expect(await app.getHtml(".loader")).toMatch("false");
});
6 changes: 4 additions & 2 deletions packages/remix-express/__tests__/server-test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import express from "express";
import supertest from "supertest";
import { createRequest } from "node-mocks-http";
import { createRequest, createResponse } from "node-mocks-http";
import {
createRequestHandler as createRemixRequestHandler,
Response as NodeResponse,
Expand Down Expand Up @@ -234,8 +234,10 @@ describe("express createRemixRequest", () => {
Host: "localhost:3000",
},
});
let expressResponse = createResponse();

expect(createRemixRequest(expressRequest)).toMatchInlineSnapshot(`
expect(createRemixRequest(expressRequest, expressResponse))
.toMatchInlineSnapshot(`
NodeRequest {
"agent": undefined,
"compress": true,
Expand Down
15 changes: 8 additions & 7 deletions packages/remix-express/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export function createRequestHandler({
next: express.NextFunction
) => {
try {
let request = createRemixRequest(req);
let request = createRemixRequest(req, res);
let loadContext = getLoadContext?.(req, res);

let response = (await handleRequest(
Expand Down Expand Up @@ -89,20 +89,21 @@ export function createRemixHeaders(
return headers;
}

export function createRemixRequest(req: express.Request): NodeRequest {
export function createRemixRequest(
req: express.Request,
res: express.Response
): NodeRequest {
let origin = `${req.protocol}://${req.get("host")}`;
let url = new URL(req.url, origin);

// Abort action/loaders once we can no longer write a response
let controller = new AbortController();

req.on("close", () => {
controller.abort();
});
res.on("close", () => controller.abort());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Abort on response close instead of request close


let init: NodeRequestInit = {
method: req.method,
headers: createRemixHeaders(req.headers),
signal: controller.signal,
signal: controller.signal as AbortSignal,
MichaelDeBoey marked this conversation as resolved.
Show resolved Hide resolved
};

if (req.method !== "GET" && req.method !== "HEAD") {
Expand Down
7 changes: 4 additions & 3 deletions packages/remix-vercel/__tests__/server-test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import supertest from "supertest";
import { createRequest } from "node-mocks-http";
import { createRequest, createResponse } from "node-mocks-http";
import { createServerWithHelpers } from "@vercel/node/dist/helpers";
import type { VercelRequest } from "@vercel/node";
import type { VercelRequest, VercelResponse } from "@vercel/node";
import {
createRequestHandler as createRemixRequestHandler,
Response as NodeResponse,
Expand Down Expand Up @@ -238,8 +238,9 @@ describe("vercel createRemixRequest", () => {
"Cache-Control": "max-age=300, s-maxage=3600",
},
}) as VercelRequest;
let response = createResponse() as unknown as VercelResponse;

expect(createRemixRequest(request)).toMatchInlineSnapshot(`
expect(createRemixRequest(request, response)).toMatchInlineSnapshot(`
NodeRequest {
"agent": undefined,
"compress": true,
Expand Down
15 changes: 8 additions & 7 deletions packages/remix-vercel/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export function createRequestHandler({
let handleRequest = createRemixRequestHandler(build, mode);

return async (req, res) => {
let request = createRemixRequest(req);
let request = createRemixRequest(req, res);
let loadContext = getLoadContext?.(req, res);

let response = (await handleRequest(request, loadContext)) as NodeResponse;
Expand Down Expand Up @@ -75,22 +75,23 @@ export function createRemixHeaders(
return headers;
}

export function createRemixRequest(req: VercelRequest): NodeRequest {
export function createRemixRequest(
req: VercelRequest,
res: VercelResponse
): NodeRequest {
let host = req.headers["x-forwarded-host"] || req.headers["host"];
// doesn't seem to be available on their req object!
let protocol = req.headers["x-forwarded-proto"] || "https";
let url = new URL(req.url!, `${protocol}://${host}`);

// Abort action/loaders once we can no longer write a response
let controller = new AbortController();

req.on("close", () => {
controller.abort();
});
res.on("close", () => controller.abort());

let init: NodeRequestInit = {
method: req.method,
headers: createRemixHeaders(req.headers),
signal: controller.signal,
signal: controller.signal as AbortSignal,
};

if (req.method !== "GET" && req.method !== "HEAD") {
Expand Down