build(prune-dts): migrate to ts-morph - #10220
Conversation
|
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the prune-dts script by migrating from the TypeScript Compiler API to ts-morph and modularizing the pruning logic into individual rules. It also updates the test suite, adds Prettier formatting to the output, and introduces a .d.ts comparison script. The review feedback is highly constructive, pointing out critical bugs where modifying heritage clauses during iteration causes ForgottenNodeExceptions, and identifying an O(N^2) performance bottleneck in type substitution that can be resolved by pre-computing subclass mappings. Additionally, the feedback suggests improving JSDoc cleaning to preserve formatting asterisks, grouping duplicate imports in cross-file deduplication, and adding a git cleanliness check to the comparison script.
ca520b3 to
8e4700a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the prune-dts script, migrating it from the TypeScript Compiler API to a modular, rule-based pipeline using ts-morph. It introduces dedicated AST transformation rules for constructor visibility, cross-file deduplication, inheritance flattening, private member stripping, and top-level declaration filtering. Additionally, it integrates Prettier formatting for the generated .d.ts files and expands the test suite with unit and package regression tests. The review feedback highlights two important improvements: targeting only the type name node during type substitution to prevent overlapping replacements with nested generic types, and normalizing Windows path separators to forward slashes in cross-file deduplication imports to avoid compilation errors on Windows platforms.
| @@ -21,9 +21,12 @@ import * as path from 'path'; | |||
| import { Extractor, ExtractorConfig } from 'api-extractor-me'; | |||
There was a problem hiding this comment.
Small (large) request: Would you be able to replace this (including replacing the dependency in package.json) with the latest version of @microsoft/api-extractor?
2 participants
Migrates the
prune-dtsbuild script from the raw TypeScript Compiler API tots-morphv28.0.0.The current script has subtle AST bugs, is difficult to maintain, and significantly increases the complexity of TypeScript version upgrades due to breaking changes in compiler internals.
The new
ts-morphscript resolves known bugs, improves maintainability via a modular architecture and a stable semver library, and unblocks future TypeScript upgrades. Going forward, upgrading TypeScript will only require addressing new compiler type errors rather than debugging and fixing AST manipulation logic.Implements go/prunedts-maintenance-strategy.
Most of the lines changed are
.d.tstest snapshots and formatting changes in reference docs. The real diff is ~2k lines added, ~800 deleted`.Changes
prune-dts.tsscript with a modular script that executes single-responsibility AST manipulation rules insrc/rules, with a top-levelsrc/pipeline.ts..d.tsfiles generated from real builds) with new snapshots generated from this branch..d.tstest snapshots.Notes
ts-morphhas known AST wrapper overhead, so script execution runtime scales with.d.tsfile size. This build-step overhead is a worthwhile trade-off for long-term maintainability and easier TypeScript upgrades.Semantic Diffs
1.(Bug Fix)
CollectionStageOptions: ChangedQuery->CollectionReferencein firestore-pipelines. This change is a result of a bug fix. The legacy script replacedCollectionReferencewith its base classQuery. SinceCollectionReferenceis a public API, this was a bug that rendered the previous type non-functional. The new script fixes the bug, and the type is now functional.2. Additional unused imports pruned: The legacy script retained 5 unused imports not referenced in the public API. The new script removes them.
Validation
It is critical that this migration results in no semantic changes in the released
.d.tsfiles. One way to do this is to run a build on main, and on this branch and comb through the diffs. Unfortunately, there is a lot of noise from formatting changes (lines over length 80 have been folded, blank line changes, etc). To make it easier to review, this PR shows the.d.tsfile diffs after a common formatter config has ran over the files: dlarocque/dts-diffs#2 - note that there is still some formatting noise.