-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: only run middleware compiler in client env #5703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReworked the compiler plugin to accept a Partial, add client-only middleware initialization (env === 'client'), and introduce defensive guards and local identifier handling across import resolution, call-expression collection, namespace resolution, and final transformation phases. Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant Plugin
participant Identifiers
Compiler->>Plugin: compileStartOutputFactory(partialIdentifiers, env)
alt env == 'client'
Plugin->>Identifiers: createMiddleware()
Identifiers-->>Plugin: add middleware identifier (identifier.paths)
else env != 'client'
Note over Plugin: middleware identifier not created
end
Plugin->>Plugin: import resolution (guard undefined identifiers)
Plugin->>Plugin: collect call-expression paths -> push to identifier.paths
Plugin->>Plugin: namespace/member resolution -> compare identifier.name, push paths
Plugin->>Plugin: final transform -> for each identifier (skip undefined) call handleCallExpression
Plugin-->>Compiler: transformed AST
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
|
View your CI Pipeline Execution ↗ for commit c7cfa35
☁️ 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/start-plugin-core/src/start-compiler-plugin/compilers.ts (1)
116-116: Consider simplifying the return statement.The
returnstatement here returns the new array length frompush(), which is unused. This works correctly but is slightly confusing.Apply this diff for clarity:
- return identifier.paths.push(path) + identifier.paths.push(path) + return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-plugin-core/src/start-compiler-plugin/compilers.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/start-plugin-core/src/start-compiler-plugin/compilers.ts
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-plugin-core/src/start-compiler-plugin/compilers.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (3)
packages/start-plugin-core/src/start-compiler-plugin/compilers.ts (3)
26-42: LGTM! Correct use ofPartial<Identifiers>for conditional inclusion.The type change to
Partial<Identifiers>correctly supports the conditional inclusion ofcreateMiddleware, ensuring type safety when not all identifiers are present.
73-75: LGTM! Proper guards for optional identifiers.The null/undefined checks at all three locations (import resolution, call expression handling, and transformation phase) correctly guard against missing identifiers when
createMiddlewareis excluded in server builds.Also applies to: 95-97, 144-146
147-152: LGTM! Safe transformation with proper guards.The transformation phase correctly accesses
identifier.pathsandidentifier.handleCallExpressiononly after confirming the identifier exists, maintaining type safety throughout.
0432e12 to
c7cfa35
Compare
Summary by CodeRabbit