fix: createIsomorphicFn compilation #6220
Conversation
📝 WalkthroughWalkthroughThis PR enhances the isomorphic function system to support module-level invocations. The runtime now returns a callable function with attached methods, while the compiler is refactored to better track function references across variable assignments and detect their invocation context, enabling proper environment-specific implementation inlining at the module level. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit c7a8a75
☁️ Nx Cloud last updated this comment at |
schiller-manuel
force-pushed
the
fix-compiler
branch
from
89a3c98 to
c7a8a75
Compare
![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 (7)
packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/call-at-module-level.ts (1)
1-1: Consider adding a header comment describing the input scenario.Based on learnings, transformation test snapshots benefit from a brief header comment at the top describing the original input scenario being tested (e.g., "Input: Module-level isomorphic function invocation"). This makes snapshots self-describing.
🔎 Suggested header comment
+// Input: Module-level invocation of createIsomorphicFn (client environment) + const getEnvironment = () => { console.log('[CLIENT] getEnvironment called'); return 'client';packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructuredRename.tsx (1)
1-2: LGTM! Factory-based initialization pattern applied correctly.The change from a no-op function to
isomorphicFn()aligns with the new factory-based approach for isomorphic functions. The destructured import with rename is handled correctly.Consider adding a header comment describing the input scenario per snapshot documentation patterns (e.g., "Input: Destructured import with rename of createIsomorphicFn").
packages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/call-at-module-level.ts (1)
1-1: Consider adding a header comment describing the input scenario.Based on learnings, transformation test snapshots benefit from a brief header comment at the top describing the original input scenario being tested (e.g., "Input: Module-level isomorphic function invocation"). This makes snapshots self-describing.
🔎 Suggested header comment
+// Input: Module-level invocation of createIsomorphicFn (server environment) + const getEnvironment = () => { console.log('[SERVER] getEnvironment called'); return 'server';packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnStarImport.tsx (1)
1-2: LGTM! Star import pattern compiled correctly.The snapshot correctly demonstrates that star imports (
* as TanStackStart) work with the factory-based initialization pattern. The compiler properly handlesTanStackStart.createIsomorphicFn().Consider adding a header comment describing the input scenario per snapshot documentation patterns (e.g., "Input: Star import of @tanstack/react-start").
packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructured.tsx (1)
1-2: LGTM! Destructured import compiled correctly.The factory-based initialization pattern is correctly applied with destructured imports. The compiler properly transforms
createIsomorphicFn()for the client environment.Consider adding a header comment describing the input scenario per snapshot documentation patterns (e.g., "Input: Destructured import of createIsomorphicFn").
packages/start-plugin-core/src/start-compiler-plugin/compiler.ts (2)
818-836: Consider the implications of parallel resolution.The candidates are resolved in parallel via
Promise.all, which is good for performance. However, if two candidates share a binding that needs resolution, they may both attempt to resolve it concurrently. Thevisitedset is per-call, not shared, so this shouldn't cause issues, but it could result in redundant resolution work.This is acceptable given the benefits of parallel resolution, but worth noting if performance profiling shows repeated resolution of the same bindings.
496-497: Add a comment clarifying the purpose of the'*'export entry for namespace imports.The
exports.set('*', config.rootExport)entry enables namespace import resolution. When code likeimport * as TanStackStart from '@tanstack/react-start'is processed, the binding hasimportedName: '*', which is then resolved by looking upexports.get('*'). Adding a brief inline comment explaining this would improve clarity for future maintainers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/start-fn-stubs/src/createIsomorphicFn.tspackages/start-fn-stubs/tests/createIsomorphicFn.test-d.tspackages/start-plugin-core/src/start-compiler-plugin/compiler.tspackages/start-plugin-core/src/start-compiler-plugin/handleCreateIsomorphicFn.tspackages/start-plugin-core/tests/clientOnlyJSX/snapshots/server/clientOnlyNotFromTanstack.tsxpackages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/call-at-module-level.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/createIsomorphicFnFactory.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/call-at-module-level.tspackages/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/createIsomorphicFnFactory.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/test-files/call-at-module-level.ts
💤 Files with no reviewable changes (2)
- packages/start-plugin-core/src/start-compiler-plugin/handleCreateIsomorphicFn.ts
- packages/start-fn-stubs/tests/createIsomorphicFn.test-d.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/snapshots/client/call-at-module-level.tspackages/start-plugin-core/tests/createIsomorphicFn/test-files/call-at-module-level.tspackages/start-plugin-core/tests/clientOnlyJSX/snapshots/server/clientOnlyNotFromTanstack.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/call-at-module-level.tspackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructuredRename.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnFactory.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/src/start-compiler-plugin/compiler.tspackages/start-fn-stubs/src/createIsomorphicFn.tspackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnFactory.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructuredRename.tsx
**/*.{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/snapshots/client/call-at-module-level.tspackages/start-plugin-core/tests/createIsomorphicFn/test-files/call-at-module-level.tspackages/start-plugin-core/tests/clientOnlyJSX/snapshots/server/clientOnlyNotFromTanstack.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/call-at-module-level.tspackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructuredRename.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnFactory.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/src/start-compiler-plugin/compiler.tspackages/start-fn-stubs/src/createIsomorphicFn.tspackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnFactory.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructuredRename.tsx
🧠 Learnings (5)
📚 Learning: 2025-12-16T02:59:06.535Z
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:06.535Z
Learning: When adding or updating snapshot files (especially transformation tests like split-exports-plugin), place a brief header comment at the top describing the input scenario being tested (e.g., "Input: Multiple function exports"). This documents what transformation is validated and makes snapshots self-describing. Apply this pattern to all TS snapshot files under any snapshots directory.
Applied to files:
packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/call-at-module-level.tspackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/call-at-module-level.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/createIsomorphicFn/snapshots/client/call-at-module-level.tspackages/start-plugin-core/tests/clientOnlyJSX/snapshots/server/clientOnlyNotFromTanstack.tsx
📚 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/test-files/call-at-module-level.tspackages/start-plugin-core/tests/clientOnlyJSX/snapshots/server/clientOnlyNotFromTanstack.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructuredRename.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.tspackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnFactory.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructuredRename.tsx
📚 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/clientOnlyJSX/snapshots/server/clientOnlyNotFromTanstack.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructuredRename.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnStarImport.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnFactory.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructured.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnFactory.tsxpackages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnDestructuredRename.tsx
📚 Learning: 2025-12-21T12:52:35.231Z
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:35.231Z
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:
packages/start-plugin-core/src/start-compiler-plugin/compiler.ts
🧬 Code graph analysis (5)
packages/start-plugin-core/tests/createIsomorphicFn/test-files/call-at-module-level.ts (1)
packages/start-fn-stubs/src/createIsomorphicFn.ts (1)
createIsomorphicFn(36-42)
packages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructured.tsx (3)
packages/start-fn-stubs/src/createIsomorphicFn.ts (1)
createIsomorphicFn(36-42)packages/start-fn-stubs/src/index.ts (1)
createIsomorphicFn(2-2)packages/start-client-core/src/index.tsx (1)
createIsomorphicFn(6-6)
packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnFactory.tsx (1)
packages/start-fn-stubs/src/createIsomorphicFn.ts (1)
createIsomorphicFn(36-42)
packages/start-plugin-core/src/start-compiler-plugin/compiler.ts (2)
packages/start-plugin-core/src/plugin.ts (1)
config(104-346)packages/router-core/src/route.ts (1)
path(1553-1555)
packages/start-fn-stubs/src/createIsomorphicFn.ts (2)
packages/start-fn-stubs/src/index.ts (1)
IsomorphicFnBase(6-6)packages/start-client-core/src/index.tsx (1)
IsomorphicFnBase(12-12)
⏰ 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 (24)
packages/start-plugin-core/tests/clientOnlyJSX/snapshots/server/clientOnlyNotFromTanstack.tsx (1)
1-1: LGTM! Test snapshot correctly reflects expected compiler behavior.The "no-transform" marker appropriately indicates that ClientOnly components not originating from @TanStack should remain untransformed by the compiler. This snapshot update aligns with the PR's refactoring of JSX handling and compiler simplification.
packages/start-fn-stubs/src/createIsomorphicFn.ts (2)
24-31: LGTM! Clean separation of base interface.The removal of
extends IsomorphicFnfromIsomorphicFnBaseand the explicit declaration of bothserverandclientmethods create a clean, symmetric interface for the factory. This supports the module-level invocation pattern correctly.
36-42: LGTM! Function-based factory enables module-level invocation.The use of
Object.assignto attachserverandclientmethods to a function value is the correct approach to enable module-level invocation. Theas anycast is justified since this is a runtime stub that will be replaced by the transformer.packages/start-plugin-core/tests/createIsomorphicFn/test-files/call-at-module-level.ts (1)
1-13: Excellent test case for module-level invocation support!This test file directly addresses issue #6217 by demonstrating that
createIsomorphicFnnow works correctly when invoked at module top-level. The test validates that:
- The isomorphic function can be created and configured at module scope
- Module-level invocation (
getEnvironment()) returns the expected value based on environment- Both server and client implementations are properly resolved
packages/start-plugin-core/tests/createIsomorphicFn/createIsomorphicFn.test.ts (2)
3-3: LGTM! Test cleanup aligns with implementation changes.The removal of console warning tests is appropriate given the changes to support module-level invocation. The factory-based approach no longer requires runtime warnings for no-implementation cases since the stub function is now callable by design.
77-97: Good test coverage for error cases.The error tests correctly validate that incomplete implementation chains (e.g.,
.client()or.server()called without providing an implementation function) still throw errors as expected. This maintains safety while supporting the new module-level invocation pattern.packages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructuredRename.tsx (1)
1-2: LGTM! Snapshot correctly demonstrates aliased import handling.The snapshot correctly shows that the compiler preserves the
createIsomorphicFn()factory call at module level when using an aliased import. This validates the fix for cross-variable resolution (issue #6206) and module-level support (issue #6217).packages/start-plugin-core/tests/createIsomorphicFn/snapshots/client/createIsomorphicFnFactory.tsx (1)
22-24: LGTM! Factory pattern correctly preserved in client snapshot.The snapshot correctly demonstrates that
createNoImplFncan return acreateIsomorphicFn()call, validating that the compiler properly handles factory-wrapped isomorphic functions. This addresses the cross-variable resolution fix (issue #6206).packages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnFactory.tsx (1)
22-24: LGTM! Server snapshot mirrors client factory behavior.The server snapshot correctly shows the same factory pattern as the client snapshot, with
createNoImplFnreturningcreateIsomorphicFn(). This validates consistent compiler behavior across both build targets.packages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnStarImport.tsx (1)
1-2: LGTM! Namespace import pattern correctly handled.The snapshot correctly demonstrates that the compiler can resolve
createIsomorphicFnthrough a namespace import (TanStackStart.createIsomorphicFn()). This validates comprehensive import pattern support for the cross-variable resolution fix (issue #6206).packages/start-plugin-core/tests/createIsomorphicFn/snapshots/server/createIsomorphicFnDestructured.tsx (1)
1-2: LGTM! Standard import pattern correctly preserved.The snapshot correctly demonstrates the most common usage pattern with a standard destructured import and direct module-level call to
createIsomorphicFn(). This validates the core module-level support fix (issue #6217).packages/start-plugin-core/src/start-compiler-plugin/compiler.ts (13)
51-77: LGTM!The
DirectCallSetuptype withfactoryNameproperty is a clean approach to distinguish factory function calls (e.g.,createServerOnlyFn()) from invocations of already-created functions. The naming is consistent withKindDetectionPatterns.
136-138: LGTM!Using
Object.keys(LookupSetup)as a single source of truth for iteration ensures the array stays synchronized with theLookupSetuprecord. The type assertion is safe given theRecord<LookupKind, ...>constraint.
176-185: LGTM!Pre-computing
DirectCallFactoryNamesat module initialization provides O(1) lookup for validating nested direct call candidates, avoiding repeated iteration during compilation.
193-200: LGTM!Simplifying
exportsto a plainMap<string, string>(exported name → local binding name) reduces complexity while maintaining all necessary functionality. The comment clearly documents the mapping semantics.
413-420: LGTM!Lazy caching of
rootWithTrailingSlashis appropriate since the root path doesn't change during the compiler's lifetime. This avoids repeated string operations ingenerateFunctionId.
556-599: LGTM!The export handling correctly populates the simplified
exportsmap:
- Named declarations map to themselves
- Renamed exports map exported name → local name
- Default exports use the identifier name or a synthetic binding
784-810: LGTM!The JSX handling now validates candidates during traversal rather than deferring validation:
- Checks the component is imported (not locally defined)
- Verifies the import source is a known TanStack package
- Confirms the import resolves to
ClientOnlyJSXThis prevents false positives from user-defined components with the same name.
1009-1021: LGTM!The two-step lookup (export name → local binding name → binding) correctly implements resolution with the simplified exports map structure.
1126-1146: LGTM!The
isKnownFactoryImporthelper is the key fix for the linked issues. It correctly distinguishes:
- ✅
createServerOnlyFn(() => ...)— factory import, should be a candidate- ✅
import { createServerOnlyFn as myFn } from '...'— renamed import, still a factory- ❌
myServerOnlyFn()— invoking an already-built function, NOT a candidateThis prevents the compiler from incorrectly treating function invocations as transformation candidates.
1180-1202: LGTM!This is the core fix for the linked issues. The logic now correctly handles:
- Method chains (
.server(),.client()): Returns the resolved kind if valid- Direct calls (identifier callee): Only treats as a candidate if
isKnownFactoryImportconfirms the callee is an actual factory function importThis prevents the bug where
myIsomorphicFn()(invoking an already-built isomorphic function) was incorrectly treated as a transformation candidate.
1276-1288: LGTM!The namespace import resolution correctly uses the two-step lookup pattern with the simplified exports map, consistent with
findExportInModule.
204-213: LGTM!The comment accurately reflects that this check applies to
directCalltypes.
1-21: LGTM!The file structure is well-organized with clear separation between detection, resolution, and compilation phases. The imports are appropriately scoped and the eslint disable comment for CommonJS is at the top.
fixes #6217
fixes #6206
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.