refactor: extract Pages API route runtime#683
Conversation
|
/bigbonk |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Good extraction that aligns with the "generated entries should stay thin" principle from AGENTS.md. The entry now delegates to typed, testable helpers, and the new tests cover the key paths. A few things to address:
Behavioral change: writeHead now lowercases non-set-cookie headers
Old code (in the generated entry):
resHeaders[k] = v;New code (pages-node-compat.ts:182):
resHeaders[key.toLowerCase()] = Array.isArray(value) ? value.join(", ") : value;The old writeHead stored non-set-cookie header keys as-is (preserving original casing from the caller). The new code lowercases them. This is arguably more correct (and consistent with setHeader which always lowercased), but it is a behavior change. Since end() passes them to new Headers() which is case-insensitive anyway, the practical impact is minimal — but worth noting as intentional.
The old code also stored raw values directly (resHeaders[k] = v), so passing an array for a non-set-cookie header would store the array as-is, which would break new Headers() construction. The new code joins arrays with ", ". This is actually a fix.
Pre-existing bug: setHeader appends to setCookieHeaders instead of replacing
Both old and new code have this issue: setHeader("set-cookie", ...) always appends to setCookieHeaders. In Node.js, res.setHeader() is supposed to replace the previous value for that header name. Calling res.setHeader("set-cookie", "a=1") twice should result in one cookie, not two.
Not introduced by this PR, but worth tracking as a follow-up since this is now the canonical location for this logic.
Fragile error dispatch in pages-api-route.ts:76
if ((error as Error).message === "Request body too large") {
return new Response("Request body too large", { status: 413 });
}Matching on error message strings is fragile. parsePagesApiBody already uses PagesApiBodyParseError for structured error dispatch. Consider doing the same for body-too-large — e.g., a typed error class or reusing PagesApiBodyParseError with status 413, thrown from parsePagesApiBody / readPagesRequestBodyWithLimit. This would make the catch clause more robust and avoid accidental matches.
Duplication with api-handler.ts
The dev server (api-handler.ts) has parallel implementations: ApiBodyParseError, getMediaType, isJsonMediaType, parseBody, parseCookies. The new pages-node-compat.ts has equivalent versions for Web Request. They serve different environments (Node.js streams vs Web Request) so they can't be trivially unified, but the media-type helpers and error class could be shared to reduce drift. Not blocking.
Test coverage suggestions
The tests cover the important happy and error paths. A few missing scenarios worth adding given the stated goal of "add direct helper coverage":
matchwith no default export — thetypeof handler !== "function"branch (pages-api-route.ts:51)match: null— the 404 pathcreatePagesReqResdirectly —res.redirect(),res.setHeader/res.getHeader/res.writeHead,res.send()with object/string data. The Node compat layer is complex enough to warrant direct unit tests.
These are covered by integration tests in pages-router.test.ts, so not blocking, but would strengthen the helper test suite.
Overall
Clean refactor. The API surface is well-typed, the delegation pattern from the generated entry is thin, and the new files are well-structured. The fragile error message matching is the main thing I'd address before merging.
|
The review has been posted successfully on PR #683. Here's a summary of the key findings: Overall: Clean refactor that correctly follows the project's architecture guidelines. The generated entry is now thin, delegating to well-typed helpers in Key findings:
|
|
@southpolesteve Hey Steve, just wanna ask, is Vinext dead now that Next is prepping their official adapter for Cloudflare? |
|
Not dead at all! They announced the adapters API as stable, but there are still major limitations that adapters cannot address. You are still locked into the Turbopack toolchain, and adapters only cover build and deploy. During development, next dev runs exclusively in Node.js with no way to plug in a different runtime. From the vinext announcement blog:
So far, vinext has shown that the ceiling for performance and DX is significantly higher than what the current Next.js setup allows. |
|
/bigbonk review prior feedback responses:
|
There was a problem hiding this comment.
Review
Good extraction — the entry is now thin and delegates to well-typed helpers, consistent with the AGENTS.md "generated entries should stay thin" principle. The previous review covered the main structural points well. I'll focus on concrete items to address before merging.
1. Fragile "Request body too large" string match (blocking)
pages-api-route.ts:76 catches an error by matching on its .message:
if ((error as Error).message === "Request body too large") {
return new Response("Request body too large", { status: 413 });
}But the error thrown at pages-node-compat.ts:86, pages-node-compat.ts:103, and pages-node-compat.ts:109 is a plain Error, not a typed class. Meanwhile PagesApiBodyParseError already exists for structured dispatch. Two options:
Option A (preferred): Throw PagesApiBodyParseError with statusCode: 413 from readPagesRequestBodyWithLimit and the content-length check in parsePagesApiBody. Then the single instanceof PagesApiBodyParseError catch in handlePagesApiRoute handles both 400 and 413 cases. The separate string-match branch disappears entirely.
Option B: Create a dedicated PagesApiBodyTooLargeError class. Less elegant but works.
Either way, the current string match is fragile — any future error with that message text would be silently swallowed as a 413.
2. setHeader("set-cookie", ...) appends instead of replacing (pre-existing, not blocking)
Both setHeader (line 188) and writeHead (line 171) unconditionally append to setCookieHeaders:
setHeader(name, value) {
if (name.toLowerCase() === "set-cookie") {
// ...
setCookieHeaders.push(String(value)); // always appendsIn Node.js, res.setHeader() replaces the previous value. Calling res.setHeader("set-cookie", "a=1") twice should result in one cookie, not two. writeHead has the same issue — Node.js writeHead merges with previously set headers but doesn't double-append set-cookie values that were already set via setHeader.
Not introduced by this PR, but since this is now the canonical location for this logic, it's worth a tracking issue. If a Pages API handler does res.setHeader("set-cookie", "session=abc"); res.setHeader("set-cookie", "session=xyz");, the response will have both cookies instead of just the replacement.
3. parsePagesApiBody empty-body branch for form-urlencoded is unreachable (minor)
At pages-node-compat.ts:113-118:
if (!rawBody) {
return isJsonMediaType(mediaType)
? {}
: mediaType === "application/x-www-form-urlencoded"
? decodeQueryString(rawBody) // rawBody is "" here
: undefined;
}When rawBody is "" and media type is application/x-www-form-urlencoded, this calls decodeQueryString("") which returns {}. This works correctly, but the intent is confusing — it looks like it should return undefined or {} for an empty form POST. Consider adding a comment clarifying this is intentional, or simplifying to just {} for both JSON and form-urlencoded empty bodies since decodeQueryString("") always returns {}.
4. res.end() is called unconditionally after the handler (pages-api-route.ts:66-67)
await handler(req, res);
res.end();This is safe because end() has an if (ended) return; guard. But it means if the handler returns without calling res.end(), res.json(), res.send(), or res.redirect(), the response resolves with whatever status/headers/body were set so far (defaulting to 200, empty body). This is correct and matches the safety-net pattern. Just noting it's intentional behavior.
5. Test coverage gaps (non-blocking, suggestions)
The tests cover the happy paths well. A few missing scenarios that would strengthen the helper test suite:
match: null→ 404 response — trivial to add, one assertion- Missing default export → 500 —
createMatch(undefined as any)or similar res.redirect()— both the 1-arg and 2-arg formsres.writeHead()with headers — verify lowercasing and array joiningres.setHeader/res.getHeaderround-trip — especially forset-cookie
These are covered by integration tests in pages-router.test.ts, so not blocking.
Overall
Clean, well-structured extraction. The PagesApiBodyParseError string-match issue (item 1) is the only thing I'd address before merging — the rest are suggestions and pre-existing items worth tracking.
|
Review posted on PR #683. Here's a summary of the key findings: Blocking:
Non-blocking observations: |
- Replace fragile error.message string matches with typed PagesBodyParseError
(statusCode: 413) thrown from pages-node-compat.ts and api-handler.ts; the
existing instanceof catch in handlePagesApiRoute handles both 400 and 413
- Fix res.setHeader('set-cookie') to replace instead of append on repeated
calls, matching Node.js ServerResponse semantics
- Extract shared getMediaType/isJsonMediaType helpers and the body-parse error
class into pages-media-type.ts to eliminate drift between api-handler.ts and
pages-node-compat.ts
- Add unit tests for 404 (null match), missing default export (500), redirect
(1-arg and 2-arg), writeHead header lowercasing/array joining, setHeader/
getHeader round-trip, and set-cookie replace-not-append parity
|
/bigbonk re-review |
There was a problem hiding this comment.
Review
The fix commit (a8f925e) addresses the two blocking items from the previous reviews cleanly:
-
Fragile string match → structured error dispatch:
readPagesRequestBodyWithLimitandparsePagesApiBodynow throwPagesBodyParseErrorwithstatusCode: 413, and the catch inhandlePagesApiRouteuses a singleinstanceof PagesBodyParseErrorbranch. The separate string-match branch is gone. -
setHeader("set-cookie", ...)replace semantics:setCookieHeaders.length = 0before push. Verified by the new"res.setHeader replaces set-cookie on repeated calls"test.
The fix commit also added the test coverage gaps identified in previous reviews: match: null → 404, missing default export → 500, res.redirect() (1-arg and 2-arg), res.writeHead() header lowercasing/array joining, res.setHeader/res.getHeader round-trip.
Shared error class consolidation is done well. PagesBodyParseError was a duplicate of PagesApiBodyParseError — the fix extracts both into PagesBodyParseError in pages-media-type.ts and re-exports the old name as a deprecated alias for backwards compat. Both api-handler.ts (Node.js dev path) and pages-node-compat.ts (fetch-based prod path) now import from the shared module.
All 12 helper tests and all 20 entry-template tests pass.
LGTM — this is ready to merge.
|
The review has been posted on PR #683 with an approval. Here's a summary: Verdict: Approved The fix commit (
All 12 helper tests and 20 entry-template tests pass. |
3 participants
Summary
pages-server-entry.tsfocused on generated wiring by delegating API route execution tohandlePagesApiRouteVerification
vp check packages/vinext/src/server/pages-node-compat.ts packages/vinext/src/server/pages-api-route.ts packages/vinext/src/entries/pages-server-entry.ts tests/pages-api-route.test.ts tests/entry-templates.test.tsvp test run tests/pages-api-route.test.ts tests/entry-templates.test.tsvp test run tests/pages-router.test.ts -t "handles API routes returning JSON|handles dynamic API routes with query params|returns 400 for invalid JSON bodies on Pages API routes|preserves duplicate urlencoded body keys on Pages API routes|sends Buffer payloads from res.send\(\) as raw bytes|preserves invalid JSON failures for Pages API routes in production|preserves duplicate urlencoded body keys for Pages API routes in production|parses empty JSON bodies for Pages API routes in production as \{}|sends Buffer payloads from res.send\(\) as raw bytes in production|preserves content-length for getServerSideProps res.end\(\) short-circuit responses in production"vp run vinext#buildWritten by Codex.