Conversation
`FormData.values()` and `FormData.entries()` incorrectly typed their return values as `string` instead of `FormDataEntryValue` (string | File), inconsistent with `get()`, `getAll()`, and `forEach()` in the same interface. Closes #27194 Co-Authored-By: Claude <noreply@anthropic.com>
|
Updated 9:49 PM PT - Apr 23rd, 2026
❌ @alii, your commit 190312e has 3 failures in
🧪 To try this PR locally: bunx bun-pr 27195That installs a local version of the PR into your bun-27195 --bun |
|
No actionable comments were generated in the recent review. 🎉 WalkthroughUpdates FormData interface type declarations in the globals definitions to return Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
I'm not confident this doesn't cause conflicts. Will revisit. |
|
@alii I investigated potential conflicts — there shouldn't be any:
Happy to address any specific conflict scenario you have in mind. |
| import { expect, test } from "bun:test"; | ||
|
|
||
| test("FormData.values() returns File objects, not just strings", () => { | ||
| const fd = new FormData(); | ||
| const file = new File(["content"], "test.txt", { type: "text/plain" }); | ||
| fd.append("textField", "hello"); | ||
| fd.append("fileField", file); | ||
|
|
||
| const values = [...fd.values()]; | ||
| expect(values).toHaveLength(2); | ||
| expect(values[0]).toBe("hello"); | ||
| expect(values[1]).toBeInstanceOf(File); | ||
| }); |
There was a problem hiding this comment.
🟡 This runtime test doesn't guard the actual change in this PR — the PR only edits .d.ts declarations, and FormData.values()/entries() already returned File at runtime, so these assertions pass on system Bun before the fix (violating the USE_SYSTEM_BUN=1 validity rule in CLAUDE.md). Per CLAUDE.md, .d.ts changes should be covered by a type-level assertion in test/integration/bun-types/fixture/globals.ts (which already exercises FormData), and since #27194 was never correct in any release it isn't a true regression — so this file shouldn't live under test/regression/issue/ either.
Extended reasoning...
What's wrong
This PR's only functional change is to two type annotations in packages/bun-types/globals.d.ts. The accompanying test at test/regression/issue/27194.test.ts is a runtime test (expect(values[1]).toBeInstanceOf(File)), but the runtime behavior it asserts was already correct before this PR — only the .d.ts types were wrong. Consequently the test does not regression-guard the change: if someone reverted values(): IterableIterator<Bun.FormDataEntryValue> back to IterableIterator<string>, this test would still pass.
Why existing guidance flags this
Two repo conventions apply directly:
- Test validity — CLAUDE.md:130 states: "CRITICAL: Verify your test fails with
USE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>. Your test is NOT VALID if it passes withUSE_SYSTEM_BUN=1." This test passes on system Bun (the C++DOMFormDatahas always yieldedFilefor blob entries), so it provides zero protection for the diff. - Where
.d.tstests belong — CLAUDE.md:37-43 says edits underpackages/bun-types/**/*.d.tsshould be tested viabun test test/integration/bun-types/bun-types.test.ts, which packs the.d.tsfiles and runstscagainst fixtures. That's the only test that would actually fail if the type declarations regressed.
Separately, CLAUDE.md:66 and test/CLAUDE.md:153 reserve test/regression/issue/${issueNumber}.test.ts for true regressions (worked in a prior release, then broke). #27194's types were never correct, so even if a runtime test were kept it would belong in test/js/web/html/FormData.test.ts, not a new regression file.
Step-by-step proof
- Before this PR,
globals.d.tsdeclaresvalues(): IterableIterator<string>, butJSDOMFormData's iterator yields the underlyingFormDataEntryValuevariant (String | Blob). - Run
USE_SYSTEM_BUN=1 bun test test/regression/issue/27194.test.tsagainst a released Bun →values[1]is aFile,toBeInstanceOf(File)passes. ✅ Test green without the fix. - Now apply this PR, then revert just the two
.d.tslines →bun bd test test/regression/issue/27194.test.tsstill passes. ✅ Test green with the fix reverted. - Conclusion: the test cannot detect the absence of the fix in either direction.
Suggested fix
Replace the runtime test with a type-level assertion in test/integration/bun-types/fixture/globals.ts (lines ~221-235 already exercise FormData), e.g.:
import { expectType } from "./utilities";
// ...
const fd = new FormData();
for (const v of fd.values()) expectType<Bun.FormDataEntryValue>(v);
for (const [k, v] of fd.entries()) {
expectType<string>(k);
expectType<Bun.FormDataEntryValue>(v);
}This will fail tsc if the iterator types ever regress to string. If runtime coverage is also desired, add it to the existing test/js/web/html/FormData.test.ts rather than a new test/regression/issue/ file.
| values(): IterableIterator<Bun.FormDataEntryValue>; | ||
| entries(): IterableIterator<[string, Bun.FormDataEntryValue]>; |
There was a problem hiding this comment.
🟣 While you're fixing the iterator return types here, consider also adding [Symbol.iterator](): IterableIterator<[string, Bun.FormDataEntryValue]>; — the runtime aliases Symbol.iterator to entries() (JSDOMFormData.cpp), but bun-types doesn't declare it, so without lib.dom for (const [k, v] of fd) fails to type-check. This is a pre-existing gap, not something this PR introduced, but it's a one-line addition adjacent to the lines you're already touching and falls under the same "FormData iterator return types" concern.
Extended reasoning...
What's missing
The interface FormData at packages/bun-types/globals.d.ts:1664-1685 declares keys(), values(), and entries(), but has no [Symbol.iterator]() declaration. Per the WHATWG spec, FormData is iterable and its default iterator yields the same [string, FormDataEntryValue] pairs as entries(). Bun's runtime implements exactly that — JSDOMFormData::finishCreation does putDirect(vm, vm.propertyNames->iteratorSymbol, getDirect(vm, builtinNames.entriesPublicName())), aliasing @@iterator to the entries function.
How it manifests
Without lib.dom loaded (the default for Bun projects, which use "lib": ["ESNext"]), TypeScript sees only the bun-types FormData interface. Since that interface has no [Symbol.iterator](), code like:
const fd = new FormData();
for (const [k, v] of fd) { /* ... */ }fails with TS2488 ("Type 'FormData' must have a 'Symbol.iterator' method that returns an iterator"), and [...fd] likewise fails. The runtime supports this perfectly fine — only the types are missing.
Why nothing else covers it
A grep across packages/bun-types shows this is the only interface FormData declaration, and Symbol.iterator is declared for CookieMap and sqlite.Statement but not FormData. bun-types depends only on @types/node (which provides no global FormData) and imports undici-types only for EventSource, so there's no other source of a [Symbol.iterator] for FormData when lib.dom is absent. The existing type fixture at test/integration/bun-types/fixture/globals.ts tests entries()/keys()/values() but not for...of, which is why this gap hasn't been caught.
Step-by-step proof
- Project uses
@types/bunwith"lib": ["ESNext"](nolib.dom) — the recommended Bun setup. - TypeScript resolves
FormDatato packages/bun-types/globals.d.ts:1664-1685. - That interface has
entries(): IterableIterator<[string, Bun.FormDataEntryValue]>(after this PR) but no[Symbol.iterator](). - User writes
for (const [name, value] of fd) { ... }. - tsc emits TS2488 because
FormDatais not declared iterable. - At runtime the same code works, since the C++ prototype installs
@@iterator = entries.
Relevance to this PR
This is pre-existing — the PR didn't introduce or worsen it. But the PR's stated goal is to "correct FormData iterator return types to include File", and the default iterator is the most common way people iterate FormData. Fixing values()/entries() while leaving for...of un-typed is an incomplete fix for the same conceptual issue, and the addition is one line right next to the lines being changed.
Suggested fix
entries(): IterableIterator<[string, Bun.FormDataEntryValue]>;
[Symbol.iterator](): IterableIterator<[string, Bun.FormDataEntryValue]>;This matches how lib.dom.iterable.d.ts declares it, so when lib.dom is loaded the merged interface remains consistent (both declare the same signature shape). Not a blocker — just a suggestion for completeness while you're in this exact spot.
|
Superseded by #36505, which batches this fix with six other |
Summary
FormData.values()return type fromIterableIterator<string>toIterableIterator<Bun.FormDataEntryValue>FormData.entries()return type fromIterableIterator<[string, string]>toIterableIterator<[string, Bun.FormDataEntryValue]>These iterator types were inconsistent with
get(),getAll(), andforEach()in the same interface, which already correctly usedBun.FormDataEntryValue(string | File).Closes #27194
Test plan
bun bd test test/regression/issue/27194.test.tspasses — validates thatvalues()andentries()returnFileinstances at runtime🤖 Generated with Claude Code