feat(dependency-management): Improve dependency filtering for better output #20
Conversation
…output This commit introduces several improvements to the dependency management system: - Filters out unconnected dependencies to optimize output - Automatically marks local types as required - Marks imported types as required only if they are actually referenced by other types - Automatically includes transitive dependencies when their parent types are referenced The key changes are: - Added `maincodeNodeIds` and `requiredNodeIds` sets to track main code and required nodes - Updated `addLocalTypes` to mark local types as both main code and required - Adjusted `collectFromImports` to only mark imported types as required if they are referenced - Implemented filtering logic in `getNodesToPrint` to return only the required nodes These changes ensure the dependency analysis produces a more optimized output, only including the necessary types and their transitive dependencies.
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds docs for dependency and maincode filtering. Refactors traversal to modular utilities for local type collection, import collection, and dependency extraction. Updates traversal to return only required nodes. Introduces TypeReferenceExtractor. Adds integration and unit tests validating filtered output, ordering, and circular references. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor TestOrCLI as Caller
participant TS as SourceFile (ts-morph)
participant DT as DependencyTraversal
participant LTC as LocalTypeCollector
participant IC as ImportCollector
participant NG as NodeGraph
participant FG as FileGraph
participant DEX as DependencyExtractor
participant TRE as TypeReferenceExtractor
TestOrCLI->>DT: startTraversal(TS)
DT->>LTC: addLocalTypes(TS, NG, mainIds, requiredIds)
LTC-->>NG: register local nodes (isImported=false, isMainCode=true)
LTC-->>DT: update mainIds, requiredIds
DT->>IC: collectFromImports(TS.imports)
IC->>FG: ensure file nodes / prevent cycles
IC->>NG: add imported nodes (isImported=true)
IC->>IC: recurse into nested imports
DT->>DEX: extractDependencies(NG, requiredIds)
loop until fixed-point
DEX->>NG: get node to process
DEX->>TRE: extractTypeReferences(tsNode)
TRE-->>DEX: referenced qualified names[]
DEX->>NG: addDependency(referenced -> current)
DEX-->>DT: add to requiredIds / queue
end
DT->>DT: getNodesToPrint(requiredIds)
DT-->>TestOrCLI: ordered subset for codegen
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/traverse/dependency-traversal.integration.test.ts (1)
435-486: Use lazy/recursive TypeBox APIs for circular references
Eagerly callingType.Optional(B)inAbeforeBis initialized triggers a TDZ at module load; switch to a lazy pattern (e.g.Type.Recursiveor$id+Type.Ref) so cross‐references defer evaluation.
🧹 Nitpick comments (25)
docs/dependency-management.md (2)
32-38: Tighten the 6-step process wording for precisionMinor clarity/grammar tweaks and make Step 4 explicit about the criterion (membership in requiredNodeIds / reachability from maincode).
-1. Collect local types from main file (automatically marked as required) -2. Process import chains recursively (adds types to graph) -3. Extract type dependencies (marks referenced types as required) -4. Filter to only connected dependencies -5. Build dependency graph -6. Topological sort for processing order +1. Collect local types from the main file (automatically mark as required). +2. Process import chains recursively (add discovered types to the graph). +3. Extract type dependencies (mark actually referenced types as required). +4. Filter nodes to the connected set (members of requiredNodeIds or reachable from maincode). +5. Build the dependency graph. +6. Topologically sort the filtered nodes to get the processing order.
11-13: Echo “filtering” intent in the component bulletsConsider mentioning the “required set” here so the orchestrator’s responsibilities mirror the algorithm.
-- Filters out unconnected dependencies +- Maintains a required set and filters out unconnected dependenciessrc/traverse/local-type-collector.ts (2)
16-22: Revisit “import-only entry file” behavior to match PR objective (“imported types only if referenced”)Current branch adds all named imports as required when there are no local types, which bloats output and contradicts the stated goal. Either drop the auto-require or guard it behind a feature flag.
Option A (strict, matches docs): do not add imports to required here.
- // If main file has no local types but has imports, add all imported types as required + // If the main file has no local types, do not auto-mark imports as required. + // They will be pulled in only if referenced during dependency extraction.Option B (keep seeding but make it explicit): add a boolean parameter
seedImportsWhenNoLocal = falseand gate therequiredNodeIds.add(...)on it.
87-103: Coverage: add classes and type-only filter (optional)If classes are a supported construct elsewhere, include them. Also consider skipping non type-only imports in the import-only branch to avoid accidental value captures.
I can send a small patch adding class collection and honoring
importDecl.isTypeOnly().docs/maincode-filtering.md (3)
18-29: Use headings instead of bold labels for file captions (fixes MD036)-**external.ts** +#### external.ts @@ -**main.ts** +#### main.ts
76-101: Standardize section captions in the second example (fixes MD036)-**level1.ts** +#### level1.ts @@ -**level2.ts** +#### level2.ts @@ -**main.ts** +#### main.ts
9-13: Clarify “connected dependencies” and state the import-only edge caseSlight wording tweak and add a brief note about files with no local types so readers know what to expect.
-2. **Connected dependencies**: Types that are directly or indirectly referenced by maincode nodes +2. **Connected dependencies**: Types directly or indirectly referenced by maincode nodes (i.e., reachable via the dependency graph) @@ -Imported types that are not referenced anywhere in the dependency chain are automatically filtered out. +Imported types that are not referenced anywhere in the dependency chain are automatically filtered out. +Note: If the entry file contains no local types, decide whether to seed its named imports as required (include all) or keep strict filtering (include none). The implementation defaults should be documented alongside configuration.tests/traverse/dependency-ordering.test.ts (4)
151-223: Great end-to-end test; add one for re-exports to prevent regression on qualified-name resolutionThis suite is strong. One missing case:
main.tsimports a type re-exported fromindex.tswhich re-exports fromtypes.ts. This guards the qualified-name bug and ensures the traversal resolves to the declaration file.If helpful, I can draft a test snippet covering
export { Foo } from './types'via an index barrel.
152-153: Avoid shadowing the top-levelprojectin these testsReuse the
beforeEach-initializedprojectfor consistency and to reduce setup overhead.- test('should handle complex structure without including unused types', () => { - const project = new Project() + test('should handle complex structure without including unused types', () => {
322-323: Same nit: reuse the sharedproject- test('should handle circular references', () => { - const project = new Project() + test('should handle circular references', () => {
224-319: Consider an “import-only entry file” test to lock intended behaviorAdd a focused test where the entry file only has
import type { Foo } from './foo'and no local declarations, then assert the output (either empty or seeded) per the chosen policy. This will keep docs, PR goals, and implementation in sync.src/traverse/dependency-extractor.ts (3)
17-22: Avoid O(n) extraction from Set on every loop iterationUse the Set iterator instead of
Array.from(...)[0]to reduce per-iteration overhead and allocations.- while (nodesToProcess.size > 0) { - const currentNodeId = Array.from(nodesToProcess)[0] - if (!currentNodeId) break - - nodesToProcess.delete(currentNodeId) + while (nodesToProcess.size > 0) { + const it = nodesToProcess.values() + const currentNodeId = it.next().value as string | undefined + if (!currentNodeId) break + nodesToProcess.delete(currentNodeId)
23-31: Guard graph access and mark processed after verifying node existencePrevents potential throws from
getNodeand avoids marking a node as processed when it’s not in the graph.- if (processedNodes.has(currentNodeId)) continue - processedNodes.add(currentNodeId) - - const nodeData = nodeGraph.getNode(currentNodeId) - if (!nodeData) continue + if (processedNodes.has(currentNodeId)) continue + if (!nodeGraph.hasNode(currentNodeId)) continue + const nodeData = nodeGraph.getNode(currentNodeId) + processedNodes.add(currentNodeId)
44-55: Deduplicate references and avoid re-queuing already processed nodesThis reduces redundant edge checks and churn in
nodesToProcess.- const typeReferences = typeReferenceExtractor.extractTypeReferences(nodeToAnalyze) + const uniqueRefs = new Set(typeReferenceExtractor.extractTypeReferences(nodeToAnalyze)) - for (const referencedType of typeReferences) { + for (const referencedType of uniqueRefs) { if (nodeGraph.hasNode(referencedType)) { - // Only add to required if not already processed - if (!requiredNodeIds.has(referencedType)) { - requiredNodeIds.add(referencedType) - nodesToProcess.add(referencedType) - } + if (!requiredNodeIds.has(referencedType)) requiredNodeIds.add(referencedType) + if (!processedNodes.has(referencedType)) nodesToProcess.add(referencedType) nodeGraph.addDependency(referencedType, currentNodeId) } }src/traverse/import-collector.ts (3)
23-28: Consider restricting initial collection to exported declarationsCollecting every alias/interface/enum/function (including non-exported) can balloon the graph for large modules. If you only need non-exported nodes when they’re referenced by an exported surface, prefer starting from
getExportedDeclarations()and lazily discovering internals during dependency extraction.
72-85: Confirm functions should be modeled as “type nodes”You add function declarations as nodes (
type: 'function'). If runtime function bodies are irrelevant to schema generation and only their signatures contribute types, ensure downstream code treats these nodes purely for type-surface traversal and never emits function runtime. Otherwise, skip functions here.
30-41: Maintain determinism: sort discovered declarations before insertionSorting
typeAliases,interfaces,enums, andfunctionsby name beforeaddTypeNodemakes graph construction deterministic across Node.js versions and file-system orders, improving reproducible builds.- const typeAliases = moduleSourceFile.getTypeAliases() + const typeAliases = moduleSourceFile.getTypeAliases().sort((a,b) => a.getName().localeCompare(b.getName())) - const interfaces = moduleSourceFile.getInterfaces() + const interfaces = moduleSourceFile.getInterfaces().sort((a,b) => a.getName().localeCompare(b.getName())) - const enums = moduleSourceFile.getEnums() + const enums = moduleSourceFile.getEnums().sort((a,b) => a.getName().localeCompare(b.getName())) - const functions = moduleSourceFile.getFunctions() + const functions = moduleSourceFile.getFunctions().sort((a,b) => (a.getName() ?? '').localeCompare(b.getName() ?? ''))Also applies to: 43-57, 59-70
tests/traverse/maincode-filter.test.ts (1)
142-231: Add coverage for generics, typeof, and inheritance casesSince
TypeReferenceExtractorhandlesTypeQuery (typeof),ExpressionWithTypeArguments, and generic type args, add a test where maincode extends a generic imported base and usestypeof SomeConstto ensure references are picked up.I can add a concise test snippet if you want.
tests/traverse/dependency-traversal.integration.test.ts (1)
181-188: Mixed testing stylesMost tests validate code-gen strings; this one still asserts via
DependencyTraversal. Consider consolidating on the public code-gen surface for consistency unless this is intentionally covering the low-level API.tests/traverse/non-transitive-dependency.test.ts (1)
75-115: Alias-transitive case covered; consider adding duplicate-name module testAdd a case with two imported modules exporting
Idto validate disambiguation picks the correct one. This will surface the current name-only matching limitation.src/traverse/type-reference-extractor.ts (2)
27-42: Handle typeof import('...').X and avoid missing references from TypeQuery import forms.TypeQuery can be “typeof import(‘...’).Foo”. Support ImportTypeNode to resolve to Foo’s declaration.
Example addition:
if (Node.isTypeQuery(node)) { const exprName = node.getExprName() - if ( + if ( Node.isIdentifier(exprName) || Node.isQualifiedName(exprName) || Node.isPropertyAccessExpression(exprName) ) { // existing logic + } else if (Node.isImportTypeNode(exprName)) { + const qualifier = exprName.getQualifier() + if (qualifier) { + const typeName = qualifier.getText().split('.').pop()! + for (const qualifiedName of this.nodeGraph.nodes()) { + const nodeData = this.nodeGraph.getNode(qualifiedName) + if (nodeData.originalName === typeName) { + references.push(qualifiedName) + break + } + } + } } }
7-10: Deduplicate before returning to cut redundant work downstream.This keeps requiredNodeIds churn low without touching all push sites.
- return references + return Array.from(new Set(references))Also applies to: 119-120
src/traverse/dependency-traversal.ts (3)
41-50: Ensure deterministic output when cycles exist — sort fallback by qualifiedName.Array.from(nodes) can produce order tied to insertion, which may vary by traversal. Sort by qualifiedName for stability.
- const allNodesInOrder = hasCycle(this.nodeGraph) - ? Array.from(this.nodeGraph.nodes()) - : topologicalSort(this.nodeGraph) + const allNodesInOrder = hasCycle(this.nodeGraph) + ? Array.from(this.nodeGraph.nodes()).sort((a, b) => + this.nodeGraph.getNode(a).qualifiedName.localeCompare( + this.nodeGraph.getNode(b).qualifiedName, + ), + ) + : topologicalSort(this.nodeGraph)
21-33: Guard against stale state across multiple runs — reset graphs/sets at start.If a single DependencyTraversal instance is reused, previous state leaks. Clear graphs/sets before new traversal.
Add this method and call it at Line 21:
private reset(): void { this.fileGraph = new FileGraph() this.nodeGraph = new NodeGraph() this.maincodeNodeIds.clear() this.requiredNodeIds.clear() this.importCollector = new ImportCollector(this.fileGraph, this.nodeGraph) }- startTraversal(mainSourceFile: SourceFile): TraversedNode[] { + startTraversal(mainSourceFile: SourceFile): TraversedNode[] { + this.reset()
17-20: maincodeNodeIds is populated but unused here. Remove or put it to use.If not used for filtering/visualization, drop it to reduce state surface; otherwise, consider exposing it for debugging or tests.
📜 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 selected for processing (12)
docs/dependency-management.md(3 hunks)docs/maincode-filtering.md(1 hunks)docs/overview.md(1 hunks)src/traverse/dependency-extractor.ts(1 hunks)src/traverse/dependency-traversal.ts(1 hunks)src/traverse/import-collector.ts(1 hunks)src/traverse/local-type-collector.ts(1 hunks)src/traverse/type-reference-extractor.ts(1 hunks)tests/traverse/dependency-ordering.test.ts(2 hunks)tests/traverse/dependency-traversal.integration.test.ts(14 hunks)tests/traverse/maincode-filter.test.ts(1 hunks)tests/traverse/non-transitive-dependency.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/traverse/local-type-collector.ts (2)
src/traverse/node-graph.ts (1)
NodeGraph(8-38)src/utils/generate-qualified-name.ts (1)
generateQualifiedNodeName(20-25)
src/traverse/import-collector.ts (3)
src/traverse/file-graph.ts (1)
FileGraph(13-25)src/traverse/node-graph.ts (1)
NodeGraph(8-38)src/utils/generate-qualified-name.ts (1)
generateQualifiedNodeName(20-25)
src/traverse/dependency-extractor.ts (2)
src/traverse/node-graph.ts (1)
NodeGraph(8-38)src/traverse/type-reference-extractor.ts (1)
TypeReferenceExtractor(4-121)
tests/traverse/non-transitive-dependency.test.ts (1)
tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)
src/traverse/type-reference-extractor.ts (1)
src/traverse/node-graph.ts (1)
NodeGraph(8-38)
tests/traverse/maincode-filter.test.ts (1)
tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)
tests/traverse/dependency-ordering.test.ts (1)
tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)
tests/traverse/dependency-traversal.integration.test.ts (1)
tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)
src/traverse/dependency-traversal.ts (6)
src/traverse/file-graph.ts (1)
FileGraph(13-25)src/traverse/node-graph.ts (1)
NodeGraph(8-38)src/traverse/import-collector.ts (1)
ImportCollector(6-91)src/traverse/types.ts (1)
TraversedNode(3-10)src/traverse/local-type-collector.ts (1)
addLocalTypes(5-104)src/traverse/dependency-extractor.ts (1)
extractDependencies(11-57)
🪛 LanguageTool
docs/maincode-filtering.md
[grammar] ~9-~9: There might be a mistake here.
Context: ...d in the main source file (not imported) 2. Connected dependencies: Types that are...
(QB_NEW_EN)
[grammar] ~115-~115: There might be a mistake here.
Context: ...incode nodes are included in the output.
(QB_NEW_EN)
docs/dependency-management.md
[grammar] ~32-~32: There might be a mistake here.
Context: ...ies ## Process 1. Collect local types from main file (automatically marked as requ...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...s import chains recursively (adds types to graph) 3. Extract type dependencies (ma...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...ilter to only connected dependencies 5. Build dependency graph 6. Topological sort fo...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...: Always included (defined in main file) - Imported types: Only included if actua...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...ed if actually referenced by other types - Transitive dependencies: Automatically...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
docs/maincode-filtering.md
18-18: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
30-30: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
76-76: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
84-84: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
93-93: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (6)
docs/overview.md (1)
51-51: Nice addition; link placement reads wellThe new entry fits the flow between dependency analysis and testing.
src/traverse/dependency-extractor.ts (1)
53-54: Edge direction is correct for topo-sortRecording edges as
dependency → dependentaligns with stable topological ordering. LGTM.src/traverse/import-collector.ts (1)
19-22: Good cycle guardTracking visited files in
FileGraphavoids infinite recursion through re-exports/import loops. LGTM.tests/traverse/maincode-filter.test.ts (1)
12-116: Great end-to-end validation of maincode filteringCovers unused imports, dependency chains, and expected emission shapes. LGTM.
tests/traverse/non-transitive-dependency.test.ts (1)
12-73: Solid non-transitive filtering coverageGood assertion that unrelated exports from imported files are excluded. LGTM.
src/traverse/dependency-traversal.ts (1)
29-31: Insert precondition check for missing requiredNodeIds
Above the call toextractDependencies, add:const missing = this.requiredNodeIds.filter(id => !this.nodeGraph.hasNode(id)); if (missing.length) throw new Error(`Unknown node IDs: ${missing.join(', ')}`);And if you still prefer a shell‐based check, run a full build (
npm run build) and point your script at the realdist/traverse/dependency-traversal.jsbefore executing it.
| ## Dependency Filtering | ||
|
|
||
| The system now filters out unconnected dependencies to optimize output: | ||
|
|
||
| - **Local types**: Always included (defined in main file) | ||
| - **Imported types**: Only included if actually referenced by other types | ||
| - **Transitive dependencies**: Automatically included when their parent types are referenced | ||
|
|
There was a problem hiding this comment.
Docs/code mismatch: imported types are “referenced-only” in docs, but code includes all imports when the entry file has no local types
addLocalTypes currently marks every named import as required if the main file defines no local types, which contradicts this section (“Imported types: Only included if actually referenced”). Please align behavior or document the special-case explicitly.
Would you like to (A) change the code to only mark imported types as required when actually referenced, or (B) keep the current import-only seeding and document it as an exception?
🧰 Tools
🪛 LanguageTool
[grammar] ~43-~43: There might be a mistake here.
Context: ...: Always included (defined in main file) - Imported types: Only included if actua...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...ed if actually referenced by other types - Transitive dependencies: Automatically...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In docs/dependency-management.md around lines 39 to 46, the docs state imported
types should only be included if actually referenced, but addLocalTypes
currently marks every named import as required when the entry file has no local
types; change addLocalTypes so it does not unconditionally mark all named
imports required in that special-case—instead, detect actual references and mark
an imported type required only if it is referenced by other types (or by an
explicit option), update related tests to assert the referenced-only behavior,
and adjust or add a feature-flag/option if you want to preserve the previous
import-seeding behavior for backwards compatibility.
| const typeReferences = typeReferenceExtractor.extractTypeReferences(nodeToAnalyze) | ||
|
|
||
| for (const referencedType of typeReferences) { | ||
| if (nodeGraph.hasNode(referencedType)) { | ||
| // Only add to required if not already processed | ||
| if (!requiredNodeIds.has(referencedType)) { | ||
| requiredNodeIds.add(referencedType) | ||
| nodesToProcess.add(referencedType) | ||
| } | ||
| nodeGraph.addDependency(referencedType, currentNodeId) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Name resolution is ambiguous across files with duplicate type names
TypeReferenceExtractor matches by originalName only, which can mis-resolve when multiple files export the same identifier (e.g., User). Consider symbol-based resolution via ts-morph TypeChecker or maintaining an import-binding map (specifier + local name → qualified node id) to disambiguate.
Happy to sketch a small resolver that uses getType().getSymbol() and getAliasedSymbol() to map references to the correct source file.
🤖 Prompt for AI Agents
In src/traverse/dependency-extractor.ts around lines 44 to 54, the extractor
currently matches type references by originalName only which can mis-resolve
when different files export the same identifier; change the resolution to use
symbol-based disambiguation: use ts-morph/TypeChecker (e.g.,
nodeToAnalyze.getType().getSymbol() and getAliasedSymbol() or
checker.getSymbolAtLocation on the type reference node) to obtain the symbol and
derive a unique file-qualified identifier (source file path + exported name)
before looking up nodeGraph; as an alternative or complement, build an
import-binding map during file parsing that maps (specifier + local name) →
qualified node id and consult it when a reference originates from an import;
update requiredNodeIds/nodesToProcess logic to use the qualified id returned by
the symbol/import resolver instead of originalName so duplicate names across
files are resolved correctly.
| const importDeclarations = sourceFile.getImportDeclarations() | ||
| for (const importDecl of importDeclarations) { | ||
| const namedImports = importDecl.getNamedImports() | ||
| for (const namedImport of namedImports) { | ||
| const importName = namedImport.getName() | ||
| const importSourceFile = importDecl.getModuleSpecifierSourceFile() | ||
| if (importSourceFile) { | ||
| const qualifiedName = generateQualifiedNodeName(importName, importSourceFile) | ||
| requiredNodeIds.add(qualifiedName) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrong file used to qualify imported symbols; breaks re-exports and generates mismatched IDs
Using importDecl.getModuleSpecifierSourceFile() to build the qualified name fails when a symbol is re-exported from an index or barrel. Resolve the named import to its declaration and use that file instead. Keep current behavior otherwise.
-import { SourceFile } from 'ts-morph'
+import { SourceFile, Node } from 'ts-morph'
@@
- const importDeclarations = sourceFile.getImportDeclarations()
- for (const importDecl of importDeclarations) {
- const namedImports = importDecl.getNamedImports()
- for (const namedImport of namedImports) {
- const importName = namedImport.getName()
- const importSourceFile = importDecl.getModuleSpecifierSourceFile()
- if (importSourceFile) {
- const qualifiedName = generateQualifiedNodeName(importName, importSourceFile)
- requiredNodeIds.add(qualifiedName)
- }
- }
- }
+ const importDeclarations = sourceFile.getImportDeclarations()
+ for (const importDecl of importDeclarations) {
+ for (const namedImport of importDecl.getNamedImports()) {
+ const importName = namedImport.getName()
+ const sym =
+ namedImport.getAliasSymbol() ?? namedImport.getNameNode().getSymbol()
+ const decl = sym?.getDeclarations()?.find(
+ (d) =>
+ Node.isTypeAliasDeclaration(d) ||
+ Node.isInterfaceDeclaration(d) ||
+ Node.isEnumDeclaration(d) ||
+ Node.isFunctionDeclaration(d),
+ )
+ if (!decl) continue
+ const qualifiedName = generateQualifiedNodeName(importName, decl.getSourceFile())
+ // NOTE: See comment below for whether to add to required here.
+ requiredNodeIds.add(qualifiedName)
+ }
+ }Follow-up: If you want strict “referenced-only” inclusion (as in docs/PR summary), don’t add to requiredNodeIds here; let dependency extraction mark them when referenced. I can provide a clean diff for that variant as well.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/traverse/local-type-collector.ts around lines 23 to 36, the code uses
importDecl.getModuleSpecifierSourceFile() to build qualified names which breaks
when symbols are re-exported; instead resolve each named import to its symbol
and use the symbol's declaration source file when computing
generateQualifiedNodeName, falling back to
importDecl.getModuleSpecifierSourceFile() only if resolution fails; keep adding
the resulting qualifiedName to requiredNodeIds as today (or, if you prefer the
“referenced-only” variant, remove this addition and let dependency extraction
handle it).
| if (Node.isTypeReference(node)) { | ||
| const typeName = node.getTypeName().getText() | ||
|
|
||
| for (const qualifiedName of this.nodeGraph.nodes()) { | ||
| const nodeData = this.nodeGraph.getNode(qualifiedName) | ||
| if (nodeData.originalName === typeName) { | ||
| references.push(qualifiedName) | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resolve references via symbol, and normalize qualified names — current string match misses aliases/namespaces and is O(N²).
- Aliased imports (import { Foo as Bar }) and namespaced refs (NS.Foo) won’t match nodeData.originalName via plain text.
- Scanning this.nodeGraph.nodes() per reference is O(N²).
Use symbol resolution when available; fall back to rightmost identifier text. Also support QualifiedName/PropertyAccess in extends/typeof.
Patch within these ranges to normalize names (minimal change, still uses current lookup). Then consider the helper below.
- if (Node.isTypeReference(node)) {
- const typeName = node.getTypeName().getText()
+ if (Node.isTypeReference(node)) {
+ const tn = node.getTypeName()
+ const typeName = Node.isQualifiedName(tn) ? tn.getRight().getText() : tn.getText()
for (const qualifiedName of this.nodeGraph.nodes()) {
const nodeData = this.nodeGraph.getNode(qualifiedName)
if (nodeData.originalName === typeName) {
references.push(qualifiedName)
break
}
}
}- if (Node.isIdentifier(exprName) || Node.isQualifiedName(exprName)) {
- const typeName = exprName.getText()
+ if (
+ Node.isIdentifier(exprName) ||
+ Node.isQualifiedName(exprName) ||
+ Node.isPropertyAccessExpression(exprName)
+ ) {
+ const typeName = Node.isQualifiedName(exprName)
+ ? exprName.getRight().getText()
+ : exprName.getText().split('.').pop()!
for (const qualifiedName of this.nodeGraph.nodes()) {
const nodeData = this.nodeGraph.getNode(qualifiedName)
if (nodeData.originalName === typeName) {
references.push(qualifiedName)
break
}
}
}- } else if (Node.isExpressionWithTypeArguments(typeNode)) {
+ } else if (Node.isExpressionWithTypeArguments(typeNode)) {
// Handle ExpressionWithTypeArguments (e.g., EntityInfo<PropertyId>)
const expression = typeNode.getExpression()
- if (Node.isIdentifier(expression)) {
- const baseTypeName = expression.getText()
+ if (
+ Node.isIdentifier(expression) ||
+ Node.isQualifiedName(expression) ||
+ Node.isPropertyAccessExpression(expression)
+ ) {
+ const baseTypeName = Node.isQualifiedName(expression)
+ ? expression.getRight().getText()
+ : expression.getText().split('.').pop()!
for (const qualifiedName of this.nodeGraph.nodes()) {
const nodeData = this.nodeGraph.getNode(qualifiedName)
if (nodeData.originalName === baseTypeName) {
references.push(qualifiedName)
break
}
}
}Optional but recommended (outside the selected ranges): introduce symbol-based resolution + a prebuilt lookup to avoid full scans.
// Inside the class
private readonly declToId = new Map<Node, string>()
private readonly nameIndex = new Map<string, string[]>()
constructor(private nodeGraph: NodeGraph) {
for (const id of this.nodeGraph.nodes()) {
const data = this.nodeGraph.getNode(id)
this.declToId.set(data.node, id)
const arr = this.nameIndex.get(data.originalName) ?? []
arr.push(id)
this.nameIndex.set(data.originalName, arr)
}
}
private addBySymbolOrName(references: string[], node: Node, fallbackName: string): void {
// Prefer symbol-based resolution (handles aliases/namespaces/re-exports)
const symbol =
(node as any).getSymbol?.() ??
(node as any).getType?.()?.getSymbol?.() ??
(node as any).getType?.()?.getAliasSymbol?.()
if (symbol) {
for (const decl of symbol.getDeclarations() ?? []) {
const id = this.declToId.get(decl as unknown as Node)
if (id) {
references.push(id)
return
}
}
}
// Fallback to name index with rightmost identifier
const simple = fallbackName.split('.').pop()!
for (const id of this.nameIndex.get(simple) ?? []) references.push(id)
}Then, within the three patched blocks above, replace the inner for-loop with:
this.addBySymbolOrName(references, tn, typeName) // for TypeReference
this.addBySymbolOrName(references, exprName, typeName) // for typeof
this.addBySymbolOrName(references, expression, baseTypeName) // for extendsAlso applies to: 31-41, 74-84
🤖 Prompt for AI Agents
In src/traverse/type-reference-extractor.ts around lines 15 to 25, the code
matches TypeReference nodes by comparing node.getTypeName().getText() to
nodeData.originalName by scanning this.nodeGraph.nodes(), which misses
aliased/namespace/re-exported symbols and is O(N²); replace the inner loop with
a symbol-aware resolution and normalized name lookup: build decl->id and
name->ids maps in the class constructor, implement a helper addBySymbolOrName
that first attempts to get a symbol (via
getSymbol/getType().getSymbol/getType().getAliasSymbol) and map its declarations
to node ids using declToId, and if that fails fall back to the rightmost
identifier text and lookup in nameIndex; then call
this.addBySymbolOrName(references, tn, typeName) here (and similarly replace the
analogous loops at lines 31-41 and 74-84 with calls to the helper).
| // Handle call expressions (for generic type calls like EntityInfo(PropertyId)) | ||
| if (Node.isCallExpression(node)) { | ||
| const expression = node.getExpression() | ||
|
|
||
| if (Node.isIdentifier(expression)) { | ||
| const typeName = expression.getText() | ||
|
|
||
| for (const qualifiedName of this.nodeGraph.nodes()) { | ||
| const nodeData = this.nodeGraph.getNode(qualifiedName) | ||
| if (nodeData.originalName === typeName) { | ||
| references.push(qualifiedName) | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove CallExpression handling — it creates false-positive dependencies from runtime code.
CallExpression is value-level. Traversing function bodies or enum initializers will incorrectly mark called functions as type dependencies (e.g., foo() becomes a “type” ref). This bloats requiredNodeIds and violates the PR goal of “only if actually referenced by other types.”
Apply this diff to drop the block:
- // Handle call expressions (for generic type calls like EntityInfo(PropertyId))
- if (Node.isCallExpression(node)) {
- const expression = node.getExpression()
-
- if (Node.isIdentifier(expression)) {
- const typeName = expression.getText()
-
- for (const qualifiedName of this.nodeGraph.nodes()) {
- const nodeData = this.nodeGraph.getNode(qualifiedName)
- if (nodeData.originalName === typeName) {
- references.push(qualifiedName)
- break
- }
- }
- }
- }📝 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.
| // Handle call expressions (for generic type calls like EntityInfo(PropertyId)) | |
| if (Node.isCallExpression(node)) { | |
| const expression = node.getExpression() | |
| if (Node.isIdentifier(expression)) { | |
| const typeName = expression.getText() | |
| for (const qualifiedName of this.nodeGraph.nodes()) { | |
| const nodeData = this.nodeGraph.getNode(qualifiedName) | |
| if (nodeData.originalName === typeName) { | |
| references.push(qualifiedName) | |
| break | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/traverse/type-reference-extractor.ts around lines 97 to 112, remove the
entire CallExpression handling block (the Node.isCallExpression branch and its
inner logic) that inspects expression identifiers and pushes qualified names
into references; delete the surrounding comment about generic type calls as
well. Ensure no other code paths expect references to be populated from runtime
CallExpressions and run tests/TS compile to confirm no remaining references to
that branch remain.
| expect(generateFormattedCode(sourceFile)).toBe( | ||
| formatWithPrettier(` | ||
| export const User = Type.Object({ | ||
| id: Type.String(), | ||
| name: Type.String(), | ||
| }); | ||
| export type User = Static<typeof User>; | ||
| export const LocalType = Type.Object({ | ||
| user: User, | ||
| userAlias: UserAlias, | ||
| }); | ||
| export type LocalType = Static<typeof LocalType>; | ||
| `), | ||
| ) |
There was a problem hiding this comment.
Unresolved identifier in expected output: UserAlias is referenced but never defined
The expected code uses userAlias: UserAlias without emitting a UserAlias schema alias. Either:
- Normalize alias usage to the canonical symbol (
User), or - Emit
export const UserAlias = User;and itsStatictype alias.
I recommend normalizing to User in object properties to avoid duplicate symbols.
- export const LocalType = Type.Object({
- user: User,
- userAlias: UserAlias,
- });
+ export const LocalType = Type.Object({
+ user: User,
+ userAlias: User,
+ });📝 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.
| expect(generateFormattedCode(sourceFile)).toBe( | |
| formatWithPrettier(` | |
| export const User = Type.Object({ | |
| id: Type.String(), | |
| name: Type.String(), | |
| }); | |
| export type User = Static<typeof User>; | |
| export const LocalType = Type.Object({ | |
| user: User, | |
| userAlias: UserAlias, | |
| }); | |
| export type LocalType = Static<typeof LocalType>; | |
| `), | |
| ) | |
| expect(generateFormattedCode(sourceFile)).toBe( | |
| formatWithPrettier(` | |
| export const User = Type.Object({ | |
| id: Type.String(), | |
| name: Type.String(), | |
| }); | |
| export type User = Static<typeof User>; | |
| export const LocalType = Type.Object({ | |
| user: User, | |
| userAlias: User, | |
| }); | |
| export type LocalType = Static<typeof LocalType>; | |
| `), | |
| ) |
🤖 Prompt for AI Agents
In tests/traverse/dependency-traversal.integration.test.ts around lines 216 to
232 the expected output references an undefined symbol `UserAlias` causing an
unresolved identifier; update the expected formatted string so the object
property uses the canonical symbol (`userAlias: User`) instead of `UserAlias`,
and remove any lines that assume an emitted alias (or alternatively emit `export
const UserAlias = User;` plus its `Static` type if you prefer aliasing) — prefer
normalizing to `User` to avoid duplicate symbols.
This commit introduces several improvements to the dependency management system:
The key changes are:
maincodeNodeIdsandrequiredNodeIdssets to track main code and required nodesaddLocalTypesto mark local types as both main code and requiredcollectFromImportsto only mark imported types as required if they are referencedgetNodesToPrintto return only the required nodesThese changes ensure the dependency analysis produces a more optimized output, only including the necessary types and their transitive dependencies.