-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add resolver store for type name management #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,8 +6,8 @@ import { makeTypeCall } from '@daxserver/validation-schema-codegen/utils/typebox | |||||||||||||||||
| import { ts, TypeAliasDeclaration } from 'ts-morph' | ||||||||||||||||||
|
|
||||||||||||||||||
| export class TypeAliasParser extends BaseParser { | ||||||||||||||||||
| parse(typeAlias: TypeAliasDeclaration): void { | ||||||||||||||||||
| const typeName = typeAlias.getName() | ||||||||||||||||||
| parse(typeAlias: TypeAliasDeclaration, aliasName?: string): void { | ||||||||||||||||||
| const typeName = aliasName || typeAlias.getName() | ||||||||||||||||||
|
|
||||||||||||||||||
|
Comment on lines
+9
to
11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Sanitize aliasName to a valid TS identifier before use. Aliases derived from paths may include invalid chars, breaking codegen. - parse(typeAlias: TypeAliasDeclaration, aliasName?: string): void {
- const typeName = aliasName || typeAlias.getName()
+ parse(typeAlias: TypeAliasDeclaration, aliasName?: string): void {
+ const makeSafeIdentifier = (name: string) => name.replace(/[^A-Za-z0-9_$]/g, '_')
+ const resolved = aliasName ?? typeAlias.getName()
+ const typeName = ts.isIdentifierText(resolved, ts.ScriptTarget.Latest)
+ ? resolved
+ : makeSafeIdentifier(resolved)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| if (this.processedTypes.has(typeName)) return | ||||||||||||||||||
| this.processedTypes.add(typeName) | ||||||||||||||||||
|
|
@@ -16,15 +16,13 @@ export class TypeAliasParser extends BaseParser { | |||||||||||||||||
|
|
||||||||||||||||||
| // Check if type alias has type parameters (generic) | ||||||||||||||||||
| if (typeParameters.length > 0) { | ||||||||||||||||||
| this.parseGenericTypeAlias(typeAlias) | ||||||||||||||||||
| this.parseGenericTypeAlias(typeAlias, typeName) | ||||||||||||||||||
| } else { | ||||||||||||||||||
| this.parseRegularTypeAlias(typeAlias) | ||||||||||||||||||
| this.parseRegularTypeAlias(typeAlias, typeName) | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private parseRegularTypeAlias(typeAlias: TypeAliasDeclaration): void { | ||||||||||||||||||
| const typeName = typeAlias.getName() | ||||||||||||||||||
|
|
||||||||||||||||||
| private parseRegularTypeAlias(typeAlias: TypeAliasDeclaration, typeName: string): void { | ||||||||||||||||||
| const typeNode = typeAlias.getTypeNode() | ||||||||||||||||||
| const typeboxTypeNode = typeNode ? getTypeBoxType(typeNode) : makeTypeCall('Any') | ||||||||||||||||||
| const typeboxType = this.printer.printNode( | ||||||||||||||||||
|
|
@@ -38,8 +36,7 @@ export class TypeAliasParser extends BaseParser { | |||||||||||||||||
| addStaticTypeAlias(this.newSourceFile, typeName, this.newSourceFile.compilerNode, this.printer) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private parseGenericTypeAlias(typeAlias: TypeAliasDeclaration): void { | ||||||||||||||||||
| const typeName = typeAlias.getName() | ||||||||||||||||||
| private parseGenericTypeAlias(typeAlias: TypeAliasDeclaration, typeName: string): void { | ||||||||||||||||||
| const typeParameters = typeAlias.getTypeParameters() | ||||||||||||||||||
|
|
||||||||||||||||||
| // Generate TypeBox function definition | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,7 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| GraphVisualizer, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type VisualizationOptions, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from '@daxserver/validation-schema-codegen/utils/graph-visualizer' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { resolverStore } from '@daxserver/validation-schema-codegen/utils/resolver-store' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { hasCycle, topologicalSort } from 'graphology-dag' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { SourceFile } from 'ts-morph' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -16,14 +17,17 @@ export class DependencyTraversal { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private nodeGraph = new NodeGraph() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private maincodeNodeIds = new Set<string>() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private requiredNodeIds = new Set<string>() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private importCollector = new ImportCollector(this.fileGraph, this.nodeGraph) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private importCollector: ImportCollector | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| constructor() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.importCollector = new ImportCollector(this.fileGraph, this.nodeGraph) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| startTraversal(mainSourceFile: SourceFile): TraversedNode[] { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| startTraversal(sourceFile: SourceFile): TraversedNode[] { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Mark main source file nodes as main code | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addLocalTypes(mainSourceFile, this.nodeGraph, this.maincodeNodeIds, this.requiredNodeIds) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addLocalTypes(sourceFile, this.nodeGraph, this.maincodeNodeIds, this.requiredNodeIds) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Start recursive traversal from imports | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const importDeclarations = mainSourceFile.getImportDeclarations() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const importDeclarations = sourceFile.getImportDeclarations() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.importCollector.collectFromImports(importDeclarations) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Extract dependencies for all nodes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -38,23 +42,30 @@ export class DependencyTraversal { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Handles circular dependencies gracefully by falling back to simple node order | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| getNodesToPrint(): TraversedNode[] { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Get all nodes in topological order, then filter to only required ones | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const allNodesInOrder = hasCycle(this.nodeGraph) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ? Array.from(this.nodeGraph.nodes()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| : topologicalSort(this.nodeGraph) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Get all qualified names from resolver store, then apply topological ordering | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const allQualifiedNames = resolverStore.getAllQualifiedNames() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Filter to only required nodes that exist in both resolver store and node graph | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const requiredQualifiedNames = allQualifiedNames.filter((qualifiedName: string) => | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.requiredNodeIds.has(qualifiedName), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Apply topological ordering only to the required nodes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const orderedNodes = hasCycle(this.nodeGraph) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ? requiredQualifiedNames | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| : topologicalSort(this.nodeGraph).filter((nodeId: string) => | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| requiredQualifiedNames.includes(nodeId), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+49
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Topological sort should be based on the required subgraph, not global cycle presence. A cycle elsewhere in the graph disables ordering for an acyclic required subset, degrading output determinism. One pragmatic fix is to attempt sorting and fall back on failure: - const orderedNodes = hasCycle(this.nodeGraph)
- ? requiredQualifiedNames
- : topologicalSort(this.nodeGraph).filter((nodeId: string) =>
- requiredQualifiedNames.includes(nodeId),
- )
+ let orderedNodes: string[]
+ try {
+ const requiredSet = new Set(requiredQualifiedNames)
+ orderedNodes = topologicalSort(this.nodeGraph).filter((id: string) => requiredSet.has(id))
+ } catch {
+ // Graph has cycles; fall back to required insertion order
+ orderedNodes = requiredQualifiedNames
+ }If feasible, prefer sorting an induced subgraph of required nodes for correctness and performance. I can sketch a small helper to build that subgraph if NodeGraph exposes the underlying Graphology graph. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const filteredNodes = allNodesInOrder | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .filter((nodeId: string) => this.requiredNodeIds.has(nodeId)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Map to actual node data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const filteredNodes = orderedNodes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .map((nodeId: string) => this.nodeGraph.getNode(nodeId)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .filter((node): node is TraversedNode => node !== null) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return filteredNodes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async visualizeGraph(options: VisualizationOptions = {}): Promise<string> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return GraphVisualizer.generateVisualization(this.nodeGraph, options) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| getNodeGraph(): NodeGraph { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return this.nodeGraph | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add a safe fallback when no alias is registered.
Ensure we never emit an undefined identifier.
📝 Committable suggestion
🤖 Prompt for AI Agents