fix(node,fetch,core): only send the standard-server header when content headers are ambiguous - #59
fix(node,fetch,core): only send the standard-server header when content headers are ambiguous#59dinwwwh wants to merge 3 commits into
Conversation
…nt headers are ambiguous File and blob responses carried a `standard-server: file` header on every request, even though a receiver can already identify them from content-type/content-length alone. Adds `resolveStandardBodyHint` to core, which resolves the body hint a receiver lands on from the standard headers, and uses it in the node and fetch adapters to skip the header when the headers already resolve to a file. Streams keep the hint unconditionally: their length is unknown at that point, but the transport can still add a content-length (an empty stream), which would read back as a file.
@standardserver/aws-lambda
@standardserver/bun
@standardserver/core
@standardserver/deno
@standardserver/fastify
@standardserver/fetch
@standardserver/node
@standardserver/peer
@standardserver/shared
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Logic is sound and well tested — one stale doc line worth a touch-up.
Reviewed changes — PR #59 stops emitting standard-server: file on every file/blob body, sending it only when the content headers would otherwise lead a receiver to parse the body as something other than a file.
- core
resolveStandardBodyHint(headers)— new function mirroring the body parsers' inference: a recognizedstandard-servervalue wins, otherwise content-type/content-length determine the hint. Careful unit coverage of every branch (empty content-type, repeated headers, empty-array unset convention, NaN-size). - fetch/node blob send —
content-lengthis now written only whenNumber.isFinite(body.size)(NaN-size bun-stream returns the stream without a boguscontent-length), andstandard-server: fileis set only whenresolveStandardBodyHint(headers)doesn't already resolve tofile. - stream send — hint kept unconditionally, since an empty stream can still pick up
content-length: 0from the transport and read back as a file. - fastify client-server hook — empty
content-typenow round-trips via acustom/emptyplaceholder instead of being deleted, preserving the typeless-empty-blob case.
The conditional is safe: the resolver returns file exactly when a hint-less receiver would already parse the body as a file (content-length present and content-type not a common type), and returns something else exactly when the hint is required to force file parsing (common content-type, or no content-length). The removed-hint edge (empty + typeless blob, which now sends no header) is covered by the existing empty-file case in tests/data-transfer.test.ts, which round-trips new File([], '', { type: '' }) through every transport and asserts type === '' — confirming empty content-type survives the wire. NaN-size blobs correctly keep the hint (pinned in packages/node/src/body.test.ts:601 and the fetch equivalents).
ℹ️ Nitpicks
README.md:170still says a file/blob body "automatically sets the ...standard-serverheader", but this PR makes it conditional. Worth rewording to note the header is only set when the content headers would otherwise mislead the receiver.
DeepSeek Flash (default — pick a model for stronger reviews) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The new commit correctly closes the empty-body gap the initial change left open, with tests that would actually fail without the fix.
Reviewed changes — incremental delta since the prior pullfrog review (head aebc021 → 17ada9c, "fix(fetch,node): keep binary bodies identifiable on bun"):
- Restored empty-body safety net — both blob senders (
toFetchBody,toNodeHttpBody) now emitstandard-server: filewheneverbody.size === 0, on top of the existing "resolver says non-file" rule. This covers the empty+typeless-blob case that the initial change had stopped sending a hint for: bun drops an emptycontent-typeand exposes a zero-length body as no body at all, so without the hint the receiver reads nothing. - Fetch receiver fix —
toStandardBodynow treatscontent-length: 0with a null body as an explicit empty body, returning an emptyFileinstead ofundefined; and sourcesFile.typefrom thecontent-typeheader rather thanblob.type(which bun empties once.bodyhas been touched). - Tests — new coverage pins the empty-file receive path (would fail without the
contentLength !== '0'fix) and the empty/common-content-type/empty-typeless send paths.
I verified the delta end-to-end: the body.size === 0 short-circuit only adds a redundant hint where resolveStandardBodyHint already returns file (so receiver behavior is unchanged), and genuinely needed for the empty typeless case. The node receiver needs no equivalent change — node http doesn't null zero-length bodies and its file branch already handles content-length: 0. Locally re-ran the fetch/node/core body suites (114 tests), data-transfer.test.ts (176), and fastify response (16) — all pass.
DeepSeek Flash (default — pick a model for stronger reviews) ｜ 𝕏
17ada9c to
6572945
Compare
There was a problem hiding this comment.
Important
The "improve" commit silently reverts the empty-file receive fix that the previous commit introduced (and the last Pullfrog review verified): the fetch receiver's File.type is sourced from re.blob() again instead of the content-type header, and the only test pinning the empty-file receive path was deleted. No rationale appears in the commit or PR body.
Reviewed changes — incremental delta since the prior pullfrog review (head 17ada9c → 6572945, "improve"):
- fetch receiver empty-file revert —
toStandardBody's file branch sourcesFile.typefromre.blob()again rather than thecontent-typeheader. The null-body early-return (hint === null && re.body === null && contentLength !== '0') was also removed; that part is fine, since senders always hint empty bodies and the line-22 guard covers typeless null bodies. - deleted
empty filereceive test — the fetch case pinningResponse(null, { 'content-length': '0', 'content-type': 'application/pdf' })→ an emptyFilecarrying the header's type was removed and not replaced, leaving the empty-file receive path untested again. test:coverage— now runspnpm run -r testbefore the coverage pass.- node/fetch send comments — empty-body blob-send branch comments reworded; logic unchanged.
⚠️ Empty-file receive fix reverted, leaving the path untested
packages/fetch/src/body.ts:57 now builds the received file as new File([blob], …, { type: blob.type }), reverting 17ada9c, whose rationale (in the comment this commit removes) was that "bun empties the blob type once .body has been touched". If that bun behavior still holds, an empty File/Blob with a real content-type (e.g. application/pdf) round-tripped through bun loses its type on receive, because re.blob() yields an empty-type blob. The commit also deletes the one test that pinned the desired behavior, so this can regress silently.
Note this delta is invisible in the base→head diff (base already used blob.type, and the test only ever existed between 17ada9c and 6572945), which is likely why it slipped through. Please confirm bun is no longer affected (and add coverage for the empty-file-with-content-type receive path), or restore the header source and the test.
Technical details
# Revert of empty-file receive fix (17ada9c -> 6572945)
## Affected sites
- packages/fetch/src/body.ts:57 — `new File([blob], fileName ?? 'blob', { type: blob.type })`;
17ada9c used `type: contentType ?? ''` (content-type header) because bun empties
`blob.type` once the body is read.
- packages/fetch/src/body.test.ts — the `'empty file (a zero content-length is an empty
body, not a missing one)'` receive test was deleted with no replacement.
## Required outcome
- Empty file/blob with a non-empty content-type must keep its type on receive even when the
transport yields a null/empty body (the bun case 17ada9c addressed).
## Open questions for the human
- Is the bun `blob.type`-emptying bug still a concern, or was `blob.type` verified correct
across runtimes? If safe, re-add a test pinning type preservation; if not, restore the
`content-type`-header source.
- Removing the null-body guard itself is correct/needed (senders always hint `body.size === 0`;
line 22 covers typeless null bodies) — no action needed there.DeepSeek Flash (default — pick a model for stronger reviews) ｜ 𝕏
There was a problem hiding this comment.
Important
The new "improve" commit (2ef4ede) silently removes the node receiver's GET/HEAD body-parsing guard together with its only test. No rationale appears in the commit or PR body. Please confirm this is intentional (and why) or restore it.
Reviewed changes — incremental delta since the prior pullfrog review (head 6572945 → 2ef4ede, "improve"):
- Removed GET/HEAD body guard (node receive) —
toStandardBodyno longer returnsundefinedearly forGET/HEADrequests that arrive without an explicitstandard-server/hintbut with headers implying a body, and the only test pinning that behavior (ignores body parsing for GET requests even when headers imply a body) was deleted.
⚠️ GET/HEAD body-parsing guard removed without rationale
Commit 2ef4ede deletes EMPTY_BODY_METHOD_SET and the early return undefined in packages/node/src/body.ts that made toStandardBody ignore the body of GET/HEAD requests lacking an explicit hint, plus the sole test that pinned it. The MDN-referenced guard was a node-only safety net (the fetch receiver has no equivalent), so this changes receive-side behavior: a GET/HEAD request whose headers imply a body — e.g. content-type: application/json with a readable stream, or a content-length + content-type — is now parsed (returning a JSON value or a File) instead of returning undefined and leaving the stream untouched. The commit message is just "improve", so it's unclear whether this is an intentional parity change (aligning node with fetch) or an accidental revert; either way the deleted test means the new behavior is no longer pinned. Adding parity with fetch is a defensible cleanup, but it deserves a stated reason and, if kept, a replacement assertion for the empty-case (with content-type but no body, the json branch already returns undefined, so a guard-like outcome often still holds — worth confirming).
Technical details
# GET/HEAD body-parsing guard removed (commit 2ef4ede)
## Affected sites
- packages/node/src/body.ts — `EMPTY_BODY_METHOD_SET = new Set(['GET','HEAD'])` (old L18-21),
the early `if (hint === undefined && EMPTY_BODY_METHOD_SET.has(toStandardMethod(req.method))) return undefined`
(old L44-46), and the now-unused `toStandardMethod` import (old L10) were removed.
- packages/node/src/body.test.ts — `'ignores body parsing for GET requests even when headers imply a body'`
(old L40-48) deleted with no replacement.
## Behavior change
`GET`/`HEAD` node http requests carrying headers that imply a body (content-type present with a
stream, or content-length + content-type) now get that body read and parsed by `toStandardBody`
(parseEmptyableJSON → value, or _streamToFile → File), where before they returned `undefined` and
left the stream untouched. No other test depends on the old guard; 925-test suite still passes.
## Required outcome
- Author explicitly confirms the removal is intentional and why (fetch parity? "GET with a body
should parse"?), or restores the guard and its test.
## Open questions for the human
- Do any node adapters/downstream rely on GET/HEAD bodies being ignored?
- Is this a leftover from the standard-server header work, or a deliberate parity change?Note: the prior review's flagged fetch-receive concern (File.type sourced from blob.type in packages/fetch/src/body.ts:57, reverting type preservation for empty file/blob round-trips) remains open — 2ef4ede doesn't touch the fetch adapter. It is tracked in the PR summary snapshot.
DeepSeek Flash (default — pick a model for stronger reviews) ｜ 𝕏
|
Closing: the premise doesn't hold. Dropping the |
1 participant
File and blob bodies used to carry a
standard-server: fileheader on every message, even when a receiver could already identify them fromcontent-type/content-lengthalone. That header is now sent only when the content headers would lead somewhere else, so ordinary file responses go out without it.New
resolveStandardBodyHint(headers)in core resolves the hint a receiver lands on from the standard headers: thestandard-serverheader when it holds a recognized value, otherwise the same content-type/content-length inference the body parsers do. The node and fetch adapters use it in their blob branch and set the header only when the headers don't already resolve tofile.Fixes
standard-server— one fewer non-standard header on every file response.application/json,multipart/form-data,application/x-www-form-urlencoded,text/event-stream), a body with no content-type and no length, and aNaN-size blob (BunS3).content-length— an empty stream goes out ascontent-length: 0and would come back as a file.content-type; it round-trips it through a placeholder so a typeless blob keeps its (valid, empty) content-type across the wire.Notes for reviewers
content-typestays a valid content-type everywhere: it is not normalized to "absent", matching what the parsers already do.resolveStandardBodyHintignores an unrecognizedstandard-servervalue and falls back to the content headers. The parsers instead treat any unknown hint as an opaque stream. That divergence is inert today — the senders only consult the resolver when no hint is set — but it needs a decision before this replaces the parsers' own if-chains.standard-serverto override the auto-set hint, and the[]unset-header convention still suppresses it.Testing
pnpm test(925 tests), typecheck and lint all pass. New core coverage for every branch ofresolveStandardBodyHint, plus adapter cases for a common-content-type blob, an empty blob, and an empty blob with no content-type.