Skip to content

Fix patternProperties parent name inference - #841

Open
terminalchai wants to merge 3 commits into
bcherny:masterfrom
terminalchai:fix/pattern-property-parent-name-161
Open

terminalchai wants to merge 3 commits into
bcherny:masterfrom
terminalchai:fix/pattern-property-parent-name-161

Conversation

@terminalchai

Copy link
Copy Markdown

Summary

  • infer patternProperties documentation owners from the final intersection name or containing property key
  • use neutral wording when no truthful parent name is available
  • add regression coverage and update affected generated snapshots

Fixes #161

Testing

  • bun run test (600 tests passed)

@03musab

03musab commented Aug 28, 2026

Copy link
Copy Markdown

Nice catch and clean fix for issue #161! Fixing the JSDoc parent name inference removes the buggy undefined references in generated TypeScript definitions. LGTM!

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @terminalchai — this fixes a wart that has been in the output since 2018 (#161 is the maintainer's own "why is this undefined?"), and the approach is the one I'd have taken: use the name the reader will actually see, fall back to the property key, and drop the clause rather than print something untrue.

Why CI is red, and the one-line fix. Your head (4cd539a) builds and passes on its own base. CI runs on the merge with master, and #837 landed 11 minutes before this PR was opened; it removed the definitionKeyOf helper from src/parser.ts (a definition's key is now stamped on the schema by the normalizer). So every job stops at Build with:

src/parser.ts(705,79): error TS2552: Cannot find name 'definitionKeyOf'. Did you mean 'DefinitionKey'?

Rebase (or merge master) and change that one call in parseRequired:

-  const parentSchemaName = schemaNameForComment(standaloneName, nameOf(owner, definitionKeyOf(owner), options))
+  const parentSchemaName = schemaNameForComment(standaloneName, nameOf(owner, owner[DefinitionKey], options))

DefinitionKey is already imported there on master. Nothing else conflicts.

Verified locally (bun 1.4.0, your branch + current master 7369533 + that line): bun install --frozen-lockfile && bun run build && CI=true bun run test → 606 pass / 0 fail, lint clean.

  • The #161 case itself (Boris's Procfile / process_types schema from the original report): master prints referenced by `undefined`'s, this branch prints referenced by `process_types`'s. intersection.2 likewise goes from undefined to Intersection.
  • Occurrences of referenced by `undefined` in the e2e snapshot: master 36 → this branch 0.
  • Existing snapshot entries whose output changes: realWorld.openapi, realWorld.schemaStore.1, intersection.2, intersection.3, enum.2, minProperties, required.undeclaredKeys, options.strictIndexSignatures.singlePatternProperty. I diffed all of them against master line by line: 75 changed lines, every one a docblock line — no emitted type changes anywhere. New wordings: the enclosing declaration's name where the object is the body of a named intersection (Parameter, MediaType, Header, HTTPSecurityScheme, Intersection), the property key for inline objects (schemas, responses, … in OpenAPI's Components; pattern, definitions, patternPropertiesAllowed in the smaller fixtures), and the neutral "referenced via the patternProperty …" for the three nameless spots in package.json's exports/typesVersions.

Two small things, neither blocking:

  1. There's no dedicated fixture for the inline-object case; the coverage is the existing snapshots moving, plus the comment added to intersection.2.ts. If you're rebasing anyway, a tiny test/e2e/patternProperties.unnamedParent.ts with the Procfile schema from #161 (object property process_types with one patternProperties entry, additionalProperties: false) would pin the exact case the issue describes. Happy to add it in a follow-up otherwise.
  2. newInterface reads the intersection's name back out of the processed cache (processed.get(schema[Intersection])?.get('ALL_OF')?.standaloneName). That works for every fixture here and degrades to the key / neutral wording if the entry isn't populated yet, so it can't produce a wrong name — but it does lean on parse order, so a one-line comment saying so would help the next reader.

Recommendation for the maintainer: merge once rebased with the one-line port above. Output change is comments only (listed above), it closes #161, and it supersedes the fix we had queued for the same issue.


Generated by Claude Code

@claude

claude Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Quick follow-up, @terminalchai — two things changed since the review above, both small, plus an offer.

1. The rebase now has one textual conflict (not your doing). #844 landed after my review and threads a new scope argument through parseSchema, so git now stops in newInterface. The resolution is your block with scope passed along:

  const intersectionName = schema[Intersection]
    ? processed.get(schema[Intersection])?.get('ALL_OF')?.standaloneName
    : undefined
  const params = parseSchema(
    schema,
    scope,
    options,
    processed,
    usedNames,
    schemaNameForComment(name ?? intersectionName, keyName),
  )

plus the definitionKeyOf(owner) → owner[DefinitionKey] line from before. With those two, your branch on today's master (3ccce84): CI=true bun run test → 608 pass / 0 fail, lint and format clean. The #161 Procfile schema prints referenced by `process_types`'s (master still prints undefined), and the same 8 snapshot entries listed above move — 75 lines, every one a docblock line, no type changes.

2. One more fallback worth catching. I ran the merged build over 46 real-world schemas (Kubernetes, Stripe, GitHub's REST API, a set from SchemaStore): referenced by `undefined` goes from 203 occurrences to 0, and all 441 changed lines are comments. One new wording slipped in, though: patternProperties on unnamed array items prints the parser's internal placeholder key. Docker Compose's compose-spec.json hits it 5 times:

 * This interface was referenced by `{keyNameFromDefinition}Items`'s JSON-Schema definition
 * via the `patternProperty` "^x-".

Minimal input: {"title": "Root", "type": "array", "items": {"type": "object", "patternProperties": {"^x-": {"type": "string"}}}}. master already has a keyOf() helper that skips that placeholder for the same reason, so the same guard in your helper gives the neutral wording there (checked: suite still 608 / 0, Compose output clean):

function schemaNameForComment(standaloneName: string | undefined, keyName: string | undefined): string | undefined {
  return (
    standaloneName ??
    (keyName && keyName !== '[k: string]' && !keyName.includes('{keyNameFromDefinition}') ? keyName : undefined)
  )
}

A one-schema fixture for that (array items + patternProperties → "referenced via the …") next to the Procfile one would pin both fallbacks.

How you'd like to land it is your call. If you have time to rebase with the above, I'll re-check it the same day. If you'd rather not chase master — it has been moving unusually fast this week, and none of that is on you — just say so here and I'll carry your commit unchanged onto a fresh branch (you stay the author, with a Co-authored-by trailer as well), add the two small changes and the fixtures as a separate commit on top, and link it back here so the credit and this review history stay with you. Either way is fine.


Generated by Claude Code

@terminalchai
terminalchai force-pushed the fix/pattern-property-parent-name-161 branch from 4cd539a to e78021f Compare August 29, 2026 19:54
@terminalchai

Copy link
Copy Markdown
Author

Updated the branch with the requested follow-ups:

  • rebased onto 3ccce84
  • passed scope through the parseSchema call and switched to owner[DefinitionKey]
  • documented the processed-cache parse-order fallback
  • filtered {keyNameFromDefinition} placeholders from comment parent names
  • added fixtures for the process_types case and unnamed array items with patternProperties

Verification:

  • bun run build
  • CI=true bun run test -> 612 pass, 0 fail, 329 snapshots

The unnamed array-items fixture now emits the neutral This interface was referenced via the patternProperty... wording without exposing the internal placeholder.

@claude

claude Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Thanks @terminalchai — re-checked e78021f cold from a fresh checkout and everything from both reviews is in:

  • bun install --frozen-lockfile && bun run build && CI=true bun run test → 612 pass / 0 fail / 329 snapshots, lint clean; CI is green on all jobs.
  • Improve schema name inference in generated patternProperties docblock #161's Procfile schema prints referenced by `process_types`'s (master: undefined); unnamed array items print the neutral "referenced via the patternProperty …" with no placeholder. Both new fixtures fail on master and pass here.
  • The 8 existing snapshot entries that move change docblock lines only — token streams are identical, no emitted type changes. referenced by `undefined` goes 36 → 0 in the e2e snapshot and 203 → 0 across a 43-schema real-world corpus, again comments only.

Nothing left to do on your side; this is ready for the maintainer's review.


Generated by Claude Code

@terminalchai
terminalchai force-pushed the fix/pattern-property-parent-name-161 branch from e78021f to d8eed8c Compare October 18, 2026 12:58
@claude

claude Bot commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

Re-checked d8eed8c cold from a fresh checkout. The rebase onto 5caacfc keeps everything from both earlier reviews in place (owner[DefinitionKey], the parse-order comment, the {keyNameFromDefinition} guard, scope passed through), and the one new piece — escaping */ in the parent name, with the parentCommentEscaping fixture — produces a well-formed docblock that transpiles clean.

  • bun install --frozen-lockfile && bun run build && CI=true bun run test → 686 pass / 0 fail / 354 snapshots; bun run lint clean; CI 11/11 green on the head.
  • The three patternProperties.* fixtures fail on master (each receives referenced by `undefined` ) and pass here.
  • Snapshot delta vs master: 3 entries added, 10 changed; every changed entry is type-identical (docblock lines only, 0 type lines moved). referenced by `undefined` in the e2e snapshot: 40 → 0, no {keyNameFromDefinition} leaks.
  • 46-schema real-world corpus, both sides: 454 changed lines, all inside comments; undefined 205 → 0. node test/corpus/run.js --e2e type-checks all 78 outputs with 0 diagnostics.

Nothing further from my side — the merge is the maintainer's call.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve schema name inference in generated patternProperties docblock

2 participants