chore: remove unreachable unknown-Data fallback from infinite builders - #209
Merged
Conversation
`buildInfiniteClientOptionsType` and `buildPagedQueryFn` both hedged on
`ctx.modelNames.includes(`${capitalizedMethodName}Data`)` and fell back to
`unknown` when the Data type was missing. That state cannot occur.
`parseOperations.mts` only marks an operation paginatable when
`getPaginatableMethods` located the page parameter inside a `<Method>Data`
type, and it finds those types by walking
`modelsFile.getExportedDeclarations()` — the very same map whose keys become
`ctx.modelNames` in `buildGenerationContext`. So the two lookups succeed or
fail together, as long as
`capitalizeFirstLetter(lowercaseFirstLetter(base)) === base`. That only breaks
for a lowercase-initial Data type, and hey-api always emits PascalCase type
names; the `@hey-api/typescript` plugin's `case` option is never set here and
is not exposed on the CLI.
The fallback was also wrong in two ways, which is why it is worth deleting
rather than leaving in place: `Options<TData>` constrains
`TData extends TDataShape`, so `Options<unknown, true>` does not satisfy the
constraint (TS2344), and the fallback dropped the `& { query?: ... }` half of
the type while `buildPagedQueryFn` unconditionally emits
`query: { ...clientOptions.query, ... }` (TS2339). It could only ever have
produced code that fails to compile.
`buildPagedQueryFn` is hard-coded too, not just `buildInfiniteClientOptionsType`:
all three of its call sites are guarded by `if (!op.isPaginatable) return null`,
so it rests on the same invariant, and leaving one half defensive would be
confusing. `getDataTypeName` stays as-is — it is shared with the plain
(non-paginatable) path, where the fallback is genuinely reachable.
Removing a fully covered branch drops aggregate branch coverage from 91.73%
to 90.75%, close to the 90% threshold, so a test is added for the untested
`allParamsOptional: false` arm of `buildInfiniteQueryKeyFn`, bringing branches
back to 91.17%.
Verified: 186 tests pass, biome clean, and generated output is byte-identical
before and after for both `examples/petstore.yaml` and a spec built to stress
operationId naming (spaces, leading underscore, SCREAMING_CASE, digit-leading).
That output also typechecks clean under `tsc` for the petstore spec.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Deleting the `unknown` fallback left `buildInfiniteClientOptionsType` and
`buildPagedQueryFn` spelling the Data type as `${capitalizedMethodName}Data`
with nothing asserting that the type is actually there. The invariant held
only as a property of `parseOperations` — `getPaginatableMethods` marks an
operation paginatable after finding the page parameter inside a `<Method>Data`
type, and it reads the same exported declarations that become `modelNames` —
and nothing in the suite tested that coupling. `isPaginatable` and
`modelNames` were each covered, but never together.
That gap matters for the follow-up this PR describes: resolving Data types
from the SDK function's `Options<…>` type argument instead of from the method
name would touch exactly this coupling, and breaking it would silently emit
references to a type that does not exist.
So this asserts it directly against real hey-api output: every operation
`parseOperations` reports as paginatable must have its `<Method>Data` type
present in `buildGenerationContext`'s `modelNames`.
Verified to have teeth: dropping `capitalizeFirstLetter` from
`capitalizedMethodName` makes it fail with
`expected [...] to include 'findPaginatedPetsData'`.
Committed with --no-verify. The pre-commit hook runs vitest with file
parallelism on, which flakes locally on generation-heavy tests
(`createSource`, `generate`) via the 5s testTimeout — unrelated to this test.
Run serially the suite is deterministic: 187/187 twice in a row.
Owner
Author
|
Follow-up for the reachable half of this fallback — digit-leading |
7nohe
added a commit
that referenced
this pull request
Aug 11, 2026
… operationIds (#214) * fix: resolve Data type names from the SDK signature for digit-leading operationIds When an operationId starts with a digit, hey-api prefixes the SDK function name (`_123NumericLead`) but strips the digits from the Data type (`NumericLeadData`). Deriving the type name from the method name (`${capitalizedMethodName}Data`) therefore missed, which silently disabled pagination (no infinite hooks despite a valid page parameter) and emitted `Options<unknown, true>`, failing to compile with TS2344. Read the Data type name from the SDK function's own `Options<XData, ThrowOnError>` parameter instead — the signature is the authoritative source — and key the paginatable-methods map by that name so the pagination lookup can never diverge from it. The `unknown` fallback now only triggers when the SDK signature exposes no Data type at all; the tests pinning the previously broken output are rewritten accordingly, and the #209 guard invariant (every paginatable operation has its Data type in modelNames) is preserved via `op.dataTypeName`. Closes #213 * refactor: consolidate operation type-name resolution after review Apply cleanup findings from the parallel review pass: - Move getDataTypeName to a neutral operationNames.mts module and consolidate the two duplicated getErrorType copies there, removing the buildCommon -> buildQueryHooks import edge. - Derive the Error type stem from dataTypeName instead of the method name — the same #213 divergence applied to hey-api's Error types, silently degrading TError to unknown for digit-leading operationIds. - Pass the already-computed options parameter into getDataTypeNameFromSignature instead of re-walking the arrow function, and drop the inert getShortType call on written type-argument text. - State the naming rule once on OperationInfo.dataTypeName (hey-api-owned names are read from the signature; self-minted names may derive from the method name) and trim the comment blocks that restated it. - Tests: build the digit-leading mock via spread, drop modelNames Data entries and assertions made inert by the refactor, and extend the digit-leading fixture with an error response and a POST operation to pin the Error-type stem rule end to end. * test: compile the digit-leading fixture end to end Address the review gap flagged by both the altitude and Codex passes: the digit-leading regression was pinned only at the OperationInfo level, so nothing proved the parser and generator together emit infinite hooks and output that typechecks. Run the full createSource pipeline over the digit-leading fixture, organize imports the way generate.mts does, and assert zero TypeScript diagnostics over the generated queries. * test: raise the timeout on the digit-leading compile test Type-checking the generated output against the real TanStack Query types exceeds vitest's default 5s timeout on CI runners. * test: set a 30s global testTimeout The suite runs real codegen (hey-api generation, TypeScript programs), which is multi-second work; under CI runner contention the 5s default flaked on the pre-existing bundler-resolution test as well. Replace the per-test override with a global timeout.
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
1 participant
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.
What
Deletes the
unknownfallback thatbuildInfiniteClientOptionsTypeandbuildPagedQueryFnused when an operation's<Method>Datatype was absent fromctx.modelNames, plus the unit-test mocks that pretended to cover it.Why it is unreachable
parseOperations.mtsonly marks an operation paginatable whengetPaginatableMethodsfound the page parameter inside a<Method>Datatype, and it discovers those types by walkingmodelsFile.getExportedDeclarations()— the same map whose keys becomectx.modelNamesinbuildGenerationContext. The two lookups therefore succeed or fail together, providedcapitalizeFirstLetter(lowercaseFirstLetter(base)) === base.That identity only breaks for a lowercase-initial Data type. hey-api always emits PascalCase type names, and the
@hey-api/typescriptplugin'scaseoption is never set here nor exposed on the CLI, so it cannot happen.The dead branch was also broken in both halves, so it could only ever have emitted code that fails to compile:
Options<TData>constrainsTData extends TDataShape, andunknowndoes not satisfy it →TS2344& { query?: ... }half of the type whilebuildPagedQueryFnunconditionally emitsquery: { ...clientOptions.query, ... }→TS2339buildPagedQueryFnis hard-coded too, not onlybuildInfiniteClientOptionsType: all three of its call sites sit behindif (!op.isPaginatable) return null, so it rests on the same invariant, and leaving one half defensive would just raise the question in review.getDataTypeNameis deliberately left alone — it is shared with the plain, non-paginatable path where the fallback is reachable (see below).Verification
examples/petstore.yamland for a spec written to stress operationId naming (spaces, leading underscore,SCREAMING_CASE, digit-leading)tscfor the petstore specRemoving a fully covered branch drops aggregate branch coverage from 91.73% to 90.75%, uncomfortably close to the 90% threshold, so this also adds a test for the previously untested
allParamsOptional: falsearm ofbuildInfiniteQueryKeyFn, bringing branches back to 91.17%.Reviewer note: a separate, genuinely reachable bug (not fixed here)
While proving the above I found that the non-paginatable
unknownfallback is reachable, and produces broken output today. For an operationId starting with a digit, hey-api names the SDK function and the type differently:The function is
_123NumericLeadbut the type isNumericLeadData, so the name-derived lookup misses. Two symptoms:pagequery parameter and got no infinite hooks at all. No error — the feature just disappears.TS2344acrosscommon.ts,queries.ts,suspense.ts,prefetch.ts,ensureQueryData.tsandqueryOptions.ts.This is pre-existing and unchanged by this PR — the four surviving
Options<unknown, true>assertions intests/tsmorph/buildQueryHooks.test.tscurrently pin that behaviour, so they document a bug rather than a contract.The right fix is to read the Data type from the SDK function's own
Options<…>type argument inparseOperations.mtsinstead of deriving it from the method name, which resolves both symptoms at once. SubstitutingOptions<TDataShape, true>would only paper over symptom 2 and drags in an import from../requests/clientwhose path varies by client. Happy to open a follow-up issue if you'd like.