-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: correct compiler plugin order, only handle createMiddleware in a … #6163
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
…single plugin fixes #5381
WalkthroughThis PR introduces support for calling server functions from within client-only functions by restructuring the build-time server function and middleware plugin pipeline. It adds a new test route demonstrating this pattern, guards against server-side execution of certain transformations, reorders plugin execution to perform server function transformations before dead-code elimination, and removes createMiddleware transformation logic from the start compiler plugin in favor of concentrating that responsibility in the create-server-fn plugin. 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)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 2de05a0
☁️ Nx Cloud last updated this comment at |
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: 0
🧹 Nitpick comments (2)
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts (1)
5-13: Consider narrowing the type signature or documenting the constraint.The function accepts
env: 'client' | 'server'but immediately throws ifenv === 'server'. While this defensive guard is appropriate for catching misuse, consider either:
- Narrowing the type to
env: 'client'if this function should never receive a server env- Adding a JSDoc comment explaining why server is rejected
This would make the API contract clearer to callers.
🔎 Option 1: Narrow the type
export function handleCreateMiddleware( path: babel.NodePath<t.CallExpression>, opts: { - env: 'client' | 'server' + env: 'client' }, ) { - if (opts.env === 'server') { - throw new Error('handleCreateMiddleware should not be called on the server') - }packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.ts (1)
10-14: Consider whether to preserve the dual-environment type signature.The
compilehelper still acceptsenv: 'client' | 'server', but after this change, only'client'is used in the tests. This isn't necessarily wrong—keeping the flexible signature allows for easier addition of server-side tests in the future if needed.However, if middleware compilation is definitively client-only (per line 53's comment), you might consider either:
- Adding a clarifying comment explaining why the server option remains, or
- Narrowing the type to
env: 'client'to match the actual usage
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
e2e/react-start/server-functions/src/routeTree.gen.ts(11 hunks)e2e/react-start/server-functions/src/routes/server-fn-in-client-only-fn.tsx(1 hunks)e2e/react-start/server-functions/tests/server-functions.spec.ts(0 hunks)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts(2 hunks)packages/start-plugin-core/src/plugin.ts(2 hunks)packages/start-plugin-core/src/start-compiler-plugin/compilers.ts(0 hunks)packages/start-plugin-core/src/start-compiler-plugin/constants.ts(0 hunks)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.ts(1 hunks)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/create-function-middleware.ts(0 hunks)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructured.tsx(1 hunks)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructuredRename.tsx(1 hunks)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareStarImport.tsx(1 hunks)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareValidator.tsx(0 hunks)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/server/createStart.tsx(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/createMiddleware.test.ts(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/client/createMiddlewareDestructured.tsx(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/client/createMiddlewareDestructuredRename.tsx(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/client/createMiddlewareStarImport.tsx(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/client/factory.ts(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/create-function-middleware.ts(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/createMiddlewareDestructured.tsx(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/createMiddlewareDestructuredRename.tsx(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/createMiddlewareStarImport.tsx(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/createMiddlewareValidator.tsx(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/factory.ts(0 hunks)packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/test-files/factory.ts(0 hunks)
💤 Files with no reviewable changes (18)
- packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareValidator.tsx
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/createMiddlewareValidator.tsx
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/create-function-middleware.ts
- packages/start-plugin-core/src/start-compiler-plugin/compilers.ts
- packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/server/createStart.tsx
- packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/create-function-middleware.ts
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/client/createMiddlewareDestructuredRename.tsx
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/client/createMiddlewareStarImport.tsx
- e2e/react-start/server-functions/tests/server-functions.spec.ts
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/createMiddleware.test.ts
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/client/factory.ts
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/client/createMiddlewareDestructured.tsx
- packages/start-plugin-core/src/start-compiler-plugin/constants.ts
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/test-files/factory.ts
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/factory.ts
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/createMiddlewareStarImport.tsx
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/createMiddlewareDestructured.tsx
- packages/start-plugin-core/tests/createMiddleware-start-compiler-plugin/snapshots/server/createMiddlewareDestructuredRename.tsx
🧰 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/src/routes/server-fn-in-client-only-fn.tsxpackages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.tspackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareStarImport.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.tspackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructuredRename.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructured.tsxpackages/start-plugin-core/src/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/src/routes/server-fn-in-client-only-fn.tsxpackages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.tspackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareStarImport.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.tspackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructuredRename.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructured.tsxpackages/start-plugin-core/src/plugin.tse2e/react-start/server-functions/src/routeTree.gen.ts
🧠 Learnings (4)
📚 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/server-fn-in-client-only-fn.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareStarImport.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.tspackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructuredRename.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructured.tsxe2e/react-start/server-functions/src/routeTree.gen.ts
📚 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/src/routes/server-fn-in-client-only-fn.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareStarImport.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructuredRename.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructured.tsxpackages/start-plugin-core/src/plugin.ts
📚 Learning: 2025-12-16T02:59:11.506Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6104
File: packages/start-plugin-core/tests/split-exports-plugin/snapshots/exports/functionExports.ts:1-1
Timestamp: 2025-12-16T02:59:11.506Z
Learning: In transformation test snapshots (e.g., split-exports plugin), comments at the top of snapshot files often describe the original input scenario being tested (e.g., "Multiple function exports") rather than the transformed output in the snapshot itself. This helps document what transformation is being validated.
Applied to files:
packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareStarImport.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructuredRename.tsxpackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructured.tsx
📚 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
🧬 Code graph analysis (4)
e2e/react-start/server-functions/src/routes/server-fn-in-client-only-fn.tsx (2)
packages/start-client-core/src/index.tsx (1)
createClientOnlyFn(12-12)packages/router-core/src/ssr/tsrScript.ts (1)
p(16-18)
packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.ts (1)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (1)
compile(204-282)
packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructured.tsx (6)
packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructuredRename.tsx (4)
withUseServer(2-4)withoutUseServer(5-7)withVariable(8-10)withZodValidator(11-13)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareStarImport.tsx (4)
withUseServer(2-4)withoutUseServer(5-7)withVariable(8-10)withZodValidator(11-13)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareValidator.tsx (1)
withUseServer(2-4)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/test-files/createMiddlewareDestructuredRename.tsx (4)
withUseServer(4-12)withoutUseServer(14-22)withVariable(24-26)withZodValidator(45-51)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/test-files/createMiddlewareDestructured.tsx (4)
withUseServer(4-12)withoutUseServer(14-22)withVariable(24-26)withZodValidator(45-51)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/test-files/createMiddlewareStarImport.tsx (4)
withUseServer(4-13)withoutUseServer(15-23)withVariable(25-27)withZodValidator(46-52)
packages/start-plugin-core/src/plugin.ts (1)
packages/start-plugin-core/src/start-compiler-plugin/plugin.ts (1)
startCompilerPlugin(69-111)
⏰ 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 (13)
packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructuredRename.tsx (1)
1-13: LGTM! Test snapshot correctly demonstrates renamed import transformation.This snapshot file properly validates that the plugin handles
createMiddlewarewhen imported with a rename (as middlewareFn). The identical configurations across all four exports are intentional for testing that the transformation uniformly processes different middleware instances under the renamed import pattern.packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts (2)
51-56: LGTM!The simplified logic correctly removes the validator call expression by replacing it with the callee's object. The
isMemberExpressioncheck provides appropriate type safety.
63-70: LGTM!The conditional properly checks for the existence of both
callExpressionPaths.serverandserverFnPath.nodebefore performing the transformation. The simplification is valid since the early guard ensures this only executes in client context.packages/start-plugin-core/src/plugin.ts (1)
388-434: Excellent fix with clear documentation!The plugin reordering correctly addresses issue #5381. The comment clearly explains the critical ordering requirement:
createServerFnPlugininjects'use server'directiveTanStackServerFnPluginextracts and registers functions in manifeststartCompilerPluginruns DCE lastThis ensures server functions referenced inside
createClientOnlyFncallbacks are registered in the manifest before DCE can remove them. The detailed comment will help future maintainers understand why this order matters.packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareDestructured.tsx (1)
1-13: LGTM! Client-side transformation snapshot is correct.This test snapshot correctly demonstrates the expected client-side output after the plugin processes
createMiddlewarecalls. Comparing to the test input file, the.server()method chains and their implementations have been properly stripped, leaving only the basecreateMiddleware({ id: 'test' })calls—which is the expected behavior for client bundles.The snapshot aligns with the PR objective of consolidating createMiddleware handling in a single plugin and validates that the plugin ordering fix maintains correct transformation behavior.
packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/snapshots/client/createMiddlewareStarImport.tsx (1)
1-13: LGTM! Valid test snapshot documenting star import transformation.This snapshot correctly represents the expected client-side output after the plugin transforms
createMiddlewarecalls using a star import pattern. The identical configurations across all four exports (id: 'test') are intentional, as this tests the transformation behavior rather than runtime logic. The filename clearly describes the test scenario being validated.Based on learnings, transformation test snapshots document what transformation is being validated and serve as fixtures for verifying plugin output.
e2e/react-start/server-functions/src/routes/server-fn-in-client-only-fn.tsx (5)
1-3: LGTM: Imports are appropriate for the test case.The imports correctly include the necessary TanStack Start primitives (
createServerFn,createClientOnlyFn) and Router utilities to demonstrate the fix for issue #5381.
5-8: LGTM: Server function definition is correct.The server function uses the standard
createServerFn().handler()pattern and returns a simple string, which is appropriate for this test scenario.
10-18: LGTM: Core test case correctly demonstrates the fix.The client-only function properly calls the server function and transforms its result. The inline comment (lines 11-14) provides valuable context about the compilation order issue that this PR addresses, making this an excellent regression test.
20-22: LGTM: Route definition follows standard patterns.The file route is correctly configured with an appropriate path name that describes the test scenario.
24-63: LGTM: Test component is well-structured.The component implementation includes:
- Proper error handling with try/catch
- Clear test-ids for e2e test assertions
- Expected result display that correctly matches the composed output from both functions
- Conditional rendering of result and error states
This provides a robust test case for the compilation order fix.
e2e/react-start/server-functions/src/routeTree.gen.ts (1)
1-705: Autogenerated file - changes are expected.This
routeTree.gen.tsfile is autogenerated by TanStack Router and correctly includes the new/server-fn-in-client-only-fnroute across all type declarations, route mappings, and the route tree structure. No manual modifications are needed.Based on learnings, autogenerated route tree files should not be manually modified or have issues flagged.
packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.ts (1)
53-60: The comment "Middleware compilation only happens on the client" is verified and correct.The
handleCreateMiddlewarefunction explicitly throws an error if called withenv: 'server', confirming that middleware processing is intentionally client-only. Server-side test removal is appropriate since server environment processing is unsupported by design.However, consider whether the compiler should guard against passing server env at the call site (compiler.ts line 270-271) to provide clearer error messaging, rather than relying on the handler rejection. This would shift the error earlier in the compilation pipeline.
…single plugin
fixes #5381
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.