fix: strip type exports upon DCE #6582
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughConsolidates AST dead-code utilities into router-utils: adds stripTypeExports and a deadCodeElimination wrapper, moves babel-dead-code-elimination into router-utils, updates imports in plugins to use the new utilities, and adds tests for TypeScript type-only AST stripping. Dependency entries adjusted in three packages. Changes
Sequence DiagramsequenceDiagram
participant Plugin as Plugin
participant AST as AST (file)
participant Strip as stripTypeExports()
participant Wrapper as deadCodeElimination()
participant Babel as babel-dead-code-elimination
Plugin->>AST: provide parsed AST (may include TS-only nodes)
Plugin->>Strip: call stripTypeExports(AST)
Strip-->>AST: remove type-only imports/exports & top-level types
Plugin->>Wrapper: call deadCodeElimination(AST, candidates?)
Wrapper->>Babel: delegate cleaned AST (+ candidates)
Babel-->>AST: eliminate unreferenced identifiers
AST-->>Plugin: return cleaned AST
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
schiller-manuel
force-pushed
the
fix-6480
branch
from
36182a2 to
dcd79e6
Compare
|
View your CI Pipeline Execution ↗ for commit d6728db
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/router-plugin/src/core/code-splitter/compilers.ts`:
- Around line 4-9: The import member order in compilers.ts violates
sort-imports; reorder the named imports from '@tanstack/router-utils' so they
are alphabetically sorted (deadCodeElimination, findReferencedIdentifiers,
generateFromAst, parseAst) to satisfy the linter; update the import line that
currently lists generateFromAst, parseAst, deadCodeElimination,
findReferencedIdentifiers accordingly.
In `@packages/router-utils/src/ast.ts`:
- Around line 78-128: The stripTypeExports logic currently misses type-only
export-all statements; inside the same traversal/function (stripTypeExports) add
a handler for Babel's ExportAllDeclaration nodes and return false when
node.exportKind === 'type' so that `export type * from '...'` is removed just
like the ImportDeclaration and ExportNamedDeclaration cases (refer to
ExportAllDeclaration and the stripTypeExports function to locate where to add
this check).
- Around line 150-153: The `candidates` parameter of deadCodeElimination is
currently typed as Set<any>; change it to the exact return type of
findReferencedIdentifiers (e.g., Set<...>) by using ReturnType<typeof
findReferencedIdentifiers> (or the concrete Set element type returned) so the
signature becomes deadCodeElimination(ast: ParseResult<_babel_types.File>,
candidates?: ReturnType<typeof findReferencedIdentifiers>): void; update any
internal usages assuming the stronger type and import/find the
findReferencedIdentifiers symbol if needed to ensure the type resolves
correctly.
In `@packages/router-utils/tests/stripTypeExports.test.ts`:
- Around line 1-2: Reorder the named imports so they satisfy the `sort-imports`
rule: in the first import keep the test functions (describe, expect, test) as-is
if already sorted, and in the second import put the specifiers in alphabetical
order — change the import list from parseAst, generateFromAst, stripTypeExports
to generateFromAst, parseAst, stripTypeExports so the named imports for
parseAst/generateFromAst/stripTypeExports are sorted.
In `@packages/start-plugin-core/src/start-compiler-plugin/compiler.ts`:
- Around line 4-10: Reorder the named imports from '@tanstack/router-utils' in
compiler.ts to satisfy the sort-imports rule: place the specifiers
alphabetically as deadCodeElimination, findReferencedIdentifiers,
generateFromAst, parseAst (so the import line uses that order) and keep the
default babel import as-is.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/router-utils/package.json`:
- Around line 66-71: Update the dependency "babel-dead-code-elimination" in
package.json from "^1.0.11" to "^1.0.12" (update the version specifier for the
package name "babel-dead-code-elimination"), then regenerate the lockfile by
running your package manager (npm install or yarn install) so transitive deps
resolve to the newer safe versions; commit the updated package.json and
lockfile.
- Bump babel-dead-code-elimination from ^1.0.11 to ^1.0.12 - Sort imports alphabetically in compilers.ts to satisfy eslint - Preserve non-exported type/interface declarations in stripTypeExports
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/router-utils/src/ast.ts`:
- Around line 45-131: The stripTypeExports function currently misses
TypeScript's "export default type/interface" and the import-equals type-only
form; update stripTypeExports to also filter out t.isExportDefaultDeclaration
nodes whose declaration is a TSTypeAliasDeclaration or TSInterfaceDeclaration
(or whose exportKind === 'type') and to filter out t.isTSImportEqualsDeclaration
nodes when node.isTypeOnly is true so type-only import-equals are removed;
modify the AST filtering logic in stripTypeExports to check for
t.isExportDefaultDeclaration and t.isTSImportEqualsDeclaration and return false
for those type-only cases (and preserve other behavior for mixed/value
declarations).
Comment on lines
+45
to
+131
| /** | ||
| * Strips TypeScript type-only exports and imports from an AST. | ||
| * | ||
| * This is necessary because babel-dead-code-elimination doesn't handle | ||
| * TypeScript type exports/imports. When a type export references an import | ||
| * that pulls in server-only code, the dead code elimination won't remove | ||
| * that import because it sees the type as still referencing it. | ||
| * | ||
| * This function removes: | ||
| * - `export type Foo = ...` | ||
| * - `export interface Foo { ... }` | ||
| * - `export type { Foo } from './module'` | ||
| * - `export type * from './module'` | ||
| * - Type specifiers in mixed exports: `export { value, type Foo }` -> `export { value }` | ||
| * - `import type { Foo } from './module'` | ||
| * - Type specifiers in mixed imports: `import { value, type Foo } from './module'` -> `import { value }` | ||
| * | ||
| * Note: Non-exported type/interface declarations are preserved as they may be | ||
| * used as type annotations within the code. | ||
| * | ||
| * @param ast - The Babel AST (or ParseResult) to mutate | ||
| */ | ||
| export function stripTypeExports(ast: ParseResult<_babel_types.File>): void { | ||
| // Filter the program body to remove type-only nodes | ||
| ast.program.body = ast.program.body.filter((node) => { | ||
| // Handle export declarations | ||
| if (t.isExportNamedDeclaration(node)) { | ||
| // Remove entire export if it's a type-only export | ||
| // e.g., `export type Foo = string`, `export interface Bar {}`, `export type { X } from './y'` | ||
| if (node.exportKind === 'type') { | ||
| return false | ||
| } | ||
|
|
||
| // For value exports with mixed specifiers, filter out type-only specifiers | ||
| // e.g., `export { value, type TypeOnly }` -> `export { value }` | ||
| if (node.specifiers.length > 0) { | ||
| node.specifiers = node.specifiers.filter((specifier) => { | ||
| if (t.isExportSpecifier(specifier)) { | ||
| return specifier.exportKind !== 'type' | ||
| } | ||
| return true | ||
| }) | ||
|
|
||
| // If all specifiers were removed, remove the entire export declaration | ||
| // (unless it has a declaration like `export const x = 1`) | ||
| if (node.specifiers.length === 0 && !node.declaration) { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Handle type-only export-all declarations | ||
| // e.g., `export type * from './module'` | ||
| if (t.isExportAllDeclaration(node)) { | ||
| if (node.exportKind === 'type') { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // Handle import declarations | ||
| if (t.isImportDeclaration(node)) { | ||
| // Remove entire import if it's a type-only import | ||
| // e.g., `import type { Foo } from './module'` | ||
| if (node.importKind === 'type') { | ||
| return false | ||
| } | ||
|
|
||
| // For value imports with mixed specifiers, filter out type-only specifiers | ||
| // e.g., `import { value, type TypeOnly } from './module'` -> `import { value }` | ||
| if (node.specifiers.length > 0) { | ||
| node.specifiers = node.specifiers.filter((specifier) => { | ||
| if (t.isImportSpecifier(specifier)) { | ||
| return specifier.importKind !== 'type' | ||
| } | ||
| return true | ||
| }) | ||
|
|
||
| // If all specifiers were removed, remove the entire import declaration | ||
| if (node.specifiers.length === 0) { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return true | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if these TypeScript syntaxes exist in the repo
echo "=== Checking for 'export default interface/type' syntax ==="
rg -nP "export\s+default\s+(interface|type)\b" --type=ts --type=tsx
echo ""
echo "=== Checking for 'import type Foo = require' syntax ==="
rg -nP "import\s+type\s+\w+\s*=\s*require" --type=ts --type=tsxRepository: TanStack/router
Length of output: 248
🏁 Script executed:
#!/bin/bash
# Check if these TypeScript syntaxes exist in the repo
echo "=== Checking for 'export default interface/type' syntax ==="
rg -nP "export\s+default\s+(interface|type)\b" packages/router-utils/src/
echo ""
echo "=== Checking for 'import type.*require' syntax ==="
rg -nP "import\s+type\s+\w+\s*=\s*require" packages/router-utils/src/
echo ""
echo "=== Broader search for TSImportEqualsDeclaration usage in entire codebase ==="
rg -n "import\s+type\s+\w+\s*=" --type=ts
echo ""
echo "=== Check for export default with type/interface in whole repo ==="
rg -n "export\s+default\s+(type|interface)" --type=tsRepository: TanStack/router
Length of output: 317
🏁 Script executed:
#!/bin/bash
# Check where stripTypeExports is used
echo "=== Usages of stripTypeExports ==="
rg -n "stripTypeExports" packages/router-utils/
echo ""
echo "=== Check test files for stripTypeExports ==="
fd -e test.ts -e test.tsx -e spec.ts packages/router-utils/ | head -20
echo ""
echo "=== Look for test files in router-utils ==="
find packages/router-utils -name "*.test.*" -o -name "*.spec.*" | head -20
echo ""
echo "=== Examine the ast.ts file context ==="
head -50 packages/router-utils/src/ast.tsRepository: TanStack/router
Length of output: 2804
🏁 Script executed:
#!/bin/bash
# Read the test file to see what cases are tested
echo "=== stripTypeExports test file ==="
cat packages/router-utils/tests/stripTypeExports.test.tsRepository: TanStack/router
Length of output: 9056
Handle edge cases for export default type declarations and TypeScript import-equals syntax.
stripTypeExports currently covers ExportNamedDeclaration, ExportAllDeclaration, and ImportDeclaration only. While export default interface/type and import type Foo = require(...) syntaxes don't currently appear in the codebase, they are valid TypeScript patterns that would not be stripped if introduced, potentially allowing server-only imports to remain.
Proposed patch
ast.program.body = ast.program.body.filter((node) => {
+ // Handle default type-only exports: `export default interface Foo {}` / `export default type Foo = ...`
+ if (t.isExportDefaultDeclaration(node)) {
+ const decl = node.declaration
+ if (t.isTSInterfaceDeclaration(decl) || t.isTSTypeAliasDeclaration(decl)) {
+ return false
+ }
+ }
+
+ // Handle TS import-equals type-only: `import type Foo = require('./module')`
+ if (t.isTSImportEqualsDeclaration(node) && node.isTypeOnly) {
+ return false
+ }🤖 Prompt for AI Agents
In `@packages/router-utils/src/ast.ts` around lines 45 - 131, The stripTypeExports
function currently misses TypeScript's "export default type/interface" and the
import-equals type-only form; update stripTypeExports to also filter out
t.isExportDefaultDeclaration nodes whose declaration is a TSTypeAliasDeclaration
or TSInterfaceDeclaration (or whose exportKind === 'type') and to filter out
t.isTSImportEqualsDeclaration nodes when node.isTypeOnly is true so type-only
import-equals are removed; modify the AST filtering logic in stripTypeExports to
check for t.isExportDefaultDeclaration and t.isTSImportEqualsDeclaration and
return false for those type-only cases (and preserve other behavior for
mixed/value declarations).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #6480
Summary by CodeRabbit