Fix/wrapper alias handling #1876
Merged
Conversation
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
Reveals bug where anonymous recursive types inside wrappers
(e.g., Default<{ self: Root }, {}>) create dangling $ref because
the synthetic name is created by cycle detection but never stored
in definitions due to isWrapperContext check.
The test shows that when an anonymous object type containing a
recursive reference is wrapped in Default/Cell/Stream, the cycle
detection correctly creates AnonymousType_1 and emits a $ref to it,
but the definition is never stored in $defs because line 224 in
schema-generator.ts prevents synthetic names from being used when
isWrapperContext is true.
Reported by automated PR review bot.
…ng references. Root Cause: When a named type (like SchemaRoot) was already in progress or existed in definitions, the code was incorrectly deleting it from the definitionStack at line 188-190. This broke cycle detection for any child types being formatted, because the parent type was no longer on the stack to detect the cycle. The Fix: Removed the incorrect definitionStack.delete() call at lines 188-190. This deletion was logically wrong because: 1. We never pushed to the stack at that point (we return early before line 210's push) 2. If something with that key WAS on the stack, it was from a parent call, and deleting it broke cycle detection Test Updates: - Updated anonymous-recursive-wrapper.expected.json to reflect the correct behavior: anonymous objects that appear only once should be inlined, not hoisted to separate definitions - The test now passes with the schema correctly inlining the anonymous object and using for SchemaRoot's cycle
…eters
Fixes improper type extraction from wrapper alias declarations that contain
generic type parameters instead of concrete types.
When processing wrapper aliases like 'type MyCell<T> = Cell<T>' or nested
aliases like 'Cell<NumberList<number>>' where 'NumberList<T> = T[]', the
code was incorrectly extracting type information from alias declarations
(which contain generic 'T') instead of from usage sites (which have
concrete types like 'string' or 'number').
Key changes:
- Filter out type parameter nodes in wrapper and array formatters by checking
if extracted types have TypeFlags.TypeParameter flag
- Fix formatChildType to properly clear parent typeNode context using
destructuring, preventing mismatched type/node pairs in recursive calls
- Improve Default wrapper node selection to prefer usage-site nodes with
concrete type arguments over alias declaration nodes
- Update wrapper-aliases test fixture to use realistic interface definitions
matching actual CommonTools wrappers instead of simple type erasure
Before: 'Cell<NumberList<number>>' generated 'items: {}' (empty object)
After: 'Cell<NumberList<number>>' generates 'items: { type: number }'
Related to commit 16aceaf which fixed anonymous recursive wrapper refs.
Contributor
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/schema-generator/test/fixtures/schema/anonymous-recursive-wrapper.expected.json">
<violation number="1" location="packages/schema-generator/test/fixtures/schema/anonymous-recursive-wrapper.expected.json:16">
The default value for `field` is `{}`, but this object simultaneously requires a `self` property. That default violates the schema and will yield invalid data for any consumer that applies defaults; please remove or adjust the default so it satisfies the required properties.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| "required": [ | ||
| "self" | ||
| ], | ||
| "default": {} |
Contributor
There was a problem hiding this comment.
The default value for field is {}, but this object simultaneously requires a self property. That default violates the schema and will yield invalid data for any consumer that applies defaults; please remove or adjust the default so it satisfies the required properties.
Prompt for AI agents
Address the following comment on packages/schema-generator/test/fixtures/schema/anonymous-recursive-wrapper.expected.json at line 16:
<comment>The default value for `field` is `{}`, but this object simultaneously requires a `self` property. That default violates the schema and will yield invalid data for any consumer that applies defaults; please remove or adjust the default so it satisfies the required properties.</comment>
<file context>
@@ -0,0 +1,25 @@
+ "required": [
+ "self"
+ ],
+ "default": {}
+ }
+ },
</file context>
✅ Addressed in 69377c0
Contributor
Author
There was a problem hiding this comment.
thanks cubic, fixed this
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.
Fix wrapper type handling for aliases and recursive types
This PR addresses two related issues in wrapper type schema generation that were causing incorrect schema output for CommonTools wrapper types (Cell, Stream, OpaqueRef, Default).
Issue 1: Dangling stack references for anonymous recursive wrapper types
Problem: When processing wrapper types containing anonymous recursive types (like Cell<RecursiveItem[]> where RecursiveItem is recursive), the stack key was being deleted prematurely from definitionStack before child type processing completed. This caused the recursive detection to fail, leading to infinite loops or missing $ref entries.
Example:
type RecursiveItem = { name: string; children?: RecursiveItem[] };
type Config = { items: Cell<RecursiveItem[]> };
Fix: Move stack deletion to occur after child type formatting completes, ensuring the stack correctly tracks active processing and detects recursion.
Issue 2: Generic type parameters extracted instead of concrete types
Problem: When processing wrapper aliases that contain generic type parameters, the code was extracting types from alias declarations (which have generics like T) instead of from usage sites (which have concrete types like string or number).
Example:
type NumberList = T[];
type MyCell = Cell<NumberList>;
// Before: { asCell: true, type: "array", items: {} } ❌ empty items
// After: { asCell: true, type: "array", items: { "type": "number" } } ✓
Fixes:
Impact
Both issues affected schema generation for wrapper types, causing:
Summary by cubic
Fixes schema generation for CommonTools wrappers and aliases so recursive structures emit correct $refs and wrapper inner types are concrete, not empty.
Bug Fixes
Refactors