fix: align Pages API body parsing and res.send(Buffer)#428
fix: align Pages API body parsing and res.send(Buffer)#428JaredStowell wants to merge 2 commits intocloudflare:mainfrom
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good set of fixes — all three changes align vinext closer to Next.js behavior, and the dev/prod parity is maintained across both code paths. Tests are thorough with both unit and integration coverage for each fix.
A few observations below, one minor issue and the rest are nits.
| if ((e as Error).message === "Request body too large") { | ||
| res.statusCode = 413; | ||
| res.end("Request body too large"); | ||
| if (e instanceof ApiBodyParseError) { |
There was a problem hiding this comment.
Minor issue: when an ApiBodyParseError is thrown, the code still calls server.ssrFixStacktrace(e) and console.error(e) and reportRequestError(...) before checking the error type. For a routine 400 (malformed JSON from the client), this will:
- Log a full stack trace to the console
- Report it as a request error to instrumentation
This is noisy for what is really a client error, not a server error. Consider checking for ApiBodyParseError first and returning early before the logging/reporting, or at least downgrading the log level.
| if (e instanceof ApiBodyParseError) { | |
| if (e instanceof ApiBodyParseError) { | |
| res.statusCode = e.statusCode; | |
| res.end(e.message); | |
| return true; | |
| } | |
| server.ssrFixStacktrace(e as Error); | |
| console.error(e); | |
| reportRequestError( | |
| e instanceof Error ? e : new Error(String(e)), | |
| { | |
| path: url, | |
| method: req.method ?? "GET", | |
| headers: Object.fromEntries( | |
| Object.entries(req.headers).map(([k, v]) => [ | |
| k, | |
| Array.isArray(v) ? v.join(", ") : String(v ?? ""), | |
| ]), | |
| ), | |
| }, | |
| { routerKind: "Pages Router", routePath: match.route.pattern, routeType: "route" }, | |
| ).catch(() => { | |
| /* ignore reporting errors */ | |
| }); |
| res.end(); | ||
| return await responsePromise; | ||
| } catch (e) { | ||
| if (e instanceof ApiBodyParseError) { |
There was a problem hiding this comment.
Same observation as in api-handler.ts — in the production entry, the ApiBodyParseError check happens after the generic console.error("[vinext] API error:", e) line. A 400 from malformed JSON will get logged as [vinext] API error: which is misleading since it's a client error. Consider reordering the check before the console.error.
| @@ -135,6 +140,14 @@ function enhanceApiObjects( | |||
| }; | |||
|
|
|||
| apiRes.send = function (data: unknown) { | |||
There was a problem hiding this comment.
Nit: Next.js's res.send() also handles Buffer-like objects via the content-type header and sets the Content-Length header. This implementation doesn't set Content-Length for the Buffer case. Not a blocker — just noting the difference for future reference.
| export const vinextConfig = ${vinextConfigJson}; | ||
|
|
||
| class ApiBodyParseError extends Error { | ||
| constructor(message, statusCode) { |
There was a problem hiding this comment.
Nit: in the generated (untyped) code, statusCode is assigned as a property on the instance but never declared. This works fine at runtime, but it might be clearer to assign it explicitly:
| constructor(message, statusCode) { | |
| constructor(message, statusCode) { | |
| super(message); | |
| this.statusCode = statusCode; | |
| this.name = 'ApiBodyParseError'; | |
| } |
Setting name makes stack traces more readable when this error does get logged (e.g., during debugging). Optional.
|
Review posted. Here's a summary of the feedback: Overall: Clean PR — all three fixes (Buffer send, urlencoded array preservation, malformed JSON 400) are correct and well-tested with both unit and integration coverage for dev and prod paths. Key feedback:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ca97c9203
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/bigbonk re-review |
| else { if (!resHeaders["content-type"]) resHeaders["content-type"] = "text/plain"; res.end(String(data)); } | ||
| if (Buffer.isBuffer(data)) { | ||
| if (!resHeaders["content-type"]) resHeaders["content-type"] = "application/octet-stream"; | ||
| res.end(data); |
There was a problem hiding this comment.
Dev/prod parity gap: the dev-mode api-handler.ts sets Content-Length for Buffer payloads (line 148), but this production path does not. The integration test for production (sends Buffer payloads from res.send() as raw bytes in production in pages-router.test.ts) also doesn't assert content-length, while the dev test does.
This means the same API handler will return Content-Length in dev but not in production — exactly the kind of divergence AGENTS.md warns against. Should be a one-liner:
| res.end(data); | |
| if (!resHeaders["content-type"]) resHeaders["content-type"] = "application/octet-stream"; | |
| resHeaders["content-length"] = String(data.length); |
| it("sends Buffer payloads from res.send() as raw bytes in production", async () => { | ||
| const res = await fetch(`${prodUrl}/api/send-buffer`); | ||
| expect(res.status).toBe(200); | ||
| expect(res.headers.get("content-type")).toContain("application/octet-stream"); |
There was a problem hiding this comment.
Since the dev-mode test asserts content-length (line 209 in api-handler.test.ts), this production test should too — otherwise the parity gap mentioned above goes undetected.
| expect(res.headers.get("content-type")).toContain("application/octet-stream"); | |
| expect(res.headers.get("content-type")).toContain("application/octet-stream"); | |
| expect(res.headers.get("content-length")).toBe("3"); |
| method: "POST", | ||
| headers: { | ||
| "content-type": "application/json", | ||
| }, |
There was a problem hiding this comment.
Same here — the dev integration test should also assert content-length to match the unit test:
| }, | |
| expect(res.headers.get("content-type")).toContain("application/octet-stream"); | |
| expect(res.headers.get("content-length")).toBe("3"); |
| await handler(apiReq, apiRes); | ||
| return true; | ||
| } catch (e) { | ||
| if (e instanceof ApiBodyParseError) { |
There was a problem hiding this comment.
Nit: the previous reviewer and Codex both flagged this, and the test already asserts ssrFixStacktrace and reportRequestError are NOT called — but that assertion currently passes only because ApiBodyParseError is caught before those calls. The control flow is correct, but it would be slightly clearer to separate the early-return from the logging block with a blank line, so a future reader immediately sees the intent:
| if (e instanceof ApiBodyParseError) { | |
| if (e instanceof ApiBodyParseError) { | |
| res.statusCode = e.statusCode; | |
| res.end(e.message); | |
| return true; | |
| } | |
| server.ssrFixStacktrace(e as Error); |
Trivial — feel free to ignore.
|
Review posted. Here's a summary of my feedback: Overall: Clean PR. All three fixes (Buffer send, urlencoded array preservation, malformed JSON 400) are correct, well-motivated, and thoroughly tested with both unit and integration coverage across dev and prod paths. Key finding — dev/prod parity gap on The dev-mode Minor nit: Suggested a blank line between the |
2 participants
Summary
Fix three Pages Router API compatibility gaps to match Next.js behavior more closely:
res.send(Buffer)now sends raw bytes withapplication/octet-streaminstead of JSON-stringifying Buffer metadataapplication/x-www-form-urlencodedbody parsing now preserves repeated keys as arrays400 Invalid JSONinstead of falling through as a raw stringThis updates both Pages API paths so dev and production stay in sync:
packages/vinext/src/server/api-handler.tspackages/vinext/src/entries/pages-server-entry.tsTests
Added coverage for all three cases in:
tests/api-handler.test.tstests/pages-router.test.tsAdded Pages Router fixture routes for integration coverage:
tests/fixtures/pages-basic/pages/api/echo-body.tstests/fixtures/pages-basic/pages/api/send-buffer.tsUpdated:
tests/__snapshots__/entry-templates.test.ts.snapVerification
Ran:
pnpm test tests/api-handler.test.tspnpm test tests/pages-router.test.tspnpm test tests/entry-templates.test.ts -upnpm run fmtpnpm run typecheckpnpm test