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

fix: nextjs pages adapter response body #946

Merged
merged 4 commits into from
Sep 16, 2024
Merged

fix: nextjs pages adapter response body #946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16013ba
fix nextjs pages adapter response body
juliusmarminge Sep 14, 2024
c27bd66
add fixme
juliusmarminge Sep 14, 2024
343bb02
whoops
juliusmarminge Sep 14, 2024
e0dda6e
fix test
juliusmarminge Sep 14, 2024
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/cuddly-pets-jog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"uploadthing": patch
---

fix: nextjs pages adapter response body regression
3 changes: 2 additions & 1 deletion packages/uploadthing/src/next-legacy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const createRouteHandler = <TRouter extends FileRouter>(
for (const [name, value] of response.headers) {
res.setHeader(name, value);
}
return res.json(response.body);
// FIXME: Should be able to just forward it instead of consuming it first
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice, but iI don't think this is a huge deal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nah lets just ship as-is and see if we ever find it high priority enough to optimize a little bit... the json blobs are never huge anyways so it's not like we're gonna load a bunch of stuff into memory

return res.json(await response.json());
};
};
15 changes: 2 additions & 13 deletions packages/uploadthing/test/adapters.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,18 +356,7 @@ describe("adapters:next-legacy", async () => {

// Should proceed to generate a signed URL
const resJson = (json.mock.calls[0] as any[])[0];
const reader = (resJson as ReadableStream).getReader();
let data = "";
// eslint-disable-next-line no-constant-condition
while (true) {
const { done, value } = await reader.read();
if (done) {
break;
}
data += new TextDecoder().decode(value);
}
const jsonData = JSON.parse(data);
expect(jsonData).toEqual([
expect(resJson).toEqual([
{
customId: null,
key: expect.stringMatching(/.+/),
Expand All @@ -382,7 +371,7 @@ describe("adapters:next-legacy", async () => {
body: {
isDev: false,
awaitServerData: true,
fileKeys: [jsonData[0]?.key],
fileKeys: [resJson[0]?.key],
metadata: {},
callbackUrl: "http://localhost:3000/",
callbackSlug: "middleware",
Expand Down
Loading