fix(jsclient): the generated TypeScript client did not compile - #11
Merged
Merged
Conversation
`export_ts_client` emitted a file `tsc` rejects outright, while the existing
tests passed. They asserted on substrings and on a balanced brace count, and
two of the three defects balanced each other out.
- `headers: {{ 'Content-Type': 'application/json' }}` -- a doubled brace left
over from `.format`-style escaping, and a syntax error in TypeScript. The
JavaScript emitter has it right; only the TS copy was wrong.
- A stray `}` before every method whose endpoint takes no parameters. The
assembler recovered the method from the combined interface+method string by
splitting on the first blank line, but a zero-parameter interface is
`export interface FooParams {\n\n}` -- its blank line comes first, so the
split returned the interface's own closing brace plus the method. The client
class closed early and every later method landed at module scope.
`generate_ts_method` now returns the method alone; `generate_ts_function`
keeps its old combined return for compatibility.
- Every optional parameter emitted as required. A function with six defaults
produced a method taking seven mandatory arguments.
Also, in `get_python_type_name`: a parameterised generic lost its arguments
whenever the annotation carried a `__name__`. On 3.10+ `typing` generics do,
so `Optional[str]` and `Union[int, str]` became the bare words "Optional" and
"Union" and the client typed them `any`, while the PEP 604 spelling
`str | None` -- which has no `__name__` -- kept its arguments. Two spellings of
one type produced two different clients. Arguments are now kept and every
union normalises to `Union[...]`. Only `extract_function_signature` reads this
function, so the blast radius is the client generators.
TS type mapping now covers unions, and sequence/mapping generics by origin
name rather than a case per spelling: `str | Sequence[str] | None` reaches
TypeScript as `string | string[] | null` instead of `any`.
Two existing tests asserted the old behaviour -- `greet(name: string, title:`
pinned the required-optional-parameter defect, and `'Optional' in type` pinned
the argument loss. Both are updated in place with the reason stated, not
deleted.
New `test_jsclient_compiles.py` runs `tsc --strict` over the generated client
and skips when npx is unavailable, with structural fallbacks that would each
have caught one of the three defects offline.
This was referenced Sep 22, 2026
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.
Found while building
ductus's HTTP surface (Phase 4), which usesqh.mk_appover the same function list its CLI and MCP surfaces dispatch from, andqh.export_ts_clientfor the frontend's typed client.The endpoint layer is fine
First, the good news, because it was the specific thing being checked for.
ductusis keyword-only from the 2nd/3rd argument throughout and every module usesfrom __future__ import annotations. Under those conditions the schema layer beneathfastmcpdrops every keyword-only default and leaves each verb uncallable while the JSON schema looks perfect (i2mint/py2mcp#12).qhdoes not have that bug. Driven through a real client, every verb is callable with only its required arguments, and the OpenAPIrequiredlist is correct. Nothing was changed there.The TypeScript emitter, however, produced a file
tscrejectsThree defects, all of which the existing tests missed because they assert on substrings and on
code.count("{") == code.count("}")— and two of them balance each other out.headers: {{ 'Content-Type': 'application/json' }}— a doubled brace left over from.format-style escaping, inside a plain (non-f) string. A syntax error in TypeScript. The JavaScript emitter has the same line written correctly; only the TS copy was wrong.A stray
}before every method whose endpoint takes no parameters.export_ts_clientrecovered the method fromgenerate_ts_function's combinedinterface + "\n" + methodstring by splitting on the first blank line. For a zero-parameter function the interface isexport interface FooParams {\n\n}— the blank line is inside it, so the split handed back that interface's own closing brace followed by the method. The client class closed early and every method after the first no-argument endpoint landed at module scope.Every optional parameter emitted as required. The interface generator respected
required; the method generator ignored it. A function with six defaults produced a method taking seven mandatory arguments — and since TypeScript forbids a required parameter after an optional one, marking them naively would not have compiled either. Optional parameters are now marked?and sorted after the required ones (stable, so declaration order is kept within each group).And a type-name defect underneath
get_python_type_namechecked__name__before handling parameterised generics. On Python 3.10+typinggenerics carry a__name__, soOptional[str]andUnion[int, str]returned the bare words"Optional"and"Union"with every argument discarded — and the client typed themany. The PEP 604 spellingstr | Nonehas no__name__, so it fell through to the origin branch and kept its arguments. Two spellings of the same type generated two different clients.Arguments are now kept, and every union normalises to
Union[...]so the spelling cannot matter. Onlyextract_function_signaturereads this function, so the blast radius is exactly the client generators.TS type mapping now covers unions, and sequence/mapping generics by origin name rather than a case per spelling.
str | Sequence[str] | Nonereaches TypeScript asstring | string[] | nullrather thanany.Two existing tests asserted the old behaviour
Flagging rather than burying these, since they were green before:
test_ts_client_optional_paramsasserted"greet(name: string, title:" in ts_code— which pinned defect 3. Now assertsgreet(name: string, title?: string | null).test_optional_parameters_in_signatureasserted'Optional' in title_param['type']— which pinned the argument loss. Now asserts the normalisedUnion[str, NoneType], plus that the PEP 604 spelling of the same type produces the same string.Both were updated in place with the reason written into the test, not deleted.
Testing
New
qh/tests/test_jsclient_compiles.pyruns the real compiler —npx tsc --noEmit --strictover the generated client — and skips cleanly when npx or the network is unavailable. Behind it sit structural assertions that would each have caught one of the three defects offline: no{{, every method inside the class body (found by brace-matching fromexport class, not by counting), and no required parameter after an optional one.A brace count cannot see any of this. The stray
}and the doubled{{/}}cancel.Full suite: 173 passed.
Not fixed
The JavaScript emitter's GET path builds
new URLSearchParams({...})directly, so an omitted optional parameter travels as the literal six charactersundefined. The TS emitter now filters those out via an emitted_defined()helper; the JS one still has the issue. Untouched because nothing here needed it and it deserves its own change.