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

Add future flag to throw request.signal.reason for aborted requests #11104

Merged
merged 7 commits into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/throw-abort-reason.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/router": minor
---

Add `createStaticHandler` `future.v7_throwAbortReason` flag to instead throw `request.signal.reason` when a request is aborted instead of our own custom `new Error()` with a message such as `query() call aborted`/`queryRoute() call aborted`.
19 changes: 19 additions & 0 deletions docs/guides/api-development-strategy.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,23 @@ const router = createBrowserRouter(routes, {
| `v7_prependBasename` | Prepend the router basename to navigate/fetch paths |
| [`v7_relativeSplatPath`][relativesplatpath] | Fix buggy relative path resolution in splat routes |

#### `createStaticHandler` Future Flags

These flags are only applicable when [SSR][ssr]-ing a React Router app:

```js
const handler = createStaticHandler(routes, {
future: {
v7_throwAbortReason: true,
},
});
```

| Flag | Description |
| ------------------------------------------- | ------------------------------------------------------------------- |
| [`v7_relativeSplatPath`][relativesplatpath] | Fix buggy relative path resolution in splat routes |
| [`v7_throwAbortReason`][abortreason] | Throw `request.signal.reason` if a query/queryRoute call is aborted |

### React Router Future Flags

These flags apply to both Data and non-Data Routers and are passed to the rendered React component:
Expand Down Expand Up @@ -98,3 +115,5 @@ These flags apply to both Data and non-Data Routers and are passed to the render
[starttransition]: https://react.dev/reference/react/startTransition
[partialhydration]: ../routers/create-browser-router#partial-hydration-data
[relativesplatpath]: ../hooks/use-resolved-path#splat-paths
[ssr]: ../guides/ssr
[abortreason]: ../routers/create-static-handler#handlerqueryrequest-opts
27 changes: 23 additions & 4 deletions docs/routers/create-static-handler.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,27 @@ export async function renderHtml(req) {

```ts
declare function createStaticHandler(
routes: RouteObject[],
opts?: {
basename?: string;
}
routes: AgnosticRouteObject[],
opts?: CreateStaticHandlerOptions
): StaticHandler;

interface CreateStaticHandlerOptions {
basename?: string;
future?: Partial<StaticHandlerFutureConfig>;
mapRouteProperties?: MapRoutePropertiesFunction;
}

interface StaticHandlerFutureConfig {
v7_relativeSplatPath: boolean;
v7_throwAbortReason: boolean;
}

interface MapRoutePropertiesFunction {
(route: AgnosticRouteObject): {
hasErrorBoundary: boolean;
} & Record<string, any>;
}

interface StaticHandler {
dataRoutes: AgnosticDataRouteObject[];
query(
Expand All @@ -86,6 +101,8 @@ These are the same `routes`/`basename` you would pass to [`createBrowserRouter`]

The `handler.query()` method takes in a Fetch request, performs route matching, and executes all relevant route action/loader methods depending on the request. The return `context` value contains all of the information required to render the HTML document for the request (route-level `actionData`, `loaderData`, `errors`, etc.). If any of the matched routes return or throw a redirect response, then `query()` will return that redirect in the form of Fetch `Response`.

If a request is aborted, `query` will throw an error such as `Error("query() call aborted")`. If you want to throw the native `AbortSignal.reason` (by default a `DOMException`) you can opt-in into the `future.v7_throwAbortReason` future flag.

### `opts.requestContext`

If you need to pass information from your server into Remix actions/loaders, you can do so with `opts.requestContext` and it will show up in your actions/loaders in the context parameter.
Expand Down Expand Up @@ -115,6 +132,8 @@ export async function render(req: express.Request) {

The `handler.queryRoute` is a more-targeted version that queries a singular route and runs it's loader or action based on the request. By default, it will match the target route based on the request URL. The return value is the values returned from the loader or action, which is usually a `Response` object.

If a request is aborted, `query` will throw an error such as `Error("query() call aborted")`. If you want to throw the native `AbortSignal.reason` (by default a `DOMException`) you can opt-in into the `future.v7_throwAbortReason` future flag.

### `opts.routeId`

If you need to call a specific route action/loader that doesn't exactly correspond to the URL (for example, a parent route loader), you can specify a `routeId`:
Expand Down
188 changes: 188 additions & 0 deletions packages/router/__tests__/ssr-test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
/**
* @jest-environment node
*/

import type { StaticHandler, StaticHandlerContext } from "../router";
import { UNSAFE_DEFERRED_SYMBOL, createStaticHandler } from "../router";
import {
Expand Down Expand Up @@ -652,6 +656,98 @@ describe("ssr", () => {
);
});

it("should handle aborted load requests (v7_throwAbortReason=true)", async () => {
let dfd = createDeferred();
let controller = new AbortController();
let { query } = createStaticHandler(
[
{
id: "root",
path: "/path",
loader: () => dfd.promise,
},
],
{ future: { v7_throwAbortReason: true } }
);
let request = createRequest("/path?key=value", {
signal: controller.signal,
});
let e;
try {
let contextPromise = query(request);
controller.abort();
// This should resolve even though we never resolved the loader
await contextPromise;
} catch (_e) {
e = _e;
}
expect(e).toBeInstanceOf(DOMException);
expect(e.name).toBe("AbortError");
expect(e.message).toBe("This operation was aborted");
});

it("should handle aborted submit requests (v7_throwAbortReason=true)", async () => {
let dfd = createDeferred();
let controller = new AbortController();
let { query } = createStaticHandler(
[
{
id: "root",
path: "/path",
action: () => dfd.promise,
},
],
{ future: { v7_throwAbortReason: true } }
);
let request = createSubmitRequest("/path?key=value", {
signal: controller.signal,
});
let e;
try {
let contextPromise = query(request);
controller.abort();
// This should resolve even though we never resolved the loader
await contextPromise;
} catch (_e) {
e = _e;
}
expect(e).toBeInstanceOf(DOMException);
expect(e.name).toBe("AbortError");
expect(e.message).toBe("This operation was aborted");
});

it("should handle aborted requests (v7_throwAbortReason=true + custom reason)", async () => {
let dfd = createDeferred();
let controller = new AbortController();
let { query } = createStaticHandler(
[
{
id: "root",
path: "/path",
loader: () => dfd.promise,
},
],
{ future: { v7_throwAbortReason: true } }
);
let request = createRequest("/path?key=value", {
signal: controller.signal,
});
let e;
try {
let contextPromise = query(request);
// Note this works in Node 18+ - but it does not work if using the
// `abort-controller` polyfill which doesn't yet support a custom `reason`
// See: https://github.com/mysticatea/abort-controller/issues/33
controller.abort(new Error("Oh no!"));
Comment on lines +746 to +749
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth calling out - this could be an issue in Remix apps that still use the polyfills if we wanted to use a custom reason

// This should resolve even though we never resolved the loader
await contextPromise;
} catch (_e) {
e = _e;
}
expect(e).toBeInstanceOf(Error);
expect(e.message).toBe("Oh no!");
});

it("should assign signals to requests by default (per the", async () => {
let { query } = createStaticHandler(SSR_ROUTES);
let request = createRequest("/", { signal: undefined });
Expand Down Expand Up @@ -1951,6 +2047,98 @@ describe("ssr", () => {
);
});

it("should handle aborted load requests (v7_throwAbortReason=true)", async () => {
let dfd = createDeferred();
let controller = new AbortController();
let { queryRoute } = createStaticHandler(
[
{
id: "root",
path: "/path",
loader: () => dfd.promise,
},
],
{ future: { v7_throwAbortReason: true } }
);
let request = createRequest("/path?key=value", {
signal: controller.signal,
});
let e;
try {
let statePromise = queryRoute(request, { routeId: "root" });
controller.abort();
// This should resolve even though we never resolved the loader
await statePromise;
} catch (_e) {
e = _e;
}
expect(e).toBeInstanceOf(DOMException);
expect(e.name).toBe("AbortError");
expect(e.message).toBe("This operation was aborted");
});

it("should handle aborted submit requests (v7_throwAbortReason=true)", async () => {
let dfd = createDeferred();
let controller = new AbortController();
let { queryRoute } = createStaticHandler(
[
{
id: "root",
path: "/path",
action: () => dfd.promise,
},
],
{ future: { v7_throwAbortReason: true } }
);
let request = createSubmitRequest("/path?key=value", {
signal: controller.signal,
});
let e;
try {
let statePromise = queryRoute(request, { routeId: "root" });
controller.abort();
// This should resolve even though we never resolved the loader
await statePromise;
} catch (_e) {
e = _e;
}
expect(e).toBeInstanceOf(DOMException);
expect(e.name).toBe("AbortError");
expect(e.message).toBe("This operation was aborted");
});

it("should handle aborted load requests (v7_throwAbortReason=true + custom reason)", async () => {
let dfd = createDeferred();
let controller = new AbortController();
let { queryRoute } = createStaticHandler(
[
{
id: "root",
path: "/path",
loader: () => dfd.promise,
},
],
{ future: { v7_throwAbortReason: true } }
);
let request = createRequest("/path?key=value", {
signal: controller.signal,
});
let e;
try {
let statePromise = queryRoute(request, { routeId: "root" });
// Note this works in Node 18+ - but it does not work if using the
// `abort-controller` polyfill which doesn't yet support a custom `reason`
// See: https://github.com/mysticatea/abort-controller/issues/33
controller.abort(new Error("Oh no!"));
// This should resolve even though we never resolved the loader
await statePromise;
} catch (_e) {
e = _e;
}
expect(e).toBeInstanceOf(Error);
expect(e.message).toBe("Oh no!");
});

it("should assign signals to requests by default (per the spec)", async () => {
let { queryRoute } = createStaticHandler(SSR_ROUTES);
let request = createRequest("/", { signal: undefined });
Expand Down
27 changes: 19 additions & 8 deletions packages/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2823,6 +2823,7 @@ export const UNSAFE_DEFERRED_SYMBOL = Symbol("deferred");
*/
export interface StaticHandlerFutureConfig {
v7_relativeSplatPath: boolean;
v7_throwAbortReason: boolean;
}

export interface CreateStaticHandlerOptions {
Expand Down Expand Up @@ -2861,6 +2862,7 @@ export function createStaticHandler(
// Config driven behavior flags
let future: StaticHandlerFutureConfig = {
v7_relativeSplatPath: false,
v7_throwAbortReason: false,
...(opts ? opts.future : null),
};

Expand Down Expand Up @@ -3130,10 +3132,7 @@ export function createStaticHandler(
);

if (request.signal.aborted) {
let method = isRouteRequest ? "queryRoute" : "query";
throw new Error(
`${method}() call aborted: ${request.method} ${request.url}`
);
throwStaticHandlerAbortedError(request, isRouteRequest, future);
}
}

Expand Down Expand Up @@ -3301,10 +3300,7 @@ export function createStaticHandler(
]);

if (request.signal.aborted) {
let method = isRouteRequest ? "queryRoute" : "query";
throw new Error(
`${method}() call aborted: ${request.method} ${request.url}`
);
throwStaticHandlerAbortedError(request, isRouteRequest, future);
}

// Process and commit output from loaders
Expand Down Expand Up @@ -3369,6 +3365,21 @@ export function getStaticContextFromError(
return newContext;
}

function throwStaticHandlerAbortedError(
request: Request,
isRouteRequest: boolean,
future: StaticHandlerFutureConfig
) {
let method = isRouteRequest ? "queryRoute" : "query";
if (future.v7_throwAbortReason && request.signal.reason !== undefined) {
throw request.signal.reason;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only downside I see here is that we lose the request method/URL that we include in the custom error, but I think if folks need that then they should be passing a custom reason when they call controller.abort. Right now, in Remix we don't expose that for customization - but we could send our own DOMException with the method/URL in the message if we wanted to.

} else {
throw new Error(
`${method}() call aborted: ${request.method} ${request.url}`
);
}
}

function isSubmissionNavigation(
opts: BaseNavigateOrFetchOptions
): opts is SubmissionNavigateOptions {
Expand Down
Loading