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

Loosen restrictions on resource route response types when accessed externally #12848

Merged
merged 1 commit into from
Jan 23, 2025
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: 8 additions & 0 deletions .changeset/kind-hats-greet.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"react-router": patch
---

Stop erroring on resource routes that return raw strings/objects and instead serialize them as `text/plain` or `application/json` responses

- This only applies when accessed as a resource route without the `.data` extension
- When accessed from a Single Fetch `.data` request, they will still be encoded via `turbo-stream`
35 changes: 27 additions & 8 deletions integration/resource-routes-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ test.describe("loader in an app", async () => {
export default () => <div data-testid="redirect-destination">You made it!</div>
`,
"app/routes/defer.tsx": js`
export let loader = () => ({ data: 'whatever' });
export let loader = () => ({ data: Promise.resolve('whatever') });
`,
"app/routes/data[.]json.tsx": js`
export let loader = () => Response.json({ hello: "world" });
Expand Down Expand Up @@ -101,6 +101,11 @@ test.describe("loader in an app", async () => {
return { hello: 'world' };
}
`,
"app/routes/return-string.tsx": js`
export let loader = () => {
return 'hello world';
}
`,
"app/routes/throw-object.tsx": js`
export let loader = () => {
throw { but: 'why' };
Expand Down Expand Up @@ -207,9 +212,25 @@ test.describe("loader in an app", async () => {
expect(await res.text()).toEqual("Partial");
});

// TODO: This test should work once we bring over the changes from
// https://github.com/remix-run/remix/pull/9349 to the v7 branch
test.skip("should handle objects returned from resource routes", async ({
test("should convert strings returned from resource routes to text responses", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
let res = await app.goto("/return-string");
expect(res.status()).toBe(200);
expect(await res.text()).toEqual("hello world");
});

test("should convert non-strings returned from resource routes to JSON responses", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
let res = await app.goto("/return-object");
expect(res.status()).toBe(200);
expect(await res.json()).toEqual({ hello: "world" });
});

test("should handle objects returned from resource routes", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
Expand Down Expand Up @@ -256,10 +277,8 @@ test.describe("loader in an app", async () => {
}) => {
let app = new PlaywrightFixture(appFixture, page);
let res = await app.goto("/defer");
expect(res.status()).toBe(500);
expect(await res.text()).toMatch(
"Error: Expected a Response to be returned from resource route handler"
);
expect(res.status()).toBe(200);
expect(await res.json()).toEqual({ data: {} });
});
});

Expand Down
13 changes: 8 additions & 5 deletions packages/react-router/lib/server-runtime/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -504,12 +504,15 @@ async function handleResourceRequest(
requestContext: loadContext,
});

invariant(
isResponse(response),
"Expected a Response to be returned from resource route handler"
);
if (isResponse(response)) {
return response;
}

return response;
if (typeof response === "string") {
return new Response(response);
}

return Response.json(response);
} catch (error: unknown) {
if (isResponse(error)) {
// Note: Not functionally required but ensures that our response headers
Expand Down
Loading