/bigbonk review this

Open in StackBlitz

npm i https://pkg.pr.new/vinext@281

commit: 04f01b1

I've reviewed and approved PR #281. Here's the summary:

Overall: Good fix. The core issue — gSSP headers/cookies/status being silently dropped — is correctly addressed in both the dev server and production server entry paths.

Key findings:

1. Prod path is solid: gsspRes is saved after gSSP runs, its accumulated headers/status/cookies are merged into the final Response via the Headers API, with correct append() for Set-Cookie. The res.end() short-circuit uses a responsePromise pattern that cleanly bridges the mock res to a Web Response.

2. Dev path works but is fragile: The dev server preserves res.statusCode explicitly, but custom headers from res.setHeader() survive only because Node.js writeHead() merges with previously-set headers on the same response object. If streamPageToResponse were refactored, this would silently break. Not blocking, but worth being aware of.

3. Minor dev/prod inconsistency: When gSSP calls res.end() to short-circuit, the dev path skips middleware header merging entirely (returns early), while the prod path still merges middleware headers into the response via sendCompressed. Unlikely to cause real issues but worth noting.

4. Tests are well-structured: Two fixture pages covering the important scenarios (custom headers + cookies + status code, and res.end() short-circuit), with assertions on all the right response properties.

github run

/bigbonk review this one more time

UnknownError: ProviderInitError: ProviderInitError

github run

@southpolesteve Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

/bigbonk review this one more time

UnknownError: ProviderInitError: ProviderInitError

github run

/bigbonk review this one more time

I've completed my review. Here's my analysis:

Review of PR #281

The fix is correct and well-structured. Both the production path (renderPage in index.ts) and the dev path (dev-server.ts) are properly updated. The previous review's findings still hold. I have one real bug to flag, plus a few observations.

Bug: Multiple Set-Cookie headers lost in prod-server.ts (pre-existing, but now exposed)

prod-server.ts:944-945 does this:

const responseHeaders: Record<string, string> = { ...middlewareHeaders };
response.headers.forEach((v, k) => { responseHeaders[k] = v; });

Headers.forEach() iterates Set-Cookie values as combined comma-separated strings with Headers.get() semantics, or as individual entries depending on the runtime. But since responseHeaders is Record<string, string>, the last Set-Cookie value wins if forEach is called per-entry. In the more common path, Headers.get("set-cookie") joins them with , , which corrupts multi-value Set-Cookie headers (cookies can't be comma-joined because Expires= dates contain commas).

This PR correctly uses responseHeaders.append("set-cookie", ...) in renderPage (index.ts:1468), which is the right approach. But when the response flows through prod-server.ts:945, that forEach + Record<string, string> pattern flattens it back. The resulting sendCompressed call passes the flattened object to writeHead, which doesn't understand multiple Set-Cookie values from a flat string.

This is a pre-existing bug, not introduced by this PR. But this PR makes it more visible since gSSP can now actually set cookies. Worth a follow-up fix — prod-server.ts should use Headers.getSetCookie() (available in modern runtimes) or response.headers.raw() to preserve array-valued Set-Cookie.

What looks good

1. Production renderPage path — Clean separation: save gsspRes, check headersSent for short-circuit, merge headers via Headers API with correct append() for Set-Cookie, force Content-Type: text/html after merge. All correct.

2. Dev server path — The header snapshot approach (headersBeforeGSSP / headersAfterGSSP diff) is robust. Removing captured headers from res before writeHead prevents duplication. Using res.setHeader(key, val) for array values before writeHead for scalar values in the headers object is the correct Node.js pattern.

3. res.end() short-circuit — Both paths handle it correctly. The prod path uses the responsePromise pattern from createReqRes (the end() method resolves it). The dev path checks res.writableEnded on the real Node.js ServerResponse. The comment about middleware headers already being on res in the dev path is accurate and helpful.

4. getHeaders() and headersSent additions — Correct implementations. getHeaders() returns a copy via Object.assign (good, no mutation risk). headersSent is backed by ended which matches Node.js semantics (headers are "sent" once end() is called).

5. Tests — Good coverage of the three key scenarios: custom headers + cookies + status, and short-circuit via res.end().

Minor observations

• var vs const/let mixing in generated code (index.ts:1255, 1460): The previous review noted this. Not a functional issue since the output targets modern runtimes, but var gsspRes = null alongside const responseHeaders = new Headers(...) is inconsistent with the surrounding style. Low priority.

• Dev statusCode precedence (dev-server.ts:490): if (!statusCode && res.statusCode !== 200) — this means middleware-set status takes precedence over gSSP status, which matches Next.js behavior. Good.

• Dev/prod parity for middleware headers on res.end() short-circuit: The previous review noted this. In dev, middleware headers are already on the ServerResponse object. In prod, prod-server.ts merges middlewareHeaders into the response from renderPage(). Both paths end up with middleware headers on the final response, just via different mechanisms. This is fine.

Verdict

The PR is correct for its stated scope. The Set-Cookie flattening issue in prod-server.ts is pre-existing and should be tracked separately. LGTM.

github run