perf(move-file): cache index file export analysis #316
Conversation
LayZeeDK
changed the title
github-actions
bot
force-pushed
the
feat/161-index-exports-cache
branch
from
70f97e9 to
a188b59
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds a caching mechanism for index file export analysis to improve performance in move-file operations by avoiding repeated regex parsing of entrypoint files.
- Introduces an index exports cache that stores parsed export/re-export specifiers with content-based invalidation
- Replaces per-call regex compilation with cached export analysis for ~9-11% performance improvement
- Maintains existing API compatibility with no changes to public interfaces or test behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| index-exports-cache.ts | New cache implementation with regex-based export parsing and content-keyed invalidation |
| is-file-exported.ts | Updated to use cached export analysis instead of inline regex compilation |
| updated-export-bench.txt | Performance benchmark results showing improved throughput |
| baseline-export-bench.txt | Baseline performance metrics for comparison |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const spec of reexports) { | ||
| const withoutExt = spec.replace(/\.(ts|tsx|js|jsx)$/i, ''); | ||
| // Ensure stored paths are normalized to start with './' | ||
| const normalized = | ||
| withoutExt.startsWith('./') || withoutExt.startsWith('../') | ||
| ? withoutExt | ||
| : `./${withoutExt}`; | ||
| exports.add(normalized); | ||
| } |
There was a problem hiding this comment.
The logic adds reexports to the exports set, which conflates two different types of exports. The reexports should remain in the reexports set, and exports should contain actual local exports from the file.
| for (const spec of reexports) { | |
| const withoutExt = spec.replace(/\.(ts|tsx|js|jsx)$/i, ''); | |
| // Ensure stored paths are normalized to start with './' | |
| const normalized = | |
| withoutExt.startsWith('./') || withoutExt.startsWith('../') | |
| ? withoutExt | |
| : `./${withoutExt}`; | |
| exports.add(normalized); | |
| } | |
| // Note: Do not add reexports to exports. Exports should only contain actual local exports. | |
| // If needed, add logic here to capture local exports (e.g., export { foo, bar };) in the future. |
|
|
||
| // Match: export * from './path'; OR export { ... } from './path'; OR export {default as X} from './path'; | ||
| const reExportPattern = | ||
| /export\s+(?:\*|\{[^}]+\})\s+from\s+['"](\.\.?\/[^'";]+)['"];?/g; |
There was a problem hiding this comment.
The regex pattern allows paths that don't start with './' or '../' by using \.\.?\/ which makes the second dot optional. This could match invalid paths like ./x incorrectly. The pattern should be (\.\.?\/[^'";]+) or more specifically (\.(?:\.\/|\/)[^'";]+) to properly match relative paths.
| /export\s+(?:\*|\{[^}]+\})\s+from\s+['"](\.\.?\/[^'";]+)['"];?/g; | |
| /export\s+(?:\*|\{[^}]+\})\s+from\s+['"](\.(?:\.\/|\/)[^'";]+)['"];?/g; |
| for (const spec of reexports) { | ||
| const withoutExt = spec.replace(/\.(ts|tsx|js|jsx)$/i, ''); |
There was a problem hiding this comment.
The file extension removal logic duplicates functionality that likely exists elsewhere in the codebase (e.g., removeSourceFileExtension that's imported in the other file). Consider extracting this to a shared utility or reusing existing functionality.
|
Applied review feedback:
All tests & lint pass. |
Implementing index export cache to parse local exports in addition to
re-exports
## Plan
- [x] Create IndexExports type and interface in export-management
- [x] Create getIndexExports function to parse local exports using AST
- [x] Update isFileExported to use the new cache system
- [x] Add comprehensive unit tests for getIndexExports
- [x] Add benchmark tests for performance validation
- [x] Update existing tests to cover local export scenarios
- [x] Run linting, build, and all tests
- [x] Format code
- [x] Update documentation
- [x] Address code review feedback
- [x] Update JSDoc comments to reflect cache usage
## Recent Changes
Updated documentation in `isFileExported()` to clarify:
- Function now uses the index exports cache system
- Cache parses local declarations but function only checks re-exports
- This addresses review feedback about outdated documentation
The distinction is important: `isFileExported()` checks if a **file** is
re-exported (e.g., `export * from './lib/utils'`), not if **symbols**
from that file are present as local exports in the index.
<!-- START COPILOT CODING AGENT SUFFIX -->
<details>
<summary>Original prompt</summary>
----
*This section details on the original issue you should resolve*
<issue_title>feat(move-file): parse local exports in index export
cache</issue_title>
<issue_description>## Summary
Enhance the index exports cache to parse and record *local* export
declarations in entrypoint (index) files in addition to existing
re-export ("export ... from") patterns.
Currently `getIndexExports()` only captures reexports and leaves
`exports` empty, forcing callers (e.g. `isFileExported`) to rely solely
on `reexports`. This prevents accurate detection of locally defined
exports (e.g. `export { foo, bar }`, `export const X = ...`, `export
function fn() {}`, `export class C {}`, and `export default ...`).
Supporting these will improve correctness of move-file validation
(unexported dependency checks, alias conversions) and reduce false
warnings.
## Motivation / Problem
When a project’s public surface is defined via local declarations in
`index.ts` rather than re-exporting from leaf modules, the current cache
misses them. As a result:
- `isFileExported()` incorrectly reports a file as unexported if it’s
aggregated via local declarations copied into the index.
- Move-file generator may warn about supposedly unexported relative
dependencies.
- Potential future optimizations (e.g., differentiating default vs named
exports) are blocked.
## Scope
Add robust parsing of local export declarations for
TypeScript/JavaScript index files, keeping the lightweight approach
(regex or jscodeshift) while maintaining performance. Preserve
separation between `exports` and `reexports` (per review feedback in PR
#316).
## Proposed Design
1. Reuse existing AST caching (jscodeshift + `astCache`) for structural
accuracy while minimizing parse cost.
2. Collect and normalize local export specifiers:
- Named declarations: `export const A`, `export function b`, `export
class C`, `export interface D`, `export type E`, `export enum F`.
- Named list: `export { a, b as c }` (without `from`).
- Default exports: `export default X`, `export default function() {}`,
`export default class {}`.
3. Represent cache structure:
```ts
interface IndexExports {
exports: Set<string>; // local named export identifiers (normalized)
reexports: Set<string>; // existing re-export specifiers ('./lib/util')
defaultExport?: string; // identifier or synthetic marker
('<anonymous>') if unnamed default
}
```
4. Normalization Rules:
- Local identifiers stored exactly as defined (no extension logic
needed).
- Anonymous default exports mapped to a sentinel (e.g. `<default>` or
`<anonymous-default>`).
5. Backwards compatibility: existing callers using `reexports` remain
unchanged; add helper `isLocalSymbolExported(name)` if needed later.
6. Performance Strategy:
- Parse AST once via `astCache.getAST`; fall back to regex only if AST
unavailable.
- Skip expensive symbol resolution—pure syntactic discovery.
- Maintain content snapshot invalidation (existing logic).
7. Update `isFileExported` logic:
- If import specifier points to a file path: keep current re-export
check.
- If validation wants to infer export of an internal symbol (future),
can consult `exports` set.
## Tasks
- [ ] Extend `IndexExports` interface (add `defaultExport` field).
- [ ] Implement local export parsing function
(`collectLocalExports(ast): { names: Set<string>; default?: string }`).
- [ ] Integrate parsing into `getIndexExports` (only when content
contains `export`).
- [ ] Add unit tests covering:
- Basic named exports (const/function/class/interface/type/enum).
- Named list / alias exports (with `as`).
- Default exports (named and anonymous).
- Mixed local + re-exports in same file.
- Empty index file (returns empty sets, undefined default).
- Cache hit behavior (content unchanged).
- Cache invalidation (content change updates sets).
- [ ] Update `is-file-exported.spec.ts` to include a scenario with local
declarations.
- [ ] Add benchmark cases to `export-management.bench.ts` for local
export detection (ensure negligible regression; target <5% throughput
impact).
- [ ] Update PR/Documentation: mention difference between local exports
and reexports.
## Acceptance Criteria
- All new tests pass; existing 770 tests remain green.
- Benchmarks show <5% performance degradation for export detection suite
or an offsetting improvement if AST parsing reduces regex overhead.
- `isFileExported` correctly returns true for files represented by local
declarations (when those files exist or are logically aggregated).
- No conflation between `exports` and `reexports`; clear docs in
`index-exports-cache.ts`.
## Performance Considerations
- AST parsing is already cached; incremental cost is node filtering for
export declarations.
- Provide fallback to fast regex ONLY if AST is unavailable (e.g., parse
error).
- Keep operations allocation-light (pre-size arrays where possible,
avoid repeated string normaliz...
</details>
- Fixes #317
<!-- START COPILOT CODING AGENT TIPS -->
---
💡 You can make Copilot smarter by setting up custom instructions,
customizing its development environment and configuring Model Context
Protocol (MCP) servers. Learn more [Copilot coding agent
tips](https://gh.io/copilot-coding-agent-tips) in the docs.
|
/review |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const indexExports = getIndexExports(tree, indexPath); | ||
| // Compare against file path without extension (as stored) | ||
| // Since local exports are not yet collected, rely on reexports for detection. | ||
| return indexExports.reexports.has(`./${fileWithoutExt}`); |
There was a problem hiding this comment.
The export check always prefixes with ./, but the cached reexports may contain paths starting with ../ (parent directory references). This will cause false negatives when checking if files exported via parent-relative paths (e.g., export * from '../sibling/file') are detected. The path normalization should account for both ./ and ../ patterns or normalize paths consistently before comparison.
| let match: RegExpExecArray | null; | ||
| while ((match = reExportPattern.exec(content))) { | ||
| const spec = match[1]; | ||
| reexports.add(spec); |
There was a problem hiding this comment.
The captured path specifiers are stored as-is without removing file extensions, but fileWithoutExt in is-file-exported.ts has extensions removed. This creates an inconsistency: if index files contain export * from './utils.ts', the cache stores './utils.ts' but the comparison checks for './utils'. Either strip extensions when storing in cache or when comparing, ensuring both sides match.
| reexports.add(spec); | |
| // Remove file extension (e.g., .ts, .js, .jsx, .tsx) from specifier | |
| const specWithoutExt = spec.replace(/\.[tj]sx?$/, ''); | |
| reexports.add(specWithoutExt); |
| // Use cached export analysis for index file | ||
| const indexExports = getIndexExports(tree, indexPath); | ||
| // Compare against file path without extension (as stored) | ||
| // Since local exports are not yet collected, rely on reexports for detection. |
There was a problem hiding this comment.
The comment on line 47 states 'Since local exports are not yet collected, rely on reexports for detection' but doesn't explain why local exports are excluded or what impact this has. Add clarification that this currently only detects re-exported files (export...from statements) and won't detect files with direct exports in the index that don't use re-export syntax.
| // Since local exports are not yet collected, rely on reexports for detection. | |
| // Since local exports are not yet collected, rely on reexports for detection. | |
| // NOTE: This currently only detects files that are re-exported using `export ... from` statements. | |
| // Files that are directly exported in the index (without using re-export syntax) will NOT be detected. |
PR Review: Cache Index File Export Analysis (#316)SummaryThis PR introduces a performance optimization for the ✅ Code Quality & Best PracticesExcellent:
Cache Invalidation:
🔍 Technical Review1. Implementation ApproachThe PR switched from regex-based parsing (in the diff) to AST-based parsing (in the actual code), which is the superior approach: Pros of AST-based approach:
Note: The PR diff shows a regex-based implementation, but the actual merged code uses AST parsing via jscodeshift. This is a significant improvement over what the diff suggests. 2. Cache Key StrategyThe cache uses file paths as keys, relying on the existing export function getIndexExports(tree: Tree, indexPath: string): IndexExports {
const cached = indexExportsCache.get(indexPath);
if (cached) return cached;
const root = astCache.getAST(tree, indexPath); // Handles content changes
// ... parse and cache
}This is correct because:
3. Performance ImpactMeasured improvements from benchmarks:
These are meaningful improvements for a frequently-called operation during file moves. 🐛 Potential IssuesCritical: Path Normalization InconsistencyThere's a subtle bug in for (const reexport of indexExports.reexports) {
const normalizedReexport = reexport.replace(/^\.\.?\//, ''); // Removes './' or '../'
const reexportWithoutExt = removeSourceFileExtension(normalizedReexport);
if (reexportWithoutExt === fileWithoutExt) { // Bug: doesn't handle '../' properly
return true;
}
}Problem: When comparing paths, this doesn't account for parent directory traversal ( Recommendation: // Option 1: Normalize both paths to absolute workspace paths before comparing
const absoluteReexport = path.join(path.dirname(indexPath), reexport);
const absoluteFile = path.join(project.root, file);
// Option 2: Use the existing path-utils helpers more carefully
// Ensure consistent resolution relative to the index file locationMinor: Missing Parent Directory Path HandlingThe regex
But doesn't handle:
Recommendation: Use a more robust path normalization utility or clarify in comments that deeply nested relative imports aren't expected in index files. 🔒 Security ConcernsLow Risk: The implementation uses the existing 📊 Test CoverageCoverage Analysis:
Recommendation: Add at least one integration test showing the cache being used across multiple 🚀 Performance ConsiderationsStrengths:
Potential Concern:
🎯 Recommendations
📝 PR Metadata ReviewTitle: ✅ Follows Conventional Commits ( ✅ Final VerdictApproval: APPROVE with minor fixes recommended This is a well-engineered performance optimization with:
The path normalization issue should be addressed before merging, but it's a localized fix that doesn't undermine the overall approach. Great work! This demonstrates strong attention to testing, performance measurement, and maintainability. 📎 References
|
perf(move-file): cache index file export analysis (#161)
Summary
Adds an index exports cache to avoid reparsing entrypoint (index) files for each export check during move-file operations. Replaces repeated regex scans with a cached, content-keyed extraction of export/re-export specifiers.
Implementation
Benchmarks (ran 2025-10-29T20:53:33Z, updated at 2025-10-29T20:55:40.852Z)
Baseline (before):
Updated (after):
Net Effect
Risk / Mitigation
Follow-ups (optional)
Closes #161.