fix: compiler plugin #6177
fix: compiler plugin #6177
Conversation
WalkthroughAdds a middleware test route and E2E test; introduces a new start-fn-stubs package and re-wires client imports; significantly refactors the create-server-fn plugin/compiler to support Isomorphic/ServerOnly/ClientOnly kinds with new AST handlers and utilities; and removes the previous start-compiler-plugin pipeline. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 5ff68a1
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/react-start/server-functions/src/routes/middleware/server-import-middleware.tsx (1)
1-82: LGTM! Test route effectively demonstrates the fix.The route implementation correctly uses the
createMiddleware().server()pattern with server-only imports, which validates that the compiler plugin fix works as intended.The explicit React import on line 4 may be unnecessary in React 19, as the new JSX transform doesn't require it. However, since
React.useStateis used, the import is needed. Consider using named import instead:🔎 Optional refactor for consistency with React 19
-import React from 'react' +import { useState } from 'react'And update the component:
function RouteComponent() { - const [result, setResult] = React.useState<{ testHeader: string } | null>( + const [result, setResult] = useState<{ testHeader: string } | null>( null, ) - const [error, setError] = React.useState<string | null>(null) + const [error, setError] = useState<string | null>(null)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/react-start/server-functions/src/routeTree.gen.ts(11 hunks)e2e/react-start/server-functions/src/routes/middleware/index.tsx(1 hunks)e2e/react-start/server-functions/src/routes/middleware/server-import-middleware.tsx(1 hunks)e2e/react-start/server-functions/tests/server-functions.spec.ts(1 hunks)packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/middleware/index.tsxe2e/react-start/server-functions/src/routes/middleware/server-import-middleware.tsxpackages/start-plugin-core/src/create-server-fn-plugin/plugin.tse2e/react-start/server-functions/src/routeTree.gen.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/middleware/index.tsxe2e/react-start/server-functions/src/routes/middleware/server-import-middleware.tsxpackages/start-plugin-core/src/create-server-fn-plugin/plugin.tse2e/react-start/server-functions/src/routeTree.gen.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/middleware/index.tsxe2e/react-start/server-functions/src/routes/middleware/server-import-middleware.tsxe2e/react-start/server-functions/src/routeTree.gen.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/server-functions/src/routes/middleware/server-import-middleware.tsxe2e/react-start/server-functions/src/routeTree.gen.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
e2e/react-start/server-functions/src/routes/middleware/server-import-middleware.tsxe2e/react-start/server-functions/src/routeTree.gen.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
e2e/react-start/server-functions/src/routeTree.gen.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-start/server-functions/src/routeTree.gen.ts
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
Applied to files:
e2e/react-start/server-functions/src/routeTree.gen.ts
📚 Learning: 2025-12-21T12:52:27.078Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:27.078Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
e2e/react-start/server-functions/src/routeTree.gen.ts
🧬 Code graph analysis (2)
e2e/react-start/server-functions/src/routes/middleware/index.tsx (2)
e2e/react-start/server-functions/src/routes/middleware/server-import-middleware.tsx (1)
Route(36-38)e2e/react-start/server-functions/src/routes/middleware/request-middleware.tsx (1)
Route(23-26)
e2e/react-start/server-functions/src/routes/middleware/server-import-middleware.tsx (2)
packages/start-server-core/src/request-response.ts (1)
getRequestHeaders(149-152)e2e/react-start/server-functions/src/routes/middleware/index.tsx (1)
Route(3-5)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (4)
e2e/react-start/server-functions/src/routes/middleware/index.tsx (1)
36-43: LGTM! Navigation link follows established patterns.The new navigation link is correctly structured and follows the same pattern as other middleware test links in the list.
e2e/react-start/server-functions/tests/server-functions.spec.ts (1)
619-637: LGTM! Comprehensive test validates the fix.The test effectively validates that server-only imports inside
createMiddleware().server()are properly stripped from the client build. The fact that the page loads without build errors proves the fix works, and the assertion verifies the middleware functionality.e2e/react-start/server-functions/src/routeTree.gen.ts (1)
37-37: Autogenerated file - no review needed.This is an autogenerated route tree file that should not be manually modified or reviewed.
Based on learnings,
routeTree.gen.tsfiles in TanStack Router repositories are autogenerated and should not be manually modified.Also applies to: 170-175, 231-231, 265-265, 300-300, 336-336, 370-370, 404-404, 439-439, 630-636, 703-703
packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (1)
60-66: LGTM! Core fix correctly matches the actual middleware usage pattern.The change from matching
.createMiddleware(to.server(correctly addresses the issue. All middleware usage in the codebase follows the patterncreateMiddleware().server(...)orcreateMiddleware({...}).server(...), so matching.server(ensures the server-only code block is properly identified and stripped from client builds. The.handler(pattern forcreateServerFnis also correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (1)
79-106: Update regex patterns to use word boundaries for more precise filtering.The transform code filters use regex patterns without word boundaries:
/createIsomorphicFn/would matchmyCreateIsomorphicFn/createServerOnlyFn/would matchnotCreateServerOnlyFn/createClientOnlyFn/would matchprefixCreateClientOnlyFnWhile the compiler performs AST-based validation later, these false positives would cause unnecessary transform hook invocations. Use word boundaries for precise matching:
const commonTransformCodeFilter = [ /\.\s*handler\(/, /\bcreateIsomorphicFn\b/, /\bcreateServerOnlyFn\b/, /\bcreateClientOnlyFn\b/, ](Note: The original suggestion had a typo—
creatIsomorphicFnwas missing the 'e')
🧹 Nitpick comments (2)
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.ts (1)
20-25: Consider structured logging for compile-time warnings.Using
console.warnwith multiple arguments works but produces fragmented output. A single formatted string would be more readable in build logs.🔎 Suggested improvement
- console.warn( - 'createIsomorphicFn called without a client or server implementation!', - 'This will result in a no-op function.', - 'Variable name:', - t.isIdentifier(variableId) ? variableId.name : 'unknown', - ) + const varName = t.isIdentifier(variableId) ? variableId.name : 'unknown' + console.warn( + `createIsomorphicFn called without a client or server implementation! This will result in a no-op function. Variable name: ${varName}`, + )packages/start-plugin-core/src/start-router-plugin/plugin.ts (1)
20-20: Useimport typefor type-only imports.Since
TanStackStartVitePluginCoreOptionsandGetConfigFnare only used as type annotations in this file, usingimport typeimproves clarity and enables better tree-shaking.🔎 Suggested fix
-import { TanStackStartVitePluginCoreOptions, GetConfigFn } from '../types' +import type { TanStackStartVitePluginCoreOptions, GetConfigFn } from '../types'
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.tspackages/start-plugin-core/src/create-server-fn-plugin/handleEnvOnly.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tspackages/start-plugin-core/src/create-server-fn-plugin/utils.tspackages/start-plugin-core/src/plugin.tspackages/start-plugin-core/src/schema.tspackages/start-plugin-core/src/start-compiler-plugin/compilers.tspackages/start-plugin-core/src/start-compiler-plugin/constants.tspackages/start-plugin-core/src/start-compiler-plugin/envOnly.tspackages/start-plugin-core/src/start-compiler-plugin/isomorphicFn.tspackages/start-plugin-core/src/start-compiler-plugin/plugin.tspackages/start-plugin-core/src/start-compiler-plugin/utils.tspackages/start-plugin-core/src/start-manifest-plugin/plugin.tspackages/start-plugin-core/src/start-router-plugin/plugin.tspackages/start-plugin-core/src/types.tspackages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructuredRename.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructuredRename.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createMiddleware/createMiddleware.test.tspackages/start-plugin-core/tests/createMiddleware/snapshots/client/create-function-middleware.tspackages/start-plugin-core/tests/createMiddleware/snapshots/client/createMiddlewareDestructured.tsxpackages/start-plugin-core/tests/createMiddleware/snapshots/client/createMiddlewareDestructuredRename.tsxpackages/start-plugin-core/tests/createMiddleware/snapshots/client/createMiddlewareStarImport.tsxpackages/start-plugin-core/tests/createMiddleware/snapshots/client/createMiddlewareValidator.tsxpackages/start-plugin-core/tests/createMiddleware/snapshots/client/createStart.tsxpackages/start-plugin-core/tests/createMiddleware/test-files/create-function-middleware.tspackages/start-plugin-core/tests/createMiddleware/test-files/createMiddlewareDestructured.tsxpackages/start-plugin-core/tests/createMiddleware/test-files/createMiddlewareDestructuredRename.tsxpackages/start-plugin-core/tests/createMiddleware/test-files/createMiddlewareStarImport.tsxpackages/start-plugin-core/tests/createMiddleware/test-files/createMiddlewareValidator.tsxpackages/start-plugin-core/tests/createMiddleware/test-files/createStart.tsxpackages/start-plugin-core/tests/createServerFn/createServerFn.test.tspackages/start-plugin-core/tests/envOnly/envOnly.test.tspackages/start-plugin-core/tests/envOnly/snapshots/client/envOnlyDestructured.tsxpackages/start-plugin-core/tests/envOnly/snapshots/client/envOnlyDestructuredRename.tsxpackages/start-plugin-core/tests/envOnly/snapshots/client/envOnlyStarImport.tsxpackages/start-plugin-core/tests/envOnly/snapshots/server/envOnlyDestructured.tsxpackages/start-plugin-core/tests/envOnly/snapshots/server/envOnlyDestructuredRename.tsxpackages/start-plugin-core/tests/envOnly/snapshots/server/envOnlyStarImport.tsx
💤 Files with no reviewable changes (18)
- packages/start-plugin-core/tests/envOnly/snapshots/server/envOnlyStarImport.tsx
- packages/start-plugin-core/src/start-compiler-plugin/utils.ts
- packages/start-plugin-core/src/start-compiler-plugin/plugin.ts
- packages/start-plugin-core/src/start-compiler-plugin/isomorphicFn.ts
- packages/start-plugin-core/src/start-compiler-plugin/constants.ts
- packages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnStarImport.tsx
- packages/start-plugin-core/tests/envOnly/snapshots/client/envOnlyDestructuredRename.tsx
- packages/start-plugin-core/tests/envOnly/snapshots/client/envOnlyDestructured.tsx
- packages/start-plugin-core/tests/envOnly/snapshots/server/envOnlyDestructured.tsx
- packages/start-plugin-core/src/start-compiler-plugin/compilers.ts
- packages/start-plugin-core/src/start-compiler-plugin/envOnly.ts
- packages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructured.tsx
- packages/start-plugin-core/tests/envOnly/snapshots/client/envOnlyStarImport.tsx
- packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnStarImport.tsx
- packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructuredRename.tsx
- packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructured.tsx
- packages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructuredRename.tsx
- packages/start-plugin-core/tests/envOnly/snapshots/server/envOnlyDestructuredRename.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/start-plugin-core/src/start-manifest-plugin/plugin.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.tspackages/start-plugin-core/src/start-router-plugin/plugin.tspackages/start-plugin-core/src/create-server-fn-plugin/handleEnvOnly.tspackages/start-plugin-core/tests/envOnly/envOnly.test.tspackages/start-plugin-core/tests/createServerFn/createServerFn.test.tspackages/start-plugin-core/src/schema.tspackages/start-plugin-core/src/plugin.tspackages/start-plugin-core/src/create-server-fn-plugin/utils.tspackages/start-plugin-core/tests/createMiddleware/createMiddleware.test.tspackages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/src/types.tspackages/start-plugin-core/src/create-server-fn-plugin/compiler.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.tspackages/start-plugin-core/src/start-router-plugin/plugin.tspackages/start-plugin-core/src/create-server-fn-plugin/handleEnvOnly.tspackages/start-plugin-core/tests/envOnly/envOnly.test.tspackages/start-plugin-core/tests/createServerFn/createServerFn.test.tspackages/start-plugin-core/src/schema.tspackages/start-plugin-core/src/plugin.tspackages/start-plugin-core/src/create-server-fn-plugin/utils.tspackages/start-plugin-core/tests/createMiddleware/createMiddleware.test.tspackages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/src/types.tspackages/start-plugin-core/src/create-server-fn-plugin/compiler.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.ts
🧠 Learnings (3)
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/start-plugin-core/src/start-router-plugin/plugin.tspackages/start-plugin-core/tests/envOnly/envOnly.test.tspackages/start-plugin-core/tests/createServerFn/createServerFn.test.tspackages/start-plugin-core/src/schema.tspackages/start-plugin-core/src/plugin.tspackages/start-plugin-core/tests/createMiddleware/createMiddleware.test.tspackages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
packages/start-plugin-core/src/start-router-plugin/plugin.tspackages/start-plugin-core/src/schema.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/start-plugin-core/tests/envOnly/envOnly.test.ts
🧬 Code graph analysis (6)
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.ts (2)
packages/start-plugin-core/src/create-server-fn-plugin/types.ts (1)
RewriteCandidate(39-42)packages/router-core/src/route.ts (1)
path(1553-1555)
packages/start-plugin-core/src/create-server-fn-plugin/handleEnvOnly.ts (3)
packages/start-plugin-core/src/create-server-fn-plugin/types.ts (1)
RewriteCandidate(39-42)packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (1)
LookupKind(35-40)packages/router-core/src/route.ts (1)
path(1553-1555)
packages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.ts (1)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (2)
compile(308-461)ServerFnCompiler(125-817)
packages/start-plugin-core/src/types.ts (1)
packages/start-plugin-core/src/schema.ts (1)
TanStackStartOutputConfig(207-207)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (3)
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts (1)
handleCreateMiddleware(10-45)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.ts (1)
handleCreateIsomorphicFn(4-46)packages/start-plugin-core/src/create-server-fn-plugin/handleEnvOnly.ts (1)
handleEnvOnlyFn(10-45)
packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (2)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (2)
LookupKind(35-40)LookupConfig(110-114)packages/start-plugin-core/src/types.ts (1)
CompileStartFrameworkOptions(3-3)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (24)
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.ts (1)
2-2: LGTM - Import path update aligns with module reorganization.The import relocation from
../start-compiler-plugin/utilsto./utilsis consistent with the newutils.tsfile added in this plugin directory.packages/start-plugin-core/src/create-server-fn-plugin/utils.ts (1)
1-24: LGTM - Clean utility for formatted error messages.The
codeFrameErrorfunction provides a useful abstraction for creating developer-friendly error messages with code context. The implementation correctly uses@babel/code-framefor syntax highlighting.packages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.ts (1)
4-46: LGTM - Solid implementation of environment-specific isomorphic function handling.The function correctly:
- Selects the appropriate implementation based on the build environment
- Provides a warning when no implementations are provided
- Falls back to no-op functions when the current environment has no implementation
- Validates that the extracted argument is an expression
packages/start-plugin-core/src/create-server-fn-plugin/handleEnvOnly.ts (2)
28-44: Good defensive runtime error for environment mismatch.The throwing arrow function provides clear runtime feedback when a server-only or client-only function is called in the wrong environment. This is the correct approach for environment-specific code that shouldn't leak across boundaries - directly addressing the issue #6169.
10-15: The logic at line 15 is correct. The calling code incompiler.ts(lines 430-451) uses an if/else chain that ensureshandleEnvOnlyFnis only invoked whenkindis eitherServerOnlyFnorClientOnlyFn. OtherLookupKindvalues are routed to different handlers (ServerFn,Middleware,IsomorphicFn), so the assumption is valid and no unexpected behavior can occur.Likely an incorrect or invalid review comment.
packages/start-plugin-core/src/schema.ts (1)
4-4: LGTM - Import path updated to reference types from centralized module.This aligns with the broader refactoring that moves type declarations to
./types.ts.packages/start-plugin-core/tests/createServerFn/createServerFn.test.ts (3)
22-27: LGTM - Test configuration updated for newkind: 'Root'property.The addition of
kind: 'Root'correctly reflects the updatedLookupConfigtype that now supports root-based resolution for library imports.
249-253: LGTM - Test assertions updated to reflect fast-path resolution behavior.The updated expectation correctly validates that root imports use the fast path with
knownRootImports, requiring only the library name (single argument) rather than both library and file name.
298-306: LGTM - Factory pattern test correctly validates slow-path behavior.The test properly verifies that:
- Root imports (
@tanstack/react-start) use the fast path (single arg)- Local factory imports (
./factory) use the slow path (two args with file context)packages/start-plugin-core/tests/envOnly/envOnly.test.ts (1)
5-41: LGTM! Well-structured test helper.The new
compilehelper properly configures theServerFnCompilerwith all required options and provides a clean async interface for tests.packages/start-plugin-core/src/plugin.ts (2)
28-32: LGTM! Type consolidation improves maintainability.Moving these types to a dedicated
types.tsfile reduces duplication and provides a single source of truth for shared type definitions.
406-407: Helpful architectural context.The comment clearly explains that
startCompilerPluginfunctionality has been merged intocreateServerFnPlugin, which aids understanding of the refactor.packages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.ts (1)
5-36: LGTM! Consistent test refactoring.The test helper follows the same pattern as other test files, providing consistent async handling across the test suite.
packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (1)
12-25: LGTM! Expanded environment support.The addition of
IsomorphicFn,ServerOnlyFn, andClientOnlyFnto both client and server lookup kinds properly supports the new function types across environments.packages/start-plugin-core/tests/createMiddleware/createMiddleware.test.ts (2)
17-17: LGTM! Simplified test configuration.The
loadModulecallback no longer needs theidparameter since it's a no-op in tests, making the test setup cleaner.
98-100: Clear documentation of behavioral change.The comment effectively explains that
init()now resolves from the project root rather than from a specific file, which helps future maintainers understand the resolution behavior.packages/start-plugin-core/src/types.ts (1)
1-34: LGTM! Well-organized type definitions.The new types module consolidates shared types across the plugin core, reducing duplication and improving maintainability. All type definitions are clear and properly documented through their structure.
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (7)
35-69: LGTM! Well-designed detection strategy.The new
LookupKindvariants and detection strategies properly handle:
- Method chain patterns (ServerFn, Middleware, IsomorphicFn)
- Direct call patterns (ServerOnlyFn, ClientOnlyFn)
- Root-as-candidate for IsomorphicFn (which can be called without
.client()or.server())The design is flexible and extensible.
71-87: Excellent optimization for shared identifiers.The
IdentifierToKindsmap correctly handles the fact that identifiers like'server'and'client'are used by multiple kinds (bothMiddlewareandIsomorphicFn). UsingSet<LookupKind>allows O(1) lookup while supporting multiple kinds per identifier.
437-451: LGTM! Comprehensive kind handling.The compiler now properly delegates to the appropriate handler for each kind:
ServerFn→handleCreateServerFnMiddleware→handleCreateMiddlewareIsomorphicFn→handleCreateIsomorphicFnServerOnlyFn/ClientOnlyFn→handleEnvOnlyFnThe else branch correctly catches the environment-specific function kinds.
464-501: Well-structured candidate collection with clear patterns.The updated logic correctly handles:
- Method chain patterns (existing behavior)
- Direct call patterns for environment-specific functions
- Root-as-candidate patterns for IsomorphicFn
The two-phase approach (method chain check first, then direct/root call check) ensures proper candidate identification.
679-693: Correct handling of root-as-candidate kinds.The logic properly identifies when a
Rootkind call should be treated as a specific kind (e.g.,createIsomorphicFn()without method chains returnsIsomorphicFnkind). This enables the no-op function generation for IsomorphicFn without implementations.
819-843: Efficient candidate detection using pre-computed map.The use of
IdentifierToKinds.get()provides O(1) lookup and correctly handles identifiers shared across multiple kinds. The validation that at least one possible kind exists invalidLookupKindsensures proper filtering.
146-194: Line 149 callsresolveId(config.libName)without an importer parameter. While importer values may affect resolution in Vite's dev server, this change is safe because:
Root-level package resolution: The
libNamevalues are always root package names (e.g.,@tanstack/react-start), not subpaths. Conditional exports and subpath patterns can provide different implementations based on conditions, but root package imports typically don't vary by importer context.No importer-dependent exports in monorepo: The packages have conditional exports (found in packages/start-client-core, start-server-core, etc.), but they use simple "import" conditions without importer-specific logic.
Tests verify intended behavior: The test suite explicitly expects
resolveIdto be called with just the package name duringinit()(line 253:expect(resolveIdMock).toHaveBeenCalledWith('@tanstack/react-start')), confirming this is the intended design.This is a deliberate optimization to cache root-level resolutions and avoid redundant async calls.
schiller-manuel
force-pushed
the
fix-6169
branch
from
597397c to
1948064
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (2)
107-139: Consider validating the single root-as-candidate assumption.The
rootAsCandidateKindfield assumes only oneLookupKindcan haveallowRootAsCandidate: true. If multiple kinds set this flag in the future, the loop at lines 131-139 will silently override earlier values, storing only the last one.Currently this works fine since only
IsomorphicFnuses this feature, but for future maintainability, consider adding validation:🔎 Proposed validation
// Precompute flags for candidate detection this.hasDirectCallKinds = false this.hasRootAsCandidateKinds = false for (const kind of options.lookupKinds) { const setup = LookupSetup[kind] if (setup.type === 'directCall') { this.hasDirectCallKinds = true } else if (setup.allowRootAsCandidate) { + if (this.rootAsCandidateKind !== null) { + throw new Error( + `Multiple kinds with allowRootAsCandidate detected: ${this.rootAsCandidateKind} and ${kind}. Only one kind can have this flag.` + ) + } this.hasRootAsCandidateKinds = true this.rootAsCandidateKind = kind } }
715-747: Pattern matching logic is correct, minor duplication acceptable.The updated pattern matching correctly uses the new
IdentifierToKindsmap to handle shared identifiers across multiple kinds. The base expression is resolved once (line 719) and reused for all checks, which is efficient.There's some structural duplication across the
ServerFn,Middleware, andIsomorphicFnblocks (lines 725-744), but each has different base-kind requirements:
ServerFn: acceptsRootorBuilderMiddleware: acceptsRoot,Builder, orMiddlewareIsomorphicFn: acceptsRoot,Builder, orIsomorphicFnGiven these differences and the "Chill" review mode, the current implementation is acceptable. A helper function could reduce duplication but might not improve clarity.
packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (1)
80-85: Consider adding word boundaries to function name patterns.The patterns for
createIsomorphicFn,createServerOnlyFn, andcreateClientOnlyFn(lines 82-84) don't include word boundaries or parenthesis checks. While this works as a filter (false positives are acceptable), adding boundaries like/\bcreateIsomorphicFn\b/would make the patterns more precise and reduce unnecessary file processing.Similarly, the
.handler(pattern (line 81) matches ANY.handler(call, not just those on server functions. Consider whether this could be more specific.🔎 Proposed improvement for pattern precision
const commonTransformCodeFilter = [ - /\.\s*handler\(/, - /createIsomorphicFn/, - /createServerOnlyFn/, - /createClientOnlyFn/, + /\.\s*handler\s*\(/, + /\bcreateIsomorphicFn\b/, + /\bcreateServerOnlyFn\b/, + /\bcreateClientOnlyFn\b/, ]packages/start-fn-stubs/eslint.config.js (1)
10-13: Consider removing the empty config object.The empty plugins and rules object appears to be placeholder scaffolding. Unless this is intentionally reserved for future package-specific rules, it can be removed for cleaner configuration.
🔎 Proposed cleanup
export default [ ...rootConfig, { files: ['**/*.{ts,tsx}'], }, - { - plugins: {}, - rules: {}, - }, ]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
labeler-config.ymlpackages/start-client-core/package.jsonpackages/start-client-core/src/getGlobalStartContext.tspackages/start-client-core/src/getRouterInstance.tspackages/start-client-core/src/getStartContextServerOnly.tspackages/start-client-core/src/getStartOptions.tspackages/start-client-core/src/index.tsxpackages/start-fn-stubs/eslint.config.jspackages/start-fn-stubs/package.jsonpackages/start-fn-stubs/src/index.tspackages/start-fn-stubs/tsconfig.jsonpackages/start-fn-stubs/vite.config.tspackages/start-plugin-core/src/create-server-fn-plugin/compiler.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tspackages/start-plugin-core/src/create-server-fn-plugin/types.tspackages/start-plugin-core/src/plugin.tspackages/start-plugin-core/src/types.tsscripts/publish.js
💤 Files with no reviewable changes (1)
- packages/start-plugin-core/src/create-server-fn-plugin/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/start-plugin-core/src/plugin.ts
- packages/start-plugin-core/src/types.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-fn-stubs/vite.config.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tspackages/start-client-core/src/index.tsxpackages/start-client-core/src/getGlobalStartContext.tspackages/start-fn-stubs/src/index.tspackages/start-client-core/src/getStartOptions.tspackages/start-client-core/src/getStartContextServerOnly.tspackages/start-client-core/src/getRouterInstance.tspackages/start-plugin-core/src/create-server-fn-plugin/compiler.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-fn-stubs/vite.config.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tspackages/start-client-core/src/index.tsxpackages/start-client-core/src/getGlobalStartContext.tspackages/start-fn-stubs/src/index.tspackages/start-client-core/src/getStartOptions.tspackages/start-client-core/src/getStartContextServerOnly.tspackages/start-client-core/src/getRouterInstance.tspackages/start-fn-stubs/eslint.config.jspackages/start-plugin-core/src/create-server-fn-plugin/compiler.tsscripts/publish.js
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace protocol
workspace:*for internal dependencies in package.json files
Files:
packages/start-fn-stubs/package.jsonpackages/start-client-core/package.json
🧠 Learnings (6)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{ts,tsx} : Use TypeScript strict mode with extensive type safety for all code
Applied to files:
packages/start-fn-stubs/tsconfig.jsonpackages/start-fn-stubs/eslint.config.js
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/start-fn-stubs/tsconfig.jsonpackages/start-fn-stubs/vite.config.tspackages/start-fn-stubs/package.jsonpackages/start-client-core/src/index.tsxpackages/start-client-core/src/getGlobalStartContext.tspackages/start-client-core/package.jsonpackages/start-client-core/src/getStartOptions.tspackages/start-client-core/src/getStartContextServerOnly.tspackages/start-client-core/src/getRouterInstance.tspackages/start-fn-stubs/eslint.config.jsscripts/publish.js
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/package.json : Use workspace protocol `workspace:*` for internal dependencies in package.json files
Applied to files:
packages/start-client-core/package.json
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/start-client-core/src/getRouterInstance.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
packages/start-fn-stubs/eslint.config.js
🧬 Code graph analysis (2)
packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (2)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (2)
LookupKind(35-40)LookupConfig(89-93)packages/start-plugin-core/src/types.ts (1)
CompileStartFrameworkOptions(3-3)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (3)
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts (1)
handleCreateMiddleware(10-45)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.ts (1)
handleCreateIsomorphicFn(4-46)packages/start-plugin-core/src/create-server-fn-plugin/handleEnvOnly.ts (1)
handleEnvOnlyFn(10-45)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (24)
labeler-config.yml (1)
85-87: LGTM!The new labeler configuration for the
start-fn-stubspackage is correctly formatted, follows the established naming convention, and is properly placed in alphabetical order. This change appropriately supports the new package introduced in this PR.packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (6)
35-87: LGTM! Key fix for middleware expression matching.The expansion of
LookupKindto includeIsomorphicFn,ServerOnlyFn, andClientOnlyFn, combined with theIdentifierToKindsmap now supportingSet<LookupKind>, directly addresses the middleware matching issue. This allows multiple kinds to share the same identifier (e.g., 'server' and 'client' are used by bothMiddlewareandIsomorphicFn) without conflicts, which was likely causing node:* modules to leak into client builds in issue #6169.The detection strategy separation (
MethodChainSetupvsDirectCallSetup) is a clean design that supports both chained method calls and direct function calls.
142-200: LGTM! Well-structured initialization with fast-path optimization.The
init()method correctly registers stub packages (@tanstack/start-fn-stubs) and populates both the fast-path lookup (knownRootImports) and module cache. The fast-path optimization avoids asyncresolveIdcalls for common imports, improving performance.The comment on line 192 clearly explains why
initis set to null whenresolvedKindis present.
441-455: LGTM! Rewrite logic correctly handles all kinds.The updated rewrite logic properly delegates to the appropriate handler for each kind:
ServerFn→handleCreateServerFnMiddleware→handleCreateMiddlewareIsomorphicFn→handleCreateIsomorphicFnServerOnlyFn/ClientOnlyFn→handleEnvOnlyFnwith kind parameterThe structure is clear and extensible.
468-501: LGTM! Candidate collection handles both patterns efficiently.The updated
collectCandidatescorrectly handles:
- Method chain pattern:
.handler(),.server(),.client()(checked first, takes precedence)- Direct call pattern:
createServerOnlyFn(),createClientOnlyFn(),createIsomorphicFn()(checked only when needed)The
continuestatement on line 480 ensures method chain matches take precedence, and the conditional check on line 488 avoids unnecessary work when direct call kinds aren't enabled. The logic also correctly handles namespace calls (e.g.,TanStackStart.createServerOnlyFn()).
678-686: LGTM! Root-as-candidate resolution is correct.The logic correctly handles kinds that allow root as candidate (currently
IsomorphicFn). When a call expression resolves to'Root'androotAsCandidateKindis set, it returns that kind, enabling direct calls likecreateIsomorphicFn()without chained methods.This works in conjunction with the precomputed
rootAsCandidateKindfield from the constructor (lines 131-139).
806-830: LGTM! Candidate detection correctly handles shared identifiers.The updated
isCandidateCallExpressionuses the newIdentifierToKindsmap to support multiple kinds sharing the same identifier. The function:
- Performs O(1) map lookup to get possible kinds for an identifier
- Checks each possible kind against the valid lookup kinds (O(k) where k is small)
- Returns the node if any valid kind is found
This is part of the core fix for the middleware expression matching issue, enabling proper recognition of identifiers like 'server' and 'client' that are used by both
MiddlewareandIsomorphicFn.packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (5)
3-3: LGTM: Type import centralized.The type import has been properly moved to the centralized types module, improving maintainability.
8-9: LGTM: Query parameter handling is correct.The implementation correctly strips query parameters from module IDs.
28-73: LGTM: Configuration structure is well-organized.The refactored
getLookupConfigurationsForEnvfunction clearly separates common and environment-specific configurations. The distinction between 'Root' kind (for builder patterns) and direct kinds (forcreateServerOnlyFnandcreateClientOnlyFn) is logical.
98-107: LGTM: Middleware pattern correctly added to client filter (key fix).The addition of
/createMiddleware\s*\(/to the client-sidetransformCodeFilter(line 106) is the critical fix for issue #6169. This ensures middleware files are properly processed by the compiler for the client environment, preventing node:* imports from leaking into the client bundle.The pattern correctly matches
createMiddleware(with optional whitespace, including newlines. The inclusion of the parenthesis check (\s*\() makes the pattern more specific than the other function patterns, which is appropriate given that "middleware" is a more common word that could appear in comments or variable names.
12-26: Middleware in client set is correct and intentional.All code in TanStack Start is isomorphic by default and runs in both server and client bundles unless explicitly constrained. During SSR, client Middleware will run on the server, which allows the client functionality to work while the client portion runs on the server. The plugin must identify
Middlewaredeclarations in client code to extract and transform them, removing server-specific code (.server()methods) from the client bundle. This prevents issues like those documented in GitHub issues where server-side code within middleware leaked into client bundles.packages/start-client-core/package.json (1)
83-83: LGTM! Dependency correctly uses workspace protocol.The new dependency on
@tanstack/start-fn-stubscorrectly uses theworkspace:*protocol, consistent with other internal dependencies in this package.packages/start-fn-stubs/tsconfig.json (1)
1-7: LGTM! TypeScript configuration is appropriate.The configuration correctly extends the root tsconfig, sets module to
esnextfor ESM output, and includes the necessary source files.packages/start-client-core/src/getGlobalStartContext.ts (1)
2-2: LGTM! Import correctly migrated to external stub package.The import source change from a local module to
@tanstack/start-fn-stubsaligns with the PR's objective to prevent server modules from leaking into client builds by extracting isomorphic function abstractions into a separate package.packages/start-client-core/src/getStartContextServerOnly.ts (1)
2-2: LGTM! Import correctly migrated to external stub package.The import source change from
./envOnlyto@tanstack/start-fn-stubsis consistent with the refactoring to extract environment-specific function utilities into a shared package.packages/start-client-core/src/getStartOptions.ts (1)
2-2: LGTM! Import correctly migrated to external stub package.The import source change is consistent with the broader refactoring to use
@tanstack/start-fn-stubsfor isomorphic function abstractions.packages/start-client-core/src/index.tsx (1)
5-13: LGTM! Public API correctly migrated to external stub package.The export consolidation correctly moves all isomorphic, server-only, and client-only function utilities to
@tanstack/start-fn-stubs. This centralizes the environment-specific function abstractions and aligns with the PR's objective to prevent server code from leaking into client builds.packages/start-fn-stubs/package.json (1)
54-56: No action required. The Node.js engine requirement of >=22.12.0 aligns with all other start framework packages in the monorepo. This is a consistent, intentional design where start packages require >=22.12.0 while core router and adapter packages require >=12. The higher requirement does not inadvertently raise the project minimum since it is scoped to the start ecosystem.scripts/publish.js (1)
147-150: LGTM! Package entry added correctly.The new
@tanstack/start-fn-stubspackage entry follows the existing format and is properly positioned in the packages array.packages/start-fn-stubs/vite.config.ts (1)
1-20: LGTM! Standard Vite/Vitest configuration.The configuration appropriately:
- Enables TypeScript type checking for build-time safety
- Outputs ESM-only (suitable for a stub package transformed at build time)
- Uses TanStack's standard Vite config
packages/start-client-core/src/getRouterInstance.ts (1)
2-2: Migration of createIsomorphicFn imports is complete and consistent.All local imports of
createIsomorphicFnacross the codebase have been successfully migrated to the external@tanstack/start-fn-stubspackage. Verified that production code ingetRouterInstance.ts,getStartOptions.ts, andgetGlobalStartContext.tsall import from@tanstack/start-fn-stubs, with no remaining relative imports outside of test files.packages/start-fn-stubs/src/index.ts (2)
36-41: Verify transformer correctly replaces dummy implementation.The dummy implementation relies on a build-time transformer to be replaced. The transformer is implemented in
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.tsand explicitly registered for@tanstack/start-fn-stubsin the compiler'sknownRootImports. It correctly extracts environment-specific implementations and throws errors if implementations are missing, with comprehensive test coverage ensuring correct transformations for both client and server environments. Given that this PR fixes issue #6169 (node:* modules leaking into client builds), ensure the transformer correctly processes these stubs in the build pipeline.
47-51: Stubs are intentionally simple; protection is provided by the compiler plugin.Functions created using createClientOnlyFn() are replaced with a function that throws an Error on the server, and the reverse is true for createServerOnlyFn functions on the client. The stub functions in
packages/start-fn-stubs/src/index.tsare correctly implemented as identity functions because they serve only for type definitions. The actual runtime protection is provided by theServerFnCompilerinpackages/start-plugin-core, which transforms calls during the build process:
- Matching environment: Extracts and uses the inner function directly
- Wrong environment: Replaces the call with a function that throws an error
- Dead code elimination: Removes server-only code and unused imports (including node:* imports) from client bundles
The test snapshots confirm the compiler correctly strips server-only implementations from client builds, preventing node:* module imports from leaking to the client bundle.
schiller-manuel
force-pushed
the
fix-6169
branch
from
8081472 to
f7876b5
Compare
schiller-manuel
force-pushed
the
fix-6169
branch
from
2e368f0 to
14179dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (2)
7-10: Consider edge cases in query string handling.The
cleanIdfunction usesindexOfandsubstringto remove query parameters. While this handles the common case, it may not handle edge cases like:
- Hash fragments (
#)- Multiple
?characters in the path (though rare)- Encoded characters in the query string
More robust query string handling (if needed)
function cleanId(id: string): string { - const queryIndex = id.indexOf('?') - return queryIndex === -1 ? id : id.substring(0, queryIndex) + // Remove query string and hash from the id + return id.split(/[?#]/)[0] || id }
80-85: The filter pattern inconsistency is intentional and partially documented, but the rationale could be clearer.The
commonTransformCodeFilterpatterns are intentionally inconsistent:
/\.\s*handler\(/matches only.handler(method calls/createIsomorphicFn/,/createServerOnlyFn/,/createClientOnlyFn/match these function names anywhere in the codeThis design is documented in inline comments (lines 98-103) and serves a purpose: the specific
.handler(pattern targetscreateServerFn().handler()calls, while the broader patterns ensure that any file mentioning these functions is included for transformation. However, the broader matching will include false positives (comments, strings, variable names containing these substrings), relying on the actual transform logic in the compiler to validate matches.For maintainability, consider adding an explanation to the
commonTransformCodeFilterdefinition clarifying why the.handler(pattern differs from the others, or standardizing the approach if possible.packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (1)
469-498: Broad candidate collection may impact performance.The direct call pattern detection (lines 486-493) adds ANY call expression with an identifier or member expression callee as a candidate when
hasDirectCallKinds || hasRootAsCandidateKindsis true. While the comment notes "kind resolution will verify it's actually a known export," this could collect many false positive candidates in files with numerous function calls.For example, in a file with 100 function calls, all 100 would become candidates if any direct-call kinds are enabled, even though most won't resolve to the target kinds.
Consider:
- Adding a fast-path check using
knownRootImportsbefore adding candidates- Or profiling to verify this doesn't cause performance issues in large files
Potential optimization to reduce false positives
// Pattern 2: Direct call pattern // Handles: // - createServerOnlyFn(), createClientOnlyFn() (direct call kinds) // - createIsomorphicFn() (root-as-candidate kinds) // - TanStackStart.createServerOnlyFn() (namespace calls) if (this.hasDirectCallKinds || this.hasRootAsCandidateKinds) { - if ( - t.isIdentifier(binding.init.callee) || - (t.isMemberExpression(binding.init.callee) && - t.isIdentifier(binding.init.callee.property)) - ) { - // Include as candidate - kind resolution will verify it's actually a known export - candidates.push(binding.init) - } + // Fast-path check: only add as candidate if the callee name could match a known export + let shouldAddCandidate = false + if (t.isIdentifier(binding.init.callee)) { + // Check if this identifier could be a known export from any library + shouldAddCandidate = true // Will be verified during resolution + } else if ( + t.isMemberExpression(binding.init.callee) && + t.isIdentifier(binding.init.callee.property) + ) { + shouldAddCandidate = true // Will be verified during resolution + } + + if (shouldAddCandidate) { + candidates.push(binding.init) + } }Note: The current implementation may already be optimal since any filtering here would require similar checks to the resolution phase. Profile before optimizing.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
labeler-config.ymlpackages/start-plugin-core/src/create-server-fn-plugin/compiler.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tspackages/start-plugin-core/src/plugin.tspackages/start-plugin-core/src/types.tspackages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- labeler-config.yml
- packages/start-plugin-core/src/plugin.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/src/types.tspackages/start-plugin-core/src/create-server-fn-plugin/compiler.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/src/types.tspackages/start-plugin-core/src/create-server-fn-plugin/compiler.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/src/types.ts
🧬 Code graph analysis (3)
packages/start-plugin-core/src/types.ts (1)
packages/start-plugin-core/src/schema.ts (1)
TanStackStartOutputConfig(207-207)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (3)
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts (1)
handleCreateMiddleware(10-45)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateIsomorphicFn.ts (1)
handleCreateIsomorphicFn(4-46)packages/start-plugin-core/src/create-server-fn-plugin/handleEnvOnly.ts (1)
handleEnvOnlyFn(10-45)
packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (2)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (2)
LookupKind(35-40)LookupConfig(89-93)packages/start-plugin-core/src/types.ts (1)
CompileStartFrameworkOptions(3-3)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (7)
packages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.ts (1)
1-120: LGTM! Test refactoring properly implements the new async compiler API.The test file correctly migrates from the previous factory pattern to the new
ServerFnCompilerclass-based approach. The async patterns, error handling withrejects.toThrowError(), and console spy implementation are all properly structured.packages/start-plugin-core/src/types.ts (1)
1-34: LGTM! Clean type definitions with proper separation of concerns.The type definitions are well-structured and correctly centralize the public API types. The interfaces are clear and properly typed.
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (5)
35-69: Well-designed multi-kind detection strategy.The expanded
LookupKindtypes and dual detection strategies (method chain vs direct call) are well-structured. TheallowRootAsCandidateflag forIsomorphicFn(line 65) correctly handles the case wherecreateIsomorphicFn()is called without.client()or.server(), resulting in a no-op function.The design allowing multiple kinds to share identifiers (e.g., 'server' and 'client' used by both
MiddlewareandIsomorphicFn) is correctly handled by usingSet<LookupKind>in theIdentifierToKindsmap.
71-87: Efficient pre-computation of identifier-to-kinds map.The pre-computed
IdentifierToKindsmap enables O(1) lookup during candidate detection and correctly handles multiple kinds sharing the same identifier name. The iteration and Set building logic is correct.
705-737: Correct handling of shared identifiers across multiple kinds.The logic for resolving method chain patterns that share identifiers (lines 706-736) correctly:
- Uses
IdentifierToKinds.get(prop)to find all possible kinds for an identifier (O(1) lookup)- Resolves the base expression once and reuses it for all pattern checks (avoiding redundant work)
- Checks each possible kind against the valid lookup kinds
- Returns the first matching kind
The specific kind checks (lines 715-735) properly validate the base kind before returning the target kind.
139-197: Verify async error handling in initialization.The
init()method performs async operations inPromise.all()(lines 151-194). IfresolveIdfails for any configuration, it will throw an error (line 164), which correctly propagates. However, ensure that partial initialization doesn't leave the compiler in an inconsistent state.The
initializedflag is set totrueonly after all configurations are processed (line 196), which is correct. But if an error occurs:
- Line 320 will call
init()again on the nextcompile()call- The moduleCache may have partial entries from the failed initialization
Consider testing error handling during initialization:
// Test case suggestion test('should handle initialization errors gracefully', async () => { const compiler = new ServerFnCompiler({ env: 'client', directive: 'use server', lookupKinds: new Set(['IsomorphicFn']), lookupConfigurations: [ { libName: 'non-existent-package', rootExport: 'createIsomorphicFn', kind: 'IsomorphicFn', }, ], loadModule: async () => {}, resolveId: async (id) => { if (id === 'non-existent-package') { return null // Simulate package not found } return id }, }) await expect( compiler.compile({ code: 'const x = 1', id: 'test.ts', isProviderFile: false, }) ).rejects.toThrow('could not resolve "non-existent-package"') // Verify compiler can recover if configuration is fixed // (This may not be the intended behavior, just verifying) })
139-149: The@tanstack/start-fn-stubspackage exists in the monorepo atpackages/start-fn-stubs/with version 1.142.8 and is properly configured with exports. The three hardcoded functions—createIsomorphicFn,createServerOnlyFn, andcreateClientOnlyFn—are all correctly exported and match theKindtype union defined in the compiler. The registration in theinit()method is accurate.
schiller-manuel
changed the title
fixes #6169
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.