fix(core): enforce rendererOptions keys in the type system#2939
Conversation
|
We shouldn't be validating this at runtime! It should just be a type system thing. If that's not possible, then let's scrap this PR. |
|
It's possible — reworked in f306fa6 as a pure type-system check; the runtime validation is gone. The pieces were mostly already there:
Callers that don't pin The type-level behavior is tested with vitest's typecheck mode ( 🤖 Generated with Claude Code |
|
Review result: no correctness findings in the type-level implementation. I verified that the PR builds cleanly and that all 78 unit tests plus the new typechecking suite pass. The GitHub Test workflow is also successful. One non-blocking cleanup: |
quicktype(), quicktypeMultiFile(), and quicktypeMultiFileSync() are now
generic over the language name, so passing a literal `lang` narrows
`rendererOptions` to that language's option map: unknown option names
and invalid enum values are compile errors. Callers that don't pin
`lang` to a literal keep the old, unchecked behavior, and there is no
runtime validation.
This also fixes OptionKey, which fell through to the Option class type
for non-enum options, making even valid string-option values like
`{ namespace: "Acme" }` fail to typecheck. Boolean options accept
booleans and their string forms, which is what BooleanOption.getValue
implements.
The type-level behavior is tested with vitest's typecheck mode via
@ts-expect-error assertions in renderer-options.test-d.ts.
The dead renderer options this uncovered in test/languages.ts
(C++ `unions`, TypeScript/Flow `declare-unions`, Zod/Effect
`array-type`, PropTypes `runtime-typecheck`) stay removed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f306fa6 to
e1a1404
Compare
|
Good catch on the symptom, but the message isn't actually dead — Fixed properly in e1a1404 by rebasing onto current master (and squashing out the discarded runtime-validation commit): the branch no longer touches 🤖 Generated with Claude Code |
Fixes #2933.
Unknown
rendererOptionskeys were silently ignored by the quicktype-core API. Per review, this is now enforced entirely in the type system — no runtime validation.What this does
quicktype(),quicktypeMultiFile(), andquicktypeMultiFileSync()generic overLang extends LanguageName(inferred fromlang), and carries the generic throughOptions. With a literallang,rendererOptionsis typed as that language's option map, so unknown option names and invalid enum values are compile errors:Fixes
OptionKey, which fell through to theOptionclass type for non-enum options — even valid usage like{ namespace: "Acme" }failed to typecheck before this. String options are nowstring, boolean optionsboolean | "true" | "false"(matching whatBooleanOption.getValueaccepts), enum options keep their key union.Callers that don't pin
langto a literal — or omit it — keep the old, unchecked behavior, so plain-JS and dynamic callers are unaffected.Testing
test/unit/renderer-options.test-d.tscovers valid options, unknown names, another language's option name, invalid enum values, boolean string forms, and the permissive non-literal fallback, using@ts-expect-errorassertions run by vitest's typecheck mode (newly enabled invitest.config.ts). Verified the harness fails when an expected error doesn't occur.test/languages.ts(C++unions, TypeScript/Flowdeclare-unions, Zod/Effectarray-type, PropTypesruntime-typecheck) stay removed.🤖 Generated with Claude Code