fix: enable dead code elimination in split-route to remove unused imports #5711
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
WalkthroughThe core logic change refactors import declaration handling in the code-splitting compiler to construct import declarations separately from their usage, then collect and track imported identifiers via traversal for dead code elimination purposes. Approximately 200+ test snapshots are updated to remove unused Route imports, reflecting the outcome of the compiler logic modification. Changes
Sequence DiagramsequenceDiagram
participant Compiler as Code Splitter Compiler
participant ImportNode as ImportDeclaration Node
participant DCE as Dead Code Elimination
rect rgb(200, 230, 255)
Note over Compiler: Old Flow: Inline Construction
Compiler->>ImportNode: Create & Replace Inline
ImportNode-->>Compiler: (no tracking)
end
rect rgb(200, 255, 220)
Note over Compiler: New Flow: Separate + Track
Compiler->>ImportNode: Construct importDecl
Compiler->>Compiler: Traverse ImportSpecifiers
Compiler->>DCE: Collect & Record refIdents
Compiler->>ImportNode: Replace with importDecl
DCE-->>Compiler: Identifiers Tracked
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Rationale: The core logic change is focused and contained within one file; however, it requires understanding the code-splitting transformation pipeline and how identifier tracking integrates with dead code elimination. The ~200+ snapshot changes are highly homogeneous (repetitive import removals), which significantly reduces review complexity—verify the pattern is consistent and snapshots reflect the expected output. No public API changes detected. Areas requiring attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 60a0dda
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-router-ssr-query
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
+730
to
+756
| const importDecl = t.importDeclaration( | ||
| path.node.declaration.declarations.map((decl) => | ||
| t.importSpecifier( | ||
| t.identifier((decl.id as any).name), | ||
| t.identifier((decl.id as any).name), | ||
| ), | ||
| ), | ||
| t.stringLiteral( | ||
| removeSplitSearchParamFromFilename(opts.filename), | ||
| ), | ||
| ) | ||
|
|
||
| path.replaceWith(importDecl) | ||
|
|
||
| // Track the imported identifier paths so deadCodeElimination can remove them if unused | ||
| // We need to traverse the newly created import to get the identifier paths | ||
| path.traverse({ | ||
| Identifier(identPath) { | ||
| // Only track the local binding identifiers (the imported names) | ||
| if ( | ||
| identPath.parentPath.isImportSpecifier() && | ||
| identPath.key === 'local' | ||
| ) { | ||
| refIdents.add(identPath) | ||
| } | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Good refactor for dead code elimination, but add type safety for destructured patterns.
The refactor correctly separates import declaration construction and adds traversal to track imported identifiers for dead code elimination. However, lines 733-734 use (decl.id as any).name without verifying that decl.id is actually an Identifier.
Variable declarations can have destructured patterns (e.g., export const { x, y } = obj), which would cause a runtime error since ObjectPattern doesn't have a .name property.
Apply this diff to add type safety:
const importDecl = t.importDeclaration(
- path.node.declaration.declarations.map((decl) =>
- t.importSpecifier(
- t.identifier((decl.id as any).name),
- t.identifier((decl.id as any).name),
+ path.node.declaration.declarations.flatMap((decl) => {
+ if (!t.isIdentifier(decl.id)) {
+ throw new Error(
+ `Destructured export patterns are not supported in code-split routes. Found pattern in ${opts.filename}`
+ )
+ }
+ return t.importSpecifier(
+ t.identifier(decl.id.name),
+ t.identifier(decl.id.name),
),
- ),
+ }),
t.stringLiteral(
removeSplitSearchParamFromFilename(opts.filename),
),
)🤖 Prompt for AI Agents
In packages/router-plugin/src/core/code-splitter/compilers.ts around lines 730
to 756, the code assumes decl.id is an Identifier and accesses (decl.id as
any).name which will crash for destructured patterns; update the logic to first
check t.isIdentifier(decl.id) and use its .name for the importSpecifier, and for
non-identifiers (ObjectPattern/ArrayPattern) extract the underlying identifier
names by walking the pattern (properties/elements) and creating importSpecifiers
for each target Identifier; skip or log unsupported pattern types and ensure
only Identifier names are passed to t.identifier to avoid runtime errors.
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.
Summary by CodeRabbit