fix(js): export all-objects converters from module.exports#3009
Merged
Conversation
With --converters all-objects, the JavaScript renderer generates toX/xToJson functions for every object type, but module.exports only listed the top-level converters, making the extra converters unreachable by callers of the module. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
schani
commented
Jul 20, 2026
The issue-1655 fixture driver decided whether to exercise the nested data123
converter by matching the generated source for the exact string
"function toData123(". Replace that brittle hard-coded gate with a check of
the invariant #1655 actually establishes: every converter the generated
module defines (`function to<Name>(json)` / `function <name>ToJson(value)`)
must also be reachable through module.exports. The module's helper functions
have different signatures and are not matched, so the check is general across
samples and converter modes rather than tied to one sample name.
The same sample is rendered with several converter options and the driver
only receives the sample path, so the set of emitted converters is read from
the generated module; in the default (top-level) rendering data123 is simply
never defined and never checked.
Co-Authored-By: Claude <noreply@anthropic.com>
schani
force-pushed
the
agent/fix-issue-1655
branch
from
July 21, 2026 15:46
9cddc5d to
37d8d92
Compare
The fixture driver read the generated TopLevel.js back and grepped its source for converter definitions to check they were all exported. That inspection of emitted source belongs in a render-and-inspect unit test, not in the round-trip fixture driver. Move the #1655 regression check into test/unit: render the JS module with converters: all-objects and assert every defined converter appears in the module.exports block (plus a top-level control that shows the nested converter is neither generated nor exported in the default mode). This mirrors the existing java-acronym-names unit test, whose comment explains the same rationale — a round-trip fixture cannot observe the defect because the top-level converter it exercises works regardless. Reverting the renderer fix makes the new test fail. The javascript fixture driver returns to a plain round-trip; issue-1655.json still renders with converters: all-objects, keeping fixture coverage that the mode generates runnable code. Co-Authored-By: Claude <noreply@anthropic.com>
schani
commented
Jul 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.
Bug
With
--converters all-objects, the JavaScript renderer generatestoX/xToJsonconverter functions for every object type in the schema, notjust the top-level types. However,
module.exportsonly ever listed thetop-level converters, so the extra per-object converters were generated but
inaccessible/unusable to callers of the module.
Repro:
produced
toData123/data123ToJsonfunctions in the file body, butmodule.exportsonly contained the top-leveltoOutput/outputToJson.Root cause
In
packages/quicktype-core/src/language/JavaScript/JavaScriptRenderer.ts,emitConvertModuleBody()already branches onthis._jsOptions.converters === ConvertersOptions.AllObjectsto decidewhether to iterate
forEachObjectorforEachTopLevelwhen emitting theconverter functions themselves.
emitModuleExports()did not have the samebranch — it unconditionally used
forEachTopLevel, so the export list nevermatched what was actually generated when
all-objectswas selected.Fix
emitModuleExports()now mirrorsemitConvertModuleBody()'s switch onthis._jsOptions.converters, usingforEachObjectforConvertersOptions.AllObjectsandforEachTopLevelotherwise (unchangeddefault behavior).
Test coverage
test/inputs/json/samples/issue-1655.json, a sample with a nestedobject type, wired into
test/languages.ts'sJavaScriptLanguage.quickTestRendererOptionswith{ converters: "all-objects" }(same tuple pattern already used for othertargeted regression samples like
pokedex.json).test/fixtures/javascript/main.jsto additionally call thenested object's exported converter (
toData123/data123ToJson) when thegenerated source defines it, so the fixture driver actually exercises the
export and fails with a runtime
TypeErrorif it's missing.fixture fail (
TypeError: TopLevel.toData123 is not a function), and thatwith the fix in place it passes.
Verification
npm run buildpasses.QUICKTEST=true FIXTURE=javascript script/test— all 59 JavaScriptfixture tests pass, including the new
issue-1655.jsoncase run withconverters: all-objects.module.exportsnow includesdata123ToJson/toData123alongside thetop-level converters.
Fixes #1655
🤖 Generated with Claude Code