Refactored the generator into explicit abstractions and typed the JavaScript surface - #7
Conversation
pkurcx
left a comment
pkurcx
left a comment
There was a problem hiding this comment.
Thanks, this is a careful refactor and the structure is a clear improvement. I built both this head and its base commit (1f1944e) and ran every fixture in test/fixtures through both CLIs: generated files, stdout, stderr and exit codes are identical for all 73, except the disclosed duplicate-schema-name wording. That covers the ten newly pinned fixtures too, which the new snapshots alone cannot prove. Also confirmed: fmt/clippy/cargo test clean (305), typecheck/lint/coverage/ava clean (345), and all ten commits compile standalone.
Two things need fixing before merge, and one rebase hazard:
- Runtime enums are declared but not exported.
index.d.tsnow promisesInputFormatandResponseTypeas runtime constants;lib/index.jsandlib/browser.jsexport onlygenerate,GenerateError,EmitTarget.InputFormat.Yaml(shown in the node-api docs) type-checks and throws at runtime. Under the oldconst enumit was inlined and worked. Inline comment below. - Inline-object depth is charged twice per level in
schema/mod.rs, roughly halving the nesting cap. Inline comment below. - Rebase onto main.
mainhas moved (v0.6.0, #8): 20 files conflict. #8 also added aLayoutconst-enum rewrite toscripts/patch-types.mjs, which this PR deletes, so that rewrite has to be ported intopatch-types.tsor it silently disappears. The ten new snapshot dirs need regenerating under the newmodel.ts/rest/<tag>.rest.tsnames.
Smaller behaviour changes worth listing in the description, since they are not covered by the byte-identical claim:
- JSON inputs with a repeated key were last-wins on the base and now fail with
E_INPUT_INVALID(no subcode, so a duplicate schema name in JSON never getsduplicate-schema-name). - A repeated key under
pathschanges code fromE_POLICY_VIOLATIONtoE_INPUT_INVALID, not only its message. default_groupnow returnsDefaultfortags: [""]where the base fell through to the path segment.- Re-export lines (
export type { A as X, ... } from) now wrap at 100 columns.
Non-blocking: a few why-comments dropped in e60917d are worth restoring (the Params-suffix collision rationale in plan/naming/fixed.rs, the verbatim-brace-instead-of-panic note in emit/angular/request.rs, the alias-collision note in emit/model/emit_ts_models.rs). The Emitter trait has one impl per target and no second implementation planned, so a plain function would carry less indirection; fine to keep if you prefer the shape.
| Yaml = 'yaml' | ||
| } | ||
| export type InputFormat = 'json' | 'yaml'; | ||
| export declare const InputFormat: { |
There was a problem hiding this comment.
This declares a runtime value, but lib/index.js and lib/browser.js do not export InputFormat (or ResponseType). Verified on this head:
node -e "console.log(require('./lib/index.js').InputFormat)" # undefined
A consumer writing inputFormat: InputFormat.Yaml, as website/src/content/docs/reference/node-api.md shows, compiles under strict + isolatedModules and throws at runtime. The old const enum was inlined to 'yaml', so this is a regression rather than the strictly-more-permissive change the description states.
Fix: export frozen mirrors the way EmitTarget already is, re-export them from browser.d.ts, and widen the allow-list in __test__/package.spec.ts (public surface is a fixed allow-list), which currently pins the export set and would have caught this if the enums had been added there.
| )?))) | ||
| } | ||
| Some("object") => Ok(SchemaType::InlineObject { | ||
| properties: normalize_properties(schema, walk.item())?, |
There was a problem hiding this comment.
Depth is now charged twice per inline-object level: walk.item() here, then walk.property(name) inside normalize_properties before normalize_type runs check_depth. The base charged one unit (normalize_properties(.., depth + 1, ..) and then normalize_schema_raw(.., depth, ..) per property), so the effective cap for nested inline objects roughly halves. Arrays, compositions and maps are unaffected. The existing depth test uses arrays only, so it does not catch this. Passing walk instead of walk.item() restores the old accounting and matches walk.rs's contract that each constructor descends exactly one level.
| /// measured by re-serialising the node tree. A source this cannot parse | ||
| /// passes, leaving the typed parse to report the error. | ||
| fn check_anchor_expansion(source: &str, display_path: &Rc<str>) -> Result<(), Diagnostic> { | ||
| let Ok(value) = serde_yml::from_str::<serde_yml::Value>(source) else { |
There was a problem hiding this comment.
Returning Ok when the Value parse fails opens a gap: serde_yml 0.0.12's Value rejects duplicate mapping keys, while the typed parse accepts them last-wins inside the serde_json::Value-typed fields (example, enum). A document with one repeated key inside example plus a fanned-out anchor skips this guard and decodes; the base rejected it (mislabelled, but fatal). serde_yml's own repetition limit still stops large bombs, so the impact is bounded, but the guard is no longer total. Options: fail on a duplicate-key Value error, or fall back to a cheap alias-count check when the Value parse fails.
| /// under `components.schemas` carries the `duplicate-schema-name` subcode so | ||
| /// consumers can route on it; anything else is a plain decode failure. | ||
| fn decode_failure(message: &str, display_path: &Rc<str>) -> Diagnostic { | ||
| if message.contains(DUPLICATE_KEY) && message.contains(SCHEMAS_FIELD_PATH) { |
There was a problem hiding this comment.
contains("components.schemas") also matches nested paths, so components.schemas.Pet.properties: duplicate key 'id' and a repeated discriminator.mapping key are still reported as duplicate-schema-name with "Each schema name must be declared once." Same behaviour as the base, but the description presents the misrouting as fixed. Matching the prefix components.schemas: would make it exact.
Related: for JSON input the same defect surfaces as plain E_INPUT_INVALID with no subcode (serde_json carries no field path), so the subcode is format-dependent. Worth a line in the description at least.
| # duplicate-mapping-key rejection, which the duplicate-schema-name and | ||
| # mapping-expansion-exceeded diagnostics both read out of serde_yml errors. | ||
| # Pinned: 0.0.13 swapped its YAML backend and dropped the line/column suffix | ||
| # from decode errors, which every `E_INPUT_INVALID` message forwards verbatim. |
There was a problem hiding this comment.
Checked 0.0.13: it drops the line/column suffix and also the field.path: prefix on decode errors. decode_failure above depends on that prefix to route duplicate-schema-name, so the pin has two reasons now; worth naming both here.
| .tags() | ||
| .first() | ||
| .map(String::as_str) | ||
| .or_else(|| ctx.lookup_indexed("pathSegments", 0)) |
There was a problem hiding this comment.
or_else only fires when tags() is empty, so tags: [""] yields Default here where the base fell through to the path segment (Pets for /pets). Tags are copied unfiltered from the spec. Low frequency, but a behaviour change with no test; filtering the tag before or_else restores the old result.
| const first = await generate({ inputPath: fixture(name), emit: [...DEFAULT_EMIT] }); | ||
| const second = await generate({ inputPath: fixture(name), emit: [...DEFAULT_EMIT] }); | ||
| const third = await generate({ inputPath: fixture(name), emit: [...DEFAULT_EMIT] }); | ||
| const [first, second, third] = await Promise.all( |
There was a problem hiding this comment.
Running the three generations concurrently weakens what this test checks: state leaking from run 1 into run 2 is only observable when they run in sequence, and a separate concurrency test already exists further down. Suggest keeping these sequential.
| const binding = await load(); | ||
| const [prepared, binding] = await Promise.all([ | ||
| prepareOptions(options, unreachableFetch), | ||
| load(), |
There was a problem hiding this comment.
prepareOptions does no I/O in the browser (URL inputs are rejected earlier), so Promise.all gains nothing here, and it changes behaviour slightly: invalid options now trigger the wasm download before failing, and if both reject the first to settle wins. Sequential was simpler and had the stricter ordering. Non-blocking.
| [ | ||
| EMIT_TARGET_UNION, | ||
| 'export declare const EmitTarget: {', | ||
| " readonly Models: 'models';", |
There was a problem hiding this comment.
Heads-up for the rebase: main (#8) added a Layout const-enum rewrite to scripts/patch-types.mjs, which this branch deletes. Git will resolve that as a modify/delete conflict and the rewrite is gone unless it is ported here as a fourth pattern.
2 participants
Summary
The generator was correct but hard to change safely: there were no
abstractions to change through. No traits; diagnostics threaded as
&mutthrough 22 signatures, which made
collect::<Result<..>>unavailableanywhere; schema depth counted by convention at each recursive call site;
four files past 400 lines of code.
Two checks were also wrong. A repeated mapping key under
pathsreported asa duplicate schema name. The fixture list
regen-snapshotsread haddrifted from
test/fixtures/, leaving nine fixtures never snapshotted, andthe reader kept a second copy of that list which already disagreed.
Nothing type-checked the 2064 lines of published JavaScript.
Verified
Output is byte-identical.
paths, exit codes — diffed against the base commit.
templates/,browser.d.tsandnative.jsuntouched.index.d.tschanges only in doc comments and in the const-enum rewrites below; its
156 declarations are unchanged.
clippy --all-targetsclean.src/is 50 lines, from 118; 28 functions over 45lines, now 5.
Release builds of this branch and of
main, same machine, the sameharness against each binding through
NAPI_RS_NATIVE_LIBRARY_PATH, threealternating rounds of 40 iterations, median of the per-round medians. Both
bindings were checked to emit identical artifacts first.
JSON is the control and barely moves: the win is the YAML double parse,
which JSON never had. None of these fixtures declare an anchor, so the
re-serialisation the expansion guard needs is skipped outright.
The
mainside is the noisy one — bench-multi-tag measured 9.2 / 10.8 /10.6 ms across rounds against 4.8 / 4.6 / 4.7 for this branch, so read that
row's 2.23x with less confidence than bench-large, where both sides are
stable.
Behaviour changes
field path and source position, and each position reports as itself.
Nothing pinned the old wording.
InputFormatandResponseTypestop beingconst enum, which a consumerunder
isolatedModulescould not import.patch-typesalready did thisfor
EmitTarget. Strictly more permissive.build:debugnow runspostbuild. It did not, so a debug build leftindex.d.tsunpatched and failedpackage.spec.ts.Reviewing
Three commits move code between files, so
git blameoversrc/isdisrupted;
--followor-Mreads it. The nine new snapshots are committeddata nobody has read — each fixture carries a one-line
pinsnote sayingwhat a failing diff means.