fix(core): a {value, type} object is no longer encoded as a file (#701) - #711
Open
rejifald wants to merge 2 commits into
Open
fix(core): a {value, type} object is no longer encoded as a file (#701)#711rejifald wants to merge 2 commits into
rejifald wants to merge 2 commits into
Conversation
`isFileWrapper` treated any object carrying a `type` key as a multipart
file wrapper, so a plain domain object — `{ value: 100, type: 'refund' }`
— was encoded as one tiny Blob and its siblings silently dropped. The
call still returned 200. On a money path that is data loss with no
signal: the server sees a 3-byte part named `refund`, typed `refund`, and
never learns the currency. Under `json` nesting it is worse — the whole
object hoists out and the `payload` part arrives as `{}`.
The guard the comment above the predicate describes was written for
exactly this harm — it already names `{ value: 100, currency: 'USD' }` —
but `type` was left in the disjunction, and `type` is far too ordinary a
domain key to prove file-ness. It is a *modifier* on a part that is
already a file (it sets the part's content type, which `appendFilePart`
still honours), never the thing that makes one. A wrapper is now a file
only when `value` is binary or an explicit `filename` names the part.
The binary arm widens from `Uint8Array` to `ArrayBuffer.isView` in the
same edit, and that is what keeps the narrowing honest: without it,
`{ value: new Float32Array(…), type: 'audio/pcm' }` would have silently
demoted from a file part to a nested object — trading one instance of
this bug for another. `Blob` already covers `File` and `Buffer` is a
`Uint8Array`; the view test adds the remaining TypedArrays and `DataView`.
BREAKING CHANGE: this narrows a detection predicate, and the blast radius
is exactly one shape — a wrapper with a NON-binary `value`, a `type`, and
no `filename`. `{ value: csv, type: 'text/csv' }` was a documented way to
give a text part an explicit content type (the upload blog names it), and
it now encodes as `k[value]` + `k[type]` string parts instead. A binary
`value` carrying a `type` is unaffected, as is anything with a
`filename`. Migration is one key: add a `filename`, or pass
`new Blob([value], { type })`.
Shipped now rather than deferred because the package is at 1.0.0-rc.7 and
the alternative is cutting 1.0.0 with silent data loss on a money path.
The CHANGELOG entry sits under `### Changed`, marked BREAKING, for the
same reason — calling it a bare `fix` would hide the one shape that
moves.
Three tests land in multipart-nesting.spec.ts beside the
`{ value, currency }` case this one slipped past: the domain object keeps
every sibling, a binary wrapper carrying only a `type` is still a file
part with its content type intact, and the `filename` arm still hoists a
file under `json` nesting.
Refs #701 — §1, §3, §4 and §5 are still live.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
) The previous commit paired the `type` narrowing with widening the binary arm from `Uint8Array` to `ArrayBuffer.isView`, and justified it as the thing that "keeps the narrowing honest". Measured against the size gate, that justification does not survive. `check:size` is a CI job and the core entry's budget has almost nothing left: `origin/main` measures 24664 B gzip against a 24678.4 B budget — 14.4 bytes. The widened predicate came in at 24671 B (7.4 B left), so it spent half the remaining margin of a budget shared with every other core PR in flight. Reordering the disjunction recovered 2 B and hoisting `w.value` to a local recovered none; the cost is irreducible because `ArrayBuffer.isView(` is a token the bundle does not otherwise contain. Dropping it lands at 24658 B — 6 bytes BELOW main, so this PR now gives the budget back instead of eating it. The correctness argument was also overstated. I claimed an exotic view in a wrapper would "trade one instance of this bug for another"; it does not. `{ value: new Float32Array([1, 2]), type: 'audio/pcm' }` recurses to `k[value][0]=1`, `k[value][1]=2`, `k[type]=audio/pcm` — every byte still on the wire, in a shape the server rejects. That is a loud 400, not the silent 200-with-fields-missing this predicate exists to prevent. The two are not the same harm class, and only the second one justifies spending a contended budget. So the predicate is now exactly the original minus `type`: Blob/File, Uint8Array/Buffer, ArrayBuffer, or an explicit `filename`. A condition changed rather than a condition added. The exotic-view gap is real and stays real — `appendFilePart` would already encode a `DataView` correctly, only the detector is narrow. It is recorded in the comment block above the predicate and tracked separately rather than smuggled in here. No test changes: the binary-wrapper test uses a `Uint8Array`, which both spellings accept. CHANGELOG migration note now names the binary types exactly instead of saying "TypedArray". Gates: lint, types, 1494 tests, changelog, contract, unknown-keys all green; `check:size` 24.08/24.10 KB whole entry and 21.50/21.55 KB for `import { stitch }`, both with more headroom than main. Refs #701 — §1, §3, §4 and §5 are still live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
A multipart body containing a plain domain object with a
typekey had that object encoded as asingle tiny file part, with every sibling key silently dropped — and the call returned
200.packages/core/src/http-adapter.ts:393-402(onorigin/main):typeis one of the most ordinary keys in a domain model.{ value: 100, type: 'refund' }is money,not an upload. Reproduced against the mock server on
origin/mainbefore touching anything — thewire showed:
currencyandtypenever left the process. Undermultipart.nesting: 'json'it is worse: theobject is hoisted out of the payload entirely and the JSON part ships as
{}.The comment directly above the predicate (
http-adapter.ts:387-392) already describes this exactharm and already names
{ value: 100, currency: 'USD' }as the shape that must not be a file.The guard was written;
typeis the key that slipped through it.This is §2 of #701.
The fix
One condition removed. A wrapper is a file only when
valueis binary, or an explicitfilenamenames the part:
typestays a modifier:appendFilePartstill reads it to set an already-file part's contenttype. It just no longer creates one. The comment block above the predicate is updated to say so.
Semver: breaking, not a plain
fixThe CHANGELOG entry is under
### Changed, markedBREAKING CHANGE, with a matching footer inthe commit body. (The subject stays
fix(core):— it is the defect being fixed.)The reasoning:
value, atype, andno
filename. That shape was documented —apps/docs/content/blog/upload-a-file-with-a-progress-bar.mdx:65presents the{ value, filename?, type? }wrapper as being for when you want to name the part "or set itscontent type explicitly". A caller doing
{ value: csv, type: 'text/csv' }gets a wire change:one typed file part becomes
k[value]+k[type]string parts.valuecarrying atypeis still a file part with itscontent type intact (pinned by a new test). Anything with a
filenameis still a file part.fixwould hide the one shape that moves, from the readers most likely to berelying on it.
Migration is one key: add a
filename, or passnew Blob([value], { type }).Shipping now rather than deferring to a later major: the package is at
1.0.0-rc.7, and thealternative is cutting
1.0.0with silent data loss on a money path.A widening I tried and then removed — and why
The first commit on this branch also widened the binary arm from
Uint8ArraytoArrayBuffer.isView, so that{ value: new Float32Array(…), type: 'audio/pcm' }would not demotefrom a file part when
typestopped qualifying. The second commit takes it back out. Both reasonsare worth stating, because the first draft of this PR argued the opposite:
It did not pay for itself on the size gate.
check:sizeis its own CI job, and the core entry'sbudget has almost nothing left:
origin/mainArrayBuffer.isViewThe widened version spent half the remaining margin of a budget shared with every core PR in flight.
Reordering the disjunction recovered 2 B; hoisting
w.valueto a local recovered none — the cost isirreducible because
ArrayBuffer.isView(is a token the bundle does not otherwise contain. Nobudget was raised — as shipped this PR is 6 bytes below
mainand hands margin back.And the correctness argument was overstated. I claimed an exotic view would "trade one instance
of this bug for another". It does not.
{ value: new Float32Array([1, 2]) }recurses tok[value][0]=1,k[value][1]=2— every byte still on the wire, in a shape the server rejects. Thatis a loud 400, not the silent 200-with-fields-missing this predicate exists to stop. Different harm
class; only the second one justifies spending a contended budget.
The exotic-view gap is real and stays real —
appendFilePartwould already encode aDataViewcorrectly, only the detector is narrow. It is recorded in the comment above the predicate and left
for its own change rather than smuggled in here.
Tested
Three tests added to
packages/core/test/multipart-nesting.spec.ts, beside the existing{ value, currency }case this bug slipped past — same style, same end-to-end assertion against theraw multipart body the mock server records:
{ value: 100, type: 'refund', currency: 'USD' }round-trips as a normal nested object —refund[value],refund[type],refund[currency]all present, no file part.type(no filename) is still a file part, and thetypestill reaches the wire as the part's content type.jsonnesting,{ value: 'hello', filename: 'n.txt' }still hoists to a named file partwhile a sibling
{ value, type }domain object rides intact inside the JSON payload.Tests 1 and 3 were confirmed red on
origin/mainbefore the fix and green after; test 2 is greenboth sides (it is the invariant guarding the narrowing).
Gates, all green:
prettier --write(changed files)pnpm --filter stitchapi check:lintpnpm --filter stitchapi check:typespnpm --filter stitchapi testpnpm --filter stitchapi build && check:sizestitchnode scripts/check-changelog.mjsnode scripts/check-contract.mjsnode scripts/check-unknown-keys.mjsPlus the full
lefthookpre-commit and pre-push sequences (format, lint, contract, unknown-keys,changelog, media-fresh, typecheck, typecheck-d, test, exports, build-docs, yakir) — all green.
pnpm -r testpasses across all 30 workspace packages.Scope
Three files: the predicate + its comment, the tests, and the CHANGELOG. §1 (JSON body refuses a
bigint), §3 (empty cross-field path), §4 and §5 of #701 are untouched.
One thing I did not touch and am flagging rather than fixing: the blog line quoted above
(
upload-a-file-with-a-progress-bar.mdx:65) still reads as thoughtypealone is enough to make apart a file. Its own example passes a
File, so the page is not wrong in what it shows, but thesentence is now looser than the code. Left out to keep the diff to the defect.
Refs #701
🤖 Generated with Claude Code