feat(ts-morph-codegen): Implement interface processing order#7
feat(ts-morph-codegen): Implement interface processing order#7
Conversation
This change introduces a new utility function `getInterfaceProcessingOrder` that determines the order in which interfaces should be processed. This is necessary to ensure that interfaces that depend on other interfaces are processed in the correct order. The changes include: - Importing the `getInterfaceProcessingOrder` function from the `@daxserver/validation-schema-codegen/utils/interface-processing-order` module. - Updating the code that processes interfaces to use the new function to determine the processing order. - Refactoring the interface processing loop to use `forEach` instead of a `for` loop. These changes improve the reliability and maintainability of the code generation process by ensuring that interfaces are processed in the correct order.
📝 WalkthroughSummary by CodeRabbit
WalkthroughImplements interface inheritance support using TypeBox.Composite and dependency-ordered interface processing. Adds a utility to compute interface processing order, updates codegen to use it, modifies the interface handler to build composites from extended interfaces, updates tests accordingly, and adjusts documentation and devDependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant G as Codegen
participant U as getInterfaceProcessingOrder
participant P as InterfaceParser
participant H as InterfaceTypeHandler
G->>U: compute order for sourceFile.getInterfaces()
U-->>G: ordered interfaces (bases before derived)
loop for each Interface in order
G->>P: parse(i)
P->>H: handle InterfaceDeclaration
alt Interface has extends
H->>H: collect base identifiers
H-->>P: Type.Composite([...bases, Type.Object({...})])
else
H-->>P: Type.Object({...})
end
P-->>G: emit schema + Static alias
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
src/handlers/typebox/object/interface-type-handler.ts (1)
20-28: Cross-file inheritance may produce unresolved identifiers.This handler assumes the base interface’s runtime schema identifier (e.g.,
Base) is already available in the output file. Currently, only type aliases are imported/ordered across files; interfaces are processed per-source-file. If a local interface extends an imported interface, you’ll emitType.Composite([ImportedBase, ...])without importingImportedBase.Would you like me to sketch an interface dependency collector akin to
DependencyCollectorfor type aliases so imported interface schemas are auto-imported?src/utils/interface-processing-order.ts (1)
48-54: DFS post-order is appropriate; consider test coverage for out-of-order declarations.The traversal will correctly order bases before derived types, including simple cycles. Add tests where
Derivedappears beforeBasein source to lock this in.I can add a test case demonstrating out-of-order declarations and one for
extends ns.Baseto validate the normalization logic above.src/ts-morph-codegen.ts (3)
71-75: forEach vs for-of: style/readability nit.forEach closes over parser state and makes early-exit/break impossible. A plain for-of is marginally clearer and friendlier to debugging. No functional difference here.
- orderedDependencies.forEach((dependency) => { - if (!processedTypes.has(dependency.typeAlias.getName())) { - typeAliasParser.parseWithImportFlag(dependency.typeAlias, dependency.isImported) - } - }) + for (const dependency of orderedDependencies) { + if (!processedTypes.has(dependency.typeAlias.getName())) { + typeAliasParser.parseWithImportFlag(dependency.typeAlias, dependency.isImported) + } + }
79-84: Same minor note for local type aliases loop.Purely stylistic; your call.
- localTypeAliases.forEach((typeAlias) => { - if (!processedTypes.has(typeAlias.getName())) { - typeAliasParser.parseWithImportFlag(typeAlias, false) - } - }) + for (const typeAlias of localTypeAliases) { + if (!processedTypes.has(typeAlias.getName())) { + typeAliasParser.parseWithImportFlag(typeAlias, false) + } + }
91-94: Processing interfaces in dependency order: correct and necessary.This should prevent forward-reference issues among local interfaces.
One caveat: imported interfaces aren’t part of this ordering or any import-collection step. If a local interface extends an imported interface (whose schema identifier must be present), generation may fail without corresponding imports.
If cross-file interface inheritance is in scope, consider:
- Adding an InterfaceDependencyCollector analogous to the type alias collector.
- Emitting imports for interface schema identifiers from external modules before parsing locals.
I can propose an approach if you confirm cross-file support is desired in this PR.tests/handlers/typebox/interfaces.test.ts (1)
165-203: Nested inheritance path is well-covered. Add out-of-order declaration and qualified-name cases.Two high-value additions to catch edge cases from this PR:
- Derived-before-base: ensure ordering utility handles source-order inversions.
- Qualified or generic bases:
interface B extends ns.A {}andinterface X<T> extends Base<T> {}.Example additional tests (sketch):
test('derived declared before base', () => { const sourceFile = createSourceFile( project, ` interface Extended extends Base { name: string } interface Base { id: string } `, ) expect(generateFormattedCode(sourceFile)).resolves.toBe( formatWithPrettier(` const Base = Type.Object({ id: Type.String() }); type Base = Static<typeof Base>; const Extended = Type.Composite([Base, Type.Object({ name: Type.String() })]); type Extended = Static<typeof Extended>; `), ) }) test('qualified base (namespace)', () => { const sourceFile = createSourceFile( project, ` declare namespace ns { interface A { a: string } } interface B extends ns.A { b: number } `, ) expect(generateFormattedCode(sourceFile)).resolves.toBe( formatWithPrettier(` // Depending on how ns.A is surfaced, ensure Composite uses ns.A correctly const B = Type.Composite([ns.A, Type.Object({ b: Type.Number() })]); type B = Static<typeof B>; `), ) }) test('generic base in extends (args ignored at runtime)', () => { const sourceFile = createSourceFile( project, ` interface Box<T> { value: T } interface StrBox extends Box<string> {} `, ) expect(generateFormattedCode(sourceFile)).resolves.toBe( formatWithPrettier(` const Box = Type.Object({ value: Type.Unknown() }); // or appropriate inference type Box = Static<typeof Box>; const StrBox = Type.Composite([Box, Type.Object({})]); type StrBox = Static<typeof StrBox>; `), ) })If you want, I can wire these up precisely to your current inference rules (Unknown vs inferred types) and adjust expectations accordingly.
ARCHITECTURE.md (5)
106-117: Fix file reference for dependency-ordered processing and add ToC entryThe text says the topological sort is “implemented in ts-morph-codegen.ts,” but per the PR it lives in a dedicated utility and is consumed from there. Also, the new section isn’t in the table of contents.
Apply this diff to correct the file reference:
-Interfaces are processed in dependency order using a topological sort algorithm implemented in <mcfile name="ts-morph-codegen.ts" path="src/ts-morph-codegen.ts"></mcfile>: +Interfaces are processed in dependency order using a topological sort implemented in <mcfile name="interface-processing-order.ts" path="src/utils/interface-processing-order.ts"></mcfile> and consumed by <mcfile name="ts-morph-codegen.ts" path="src/ts-morph-codegen.ts"></mcfile>:Optional: Add a TOC entry near the top of the file:
118-125: Rename heading to “Type.Composite” and note version compatibilityHeading uses “TypeBox.Composite” which isn’t the API surface; it should be “Type.Composite”. Consider noting minimum TypeBox support for Composite or the fallback strategy.
Apply this diff:
-### TypeBox.Composite Generation +### Type.Composite GenerationOptionally append a note after this bullet list:
Note: Ensure your TypeBox version supports `Type.Composite`. If not available, configure a fallback strategy (e.g., `Type.Intersect`) or pin a compatible TypeBox version.
122-124: Tighten wording on how composites are formedSmall clarity improvement: show the composite shape once and reference it.
Apply this diff:
-- **Property Combination**: The `InterfaceTypeHandler` generates `Type.Composite([BaseInterface, Type.Object({...})])` for extended interfaces +- **Property Combination**: Extended interfaces are emitted as `Type.Composite([BaseInterface, Type.Object({ ...extendedProps })])`
129-131: Good implementation notes; consider adding failure mode hintNice callouts. Consider adding what error looks like when order is wrong to aid troubleshooting.
Suggested addition (after the “Error Prevention” bullet):
- **Troubleshooting**: If dependency ordering is bypassed, you may see errors like “No handler found for type 'BaseInterface'”. Verify `getInterfaceProcessingOrder` is applied before parsing.
257-257: LGTM, plus a small consistency suggestionThe new parser entry looks good. For completeness, consider updating “Parser Instantiation” (Function Flow, Step 3) to explicitly list
InterfaceParser.Proposed wording to add under the parser instantiation list (outside this hunk):
- `InterfaceParser`: processes interface declarations (including inheritance) and emits object schemas and composites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
ARCHITECTURE.md(4 hunks)package.json(1 hunks)src/handlers/typebox/object/interface-type-handler.ts(2 hunks)src/handlers/typebox/typebox-type-handlers.ts(0 hunks)src/ts-morph-codegen.ts(2 hunks)src/utils/interface-processing-order.ts(1 hunks)tests/handlers/typebox/interfaces.test.ts(1 hunks)tests/handlers/typebox/object-types.test.ts(0 hunks)
💤 Files with no reviewable changes (2)
- tests/handlers/typebox/object-types.test.ts
- src/handlers/typebox/typebox-type-handlers.ts
🧰 Additional context used
🧬 Code graph analysis (3)
tests/handlers/typebox/interfaces.test.ts (1)
tests/utils.ts (3)
createSourceFile(8-10)generateFormattedCode(17-28)formatWithPrettier(12-15)
src/handlers/typebox/object/interface-type-handler.ts (1)
src/utils/typebox-codegen-utils.ts (1)
makeTypeCall(3-12)
src/ts-morph-codegen.ts (1)
src/utils/interface-processing-order.ts (1)
getInterfaceProcessingOrder(3-54)
🔇 Additional comments (10)
package.json (2)
16-16: No functional impact from devDependency reordering.Reordering eslint-plugin-prettier is harmless. Nothing to change here.
9-9: Confirm necessity of “@sinclair/typebox-codegen” devDependencyI ran a repository-wide search (excluding package.json) and found no references to
@sinclair/typebox-codegenin source code or tests—only entries in bun.lock.
- If this package isn’t yet used, please remove it from package.json to keep installs lean.
- If it’s planned for imminent use, add a brief note in the PR description to clarify its purpose for future reviewers.
src/handlers/typebox/object/interface-type-handler.ts (3)
2-2: Good reuse of shared AST helper.Importing and using makeTypeCall keeps the TypeBox call construction consistent with the rest of the codegen.
11-16: Base path for non-inherited interfaces is correct.Falling back to the base object type when there are no heritage clauses maintains prior behavior.
34-37: Composite order is correct (bases first, then current).Placing extended types first and the current interface’s object last matches allOf/Compose semantics and the tests’ expectations.
src/ts-morph-codegen.ts (2)
10-10: Importing getInterfaceProcessingOrder is a solid integration point.Clean separation of concerns; keeps sorting logic out of the main generator.
87-89: Enum processing looks good.No ordering concerns with the surrounding changes.
tests/handlers/typebox/interfaces.test.ts (2)
12-40: Solid baseline tests for exported/non-exported interfaces.These validate the pre-inheritance path still emits Type.Object and Static aliases as expected.
72-107: Good coverage of multiple inheritance and Composite ordering.Verifies A, B precede C’s own object in the array. This aligns with the handler.
ARCHITECTURE.md (1)
221-221: LGTM: Interface handler description matches implementation directionThe description clearly states Composite-based inheritance and aligns with the PR goals.
| 6. **Type Alias Processing**: The `TypeAliasParser` is instantiated and iterates through all `type alias` declarations in the input `sourceFile`. For each type alias, its underlying type node is converted into a TypeBox-compatible type representation, a TypeBox schema is generated, and a corresponding static type alias is added. | ||
|
|
||
| 7. **Interface Processing**: The `InterfaceParser` is instantiated and iterates through all `interface` declarations in the input `sourceFile`. For each interface, its properties and methods are converted into TypeBox object schemas with corresponding static type aliases. | ||
| 7. **Interface Processing**: The `InterfaceParser` is instantiated and iterates through all `interface` declarations in the input `sourceFile`. Interfaces are processed in dependency order to handle inheritance properly. For each interface, its properties and methods are converted into TypeBox object schemas. Interfaces that extend other interfaces are generated using `Type.Intersect` to combine the base interface with additional properties, ensuring proper inheritance semantics. |
There was a problem hiding this comment.
Update to reflect Type.Composite and clarify method handling
This still documents Type.Intersect and says “properties and methods are converted,” which conflicts with the new Composite-based inheritance flow and can mislead readers (methods aren’t represented in TypeBox object schemas unless modeled as function-valued properties).
Apply this diff to align with the implementation and remove the misleading “methods” claim:
-7. **Interface Processing**: The `InterfaceParser` is instantiated and iterates through all `interface` declarations in the input `sourceFile`. Interfaces are processed in dependency order to handle inheritance properly. For each interface, its properties and methods are converted into TypeBox object schemas. Interfaces that extend other interfaces are generated using `Type.Intersect` to combine the base interface with additional properties, ensuring proper inheritance semantics.
+7. **Interface Processing**: The `InterfaceParser` is instantiated and iterates through all `interface` declarations in the input `sourceFile`. Interfaces are processed in dependency order (via the `getInterfaceProcessingOrder` utility) to handle inheritance correctly. For each interface, its properties are converted into TypeBox object schemas (method signatures are not emitted unless modeled as function-valued properties). Interfaces that extend other interfaces are generated using `Type.Composite([BaseInterface, Type.Object({ ... })])` to combine base interfaces with additional properties.Additionally, consider adding a cross-link to the utility:
- See:
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 7. **Interface Processing**: The `InterfaceParser` is instantiated and iterates through all `interface` declarations in the input `sourceFile`. Interfaces are processed in dependency order to handle inheritance properly. For each interface, its properties and methods are converted into TypeBox object schemas. Interfaces that extend other interfaces are generated using `Type.Intersect` to combine the base interface with additional properties, ensuring proper inheritance semantics. | |
| 7. **Interface Processing**: The `InterfaceParser` is instantiated and iterates through all `interface` declarations in the input `sourceFile`. Interfaces are processed in dependency order (via the `getInterfaceProcessingOrder` utility) to handle inheritance correctly. For each interface, its properties are converted into TypeBox object schemas (method signatures are not emitted unless modeled as function-valued properties). Interfaces that extend other interfaces are generated using `Type.Composite([BaseInterface, Type.Object({ ... })])` to combine base interfaces with additional properties. | |
| - See: <mcfile name="interface-processing-order.ts" path="src/utils/interface-processing-order.ts"></mcfile> |
🤖 Prompt for AI Agents
In ARCHITECTURE.md around line 64, the text still references Type.Intersect and
claims “properties and methods are converted,” which is inaccurate; update the
wording to state that InterfaceParser uses Type.Composite for inheritance (not
Type.Intersect), clarify that only properties are mapped to TypeBox object
schemas (methods are not represented unless explicitly modeled as
function-valued properties), and add a cross-link to the utility file
src/utils/interface-processing-order.ts (mcfile reference) so readers can follow
the processing order implementation.
| for (const heritageClause of heritageClauses) { | ||
| if (heritageClause.getToken() === ts.SyntaxKind.ExtendsKeyword) { | ||
| for (const typeNode of heritageClause.getTypeNodes()) { | ||
| // For interface inheritance, we reference the already processed interface by name | ||
| const referencedTypeName = typeNode.getText() | ||
| extendedTypes.push(ts.factory.createIdentifier(referencedTypeName)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incorrect creation of references to extended interfaces (qualified names/generics).
Using typeNode.getText() and ts.factory.createIdentifier(referencedTypeName) will break for:
- Qualified names (e.g.,
ns.Base) →createIdentifier('ns.Base')is invalid TS. - Generic instantiations (e.g.,
Base<T>) → becomes an invalid identifier. - Import-qualified expressions (rare, but similar issues).
Build a proper expression:
- Extract the expression part (
typeNode.getExpression()), not the full text. - Trim generic args if present.
- Expand property access chains into nested
PropertyAccessExpressions.
Apply this diff within the loop to construct a correct ts.Expression for each extended type:
- for (const typeNode of heritageClause.getTypeNodes()) {
- // For interface inheritance, we reference the already processed interface by name
- const referencedTypeName = typeNode.getText()
- extendedTypes.push(ts.factory.createIdentifier(referencedTypeName))
- }
+ for (const typeNode of heritageClause.getTypeNodes()) {
+ // Resolve the expression (e.g., Base, ns.Base, ns.sub.Base), ignoring any type arguments.
+ const exprText = (typeNode as any).getExpression
+ ? (typeNode as any).getExpression().getText()
+ : typeNode.getText()
+ const genericStart = exprText.indexOf('<')
+ const withoutGenerics = genericStart === -1 ? exprText : exprText.slice(0, genericStart)
+ const parts = withoutGenerics.split('.').filter(Boolean)
+ let refExpr: ts.Expression = ts.factory.createIdentifier(parts[0])
+ for (let i = 1; i < parts.length; i++) {
+ refExpr = ts.factory.createPropertyAccessExpression(refExpr, parts[i])
+ }
+ extendedTypes.push(refExpr)
+ }Follow-up: consider de-duplicating extendedTypes if the same base is listed more than once (low priority).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const heritageClause of heritageClauses) { | |
| if (heritageClause.getToken() === ts.SyntaxKind.ExtendsKeyword) { | |
| for (const typeNode of heritageClause.getTypeNodes()) { | |
| // For interface inheritance, we reference the already processed interface by name | |
| const referencedTypeName = typeNode.getText() | |
| extendedTypes.push(ts.factory.createIdentifier(referencedTypeName)) | |
| } | |
| } | |
| } | |
| for (const heritageClause of heritageClauses) { | |
| if (heritageClause.getToken() === ts.SyntaxKind.ExtendsKeyword) { | |
| for (const typeNode of heritageClause.getTypeNodes()) { | |
| // Resolve the expression (e.g., Base, ns.Base, ns.sub.Base), ignoring any type arguments. | |
| const exprText = (typeNode as any).getExpression | |
| ? (typeNode as any).getExpression().getText() | |
| : typeNode.getText() | |
| const genericStart = exprText.indexOf('<') | |
| const withoutGenerics = genericStart === -1 | |
| ? exprText | |
| : exprText.slice(0, genericStart) | |
| const parts = withoutGenerics.split('.').filter(Boolean) | |
| let refExpr: ts.Expression = ts.factory.createIdentifier(parts[0]) | |
| for (let i = 1; i < parts.length; i++) { | |
| refExpr = ts.factory.createPropertyAccessExpression(refExpr, parts[i]) | |
| } | |
| extendedTypes.push(refExpr) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/handlers/typebox/object/interface-type-handler.ts around lines 20 to 28,
the current code uses typeNode.getText() and ts.factory.createIdentifier(...)
which breaks for qualified names (ns.Base), generic instantiations (Base<T>),
and other non-identifier expressions; replace this by extracting the expression
part (use typeNode.getExpression()), then construct a proper ts.Expression by
recursively converting that expression: if it's an Identifier create
ts.factory.createIdentifier(name), if it's a QualifiedName or
PropertyAccessChain build nested
ts.factory.createPropertyAccessExpression(leftExpr, rightName) by walking the
left/right parts, and ignore/strip typeArguments (using the expression avoids
generics); push the resulting ts.Expression into extendedTypes instead of a
string-based Identifier; optionally de-duplicate extendedTypes if the same base
appears more than once.
| heritageClause.getTypeNodes().forEach((typeNode) => { | ||
| const baseInterfaceName = typeNode.getText() | ||
| if (interfaceMap.has(baseInterfaceName)) { | ||
| visit(baseInterfaceName) | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Name resolution for base interfaces can misorder dependencies.
typeNode.getText() may include generics (Base<T>) or qualified paths (ns.Base), which won’t match iface.getName() (e.g., Base). This can cause derived interfaces to be processed before their bases.
Fix by extracting the expression, normalizing to the simple name, and matching the map key.
Apply this diff:
- heritageClause.getTypeNodes().forEach((typeNode) => {
- const baseInterfaceName = typeNode.getText()
- if (interfaceMap.has(baseInterfaceName)) {
- visit(baseInterfaceName)
- }
- })
+ heritageClause.getTypeNodes().forEach((typeNode) => {
+ // Expression text excludes type arguments; handle qualified names (ns.Base)
+ const exprText =
+ (typeNode as any).getExpression ? (typeNode as any).getExpression().getText() : typeNode.getText()
+ const simple = exprText.split('.').pop()!
+ const baseInterfaceName = simple.split('<')[0]
+ if (interfaceMap.has(baseInterfaceName)) {
+ visit(baseInterfaceName)
+ }
+ })Optional: if you plan to support duplicate interface names under different namespaces (in the same file), this map-by-name approach will need to be revisited. For now, this is fine.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| heritageClause.getTypeNodes().forEach((typeNode) => { | |
| const baseInterfaceName = typeNode.getText() | |
| if (interfaceMap.has(baseInterfaceName)) { | |
| visit(baseInterfaceName) | |
| } | |
| }) | |
| }) | |
| heritageClause.getTypeNodes().forEach((typeNode) => { | |
| // Expression text excludes type arguments; handle qualified names (ns.Base) | |
| const exprText = | |
| (typeNode as any).getExpression ? (typeNode as any).getExpression().getText() : typeNode.getText() | |
| const simple = exprText.split('.').pop()! | |
| const baseInterfaceName = simple.split('<')[0] | |
| if (interfaceMap.has(baseInterfaceName)) { | |
| visit(baseInterfaceName) | |
| } | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In src/utils/interface-processing-order.ts around lines 35 to 41, the code uses
typeNode.getText() to lookup base interfaces which can include generics or
qualified names and therefore not match iface.getName(); change the lookup to
extract and normalize the simple identifier: if the node is a qualified name
take the rightmost identifier, if it includes type arguments strip them (or use
the underlying expression/typeName), then use that simple name to check
interfaceMap and call visit; keep current behavior for unmatched names. This
ensures Base<T> and ns.Base resolve to "Base" before matching the map key.
1 participant
This change introduces a new utility function
getInterfaceProcessingOrderthatdetermines the order in which interfaces should be processed. This is necessary
to ensure that interfaces that depend on other interfaces are processed in the
correct order.
The changes include:
getInterfaceProcessingOrderfunction from the@daxserver/validation-schema-codegen/utils/interface-processing-ordermodule.determine the processing order.
forEachinstead of aforloop.
These changes improve the reliability and maintainability of the code generation
process by ensuring that interfaces are processed in the correct order.