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

feat(remix-server-runtime): strip body from request before calling loaders #3207

Merged
merged 2 commits into from
May 17, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
8 changes: 4 additions & 4 deletions docs/decisions/0002-do-not-clone-request.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ Status: accepted

## Context

To allow multiple loaders / actions to read the body of a request, we have been cloning the request before forwarding it to user-code. This is not the best thing to do as some runtimes will begin buffering the body to allow for multiple consumers. It is also goes against "the platform" that states a request body should only be consumed once.
To allow multiple loaders / actions to read the body of a request, we have been cloning the request before forwarding it to user-code. This is not the best thing to do as some runtimes will begin buffering the body to allow for multiple consumers. It also goes against "the platform" that states a request body should only be consumed once.

## Decision

Do not clone requests before they are passed to user-code (loaders, actions, handleDocumentRequest, handleDataRequest, etc.).
Do not clone requests before they are passed to user-code (actions, handleDocumentRequest, handleDataRequest), and remove body from request passed to loaders. Loaders should be thought of as a "GET" / "HEAD" request handler. These request methods are not allowed to have a body, therefore you should not be reading it in your Remix loader function.

## Consequences

If you are reading the request body in both an action and a loader this will now fail. Loaders should be thought of as a "GET" / "HEAD" request handler. These request methods are not allowed to have a body, therefore you should not be reading it in your Remix loader function.
Loaders always receive a null body for the request.

If you wish to continue reading the request body in multiple places for a single request against recommendations, consider using `.clone()` before reading it; just know this comes with tradeoffs.
If you are reading the request body in both an action and handleDocumentRequest or handleDataRequest this will now fail as the body will have already been read. If you wish to continue reading the request body in multiple places for a single request against recommendations, consider using `.clone()` before reading it; just know this comes with tradeoffs.
16 changes: 0 additions & 16 deletions packages/remix-server-runtime/responses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,19 +69,3 @@ export function isRedirectResponse(response: Response): boolean {
export function isCatchResponse(response: Response) {
return response.headers.get("X-Remix-Catch") != null;
}

export function extractData(response: Response): Promise<unknown> {
let contentType = response.headers.get("Content-Type");

if (contentType && /\bapplication\/json\b/.test(contentType)) {
return response.json();
}

// What other data types do we need to handle here? What other kinds of
// responses are people going to be returning from their loaders?
// - application/x-www-form-urlencoded ?
// - multipart/form-data ?
// - binary (audio/video) ?

return response.text();
}
7 changes: 6 additions & 1 deletion packages/remix-server-runtime/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -293,13 +293,18 @@ async function handleDocumentRequest({
);
}

let loaderRequest = new Request(request.url, {
...request,
body: null,
});

let routeLoaderResults = await Promise.allSettled(
matchesToLoad.map((match) =>
match.route.module.loader
? callRouteLoader({
loadContext,
match,
request,
request: loaderRequest,
})
: Promise.resolve(undefined)
)
Expand Down