fix: resolve Data type names from the SDK signature for digit-leading operationIds - #214
Merged
Conversation
… 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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
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.
Type-checking the generated output against the real TanStack Query types exceeds vitest's default 5s timeout on CI runners.
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.
Closes #213
Problem
When an
operationIdstarts with a digit (e.g.123numericLead), hey-api names the SDK function and itsDatatype differently:_123NumericLead(prefixed — identifiers cannot start with a digit)NumericLeadData(digits dropped)This codegen derived the
Datatype name from the method name (${capitalizedMethodName}Data→_123NumericLeadData), which does not exist. Two symptoms followed:getPaginatableMethodskeyed its map by a name derived from theDatatype, so the lookup missed and the operation got no infinite hooks despite a valid page parameter.unknownfallback emittedOptions<unknown, true>, producing 6 ×TS2344('unknown' does not satisfy the constraint 'TDataShape').Fix
Read the
Datatype name from the SDK function's own signature — the first type argument ofOptions<XData, ThrowOnError>insdk.gen.tsis the authoritative answer and is already in the AST thatparseOperations.mtswalks:parseOperationsextractsdataTypeNamefrom the signature and carries it onOperationInfo.getPaginatableMethodsnow keys its map by theDatatype name itself, so the pagination lookup can never diverge from the resolved type.buildQueryHooks,buildMutationHooks,buildCommon,buildQueryOptions) consumeop.dataTypeNamevia a singlegetDataTypeName(op)helper instead of re-deriving the name. Builders whosectxparameter became unused (buildQueryKeyFn,buildQueryOptionsFn,buildPrefetchFn,buildEnsureQueryDataFn) had it removed.The
unknownfallback now only triggers when the SDK signature exposes noDatatype at all.Tests
tests/inputs/digit-leading.yamlfixture generating the real hey-api output (_123NumericLead/NumericLeadData), asserting the divergence is resolved and pagination is detected.Options<unknown, true>assertions that pinned the broken output are rewritten: the fallback is now pinned to the no-Data-type-in-signature case, and new digit-leading cases assert the correct type is emitted.op.dataTypeName.Verified end-to-end with the repro from the issue: the generated output contains
use_123NumericLeadInfinitecasting toOptions<NumericLeadData, true>, andtsc --noEmitover the generated files exits 0.Follow-up: review pass (b2b2356, b053bf1)
A multi-angle cleanup review over the diff surfaced one sibling defect and several simplifications, applied in two follow-up commits:
getErrorTypederived${capitalizedMethodName}Error, so digit-leading operationIds silently degradedTErrortounknown. The stem now comes fromdataTypeName(hey-api mints both names from the same stem — pinned by a new fixture assertion).getDataTypeNameand the previously duplicatedgetErrorTypenow live in a neutralsrc/tsmorph/operationNames.mts, removing thebuildCommon → buildQueryHooksimport edge.getDataTypeNameFromSignaturetakes the already-computed options parameter instead of re-walking the arrow function; an inertgetShortTypecall was dropped.OperationInfo.dataTypeName; duplicated comment blocks and test assertions made inert by the refactor were removed.createSourcepipeline and the generated output is compiled with zero TypeScript diagnostics.