performance: no reactivity on the server #6497
Conversation
schiller-manuel
changed the title
📝 WalkthroughWalkthroughAdds SSR-aware short‑circuits in framework useRouterState hooks and a lightweight server router store: server paths bypass deep-equality, batching, and subscriptions; client behavior and public APIs remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as RouterCore.build()
participant Env as isServer
participant ServerStore as createServerStore
participant ClientStore as Store(onUpdate)
participant Batch as batch()
Builder->>Env: check isServer
alt isServer == true
Builder->>ServerStore: createServerStore(initialState)
ServerStore-->>Builder: state holder (state + setState)
note right of ServerStore: no subscriptions\nno batching
else isServer == false
Builder->>ClientStore: new Store(initialState, { onUpdate })
ClientStore->>Batch: wrap updates with batch(fn)
Batch-->>ClientStore: execute batched updates
ClientStore-->>Subscribers: notify subscribers (useRouterState, etc.)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit db465e1
☁️ 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: 2
🤖 Fix all issues with AI agents
In `@packages/router-core/src/router.ts`:
- Around line 902-923: The server store is being unsafely cast to a generic
Store which weakens type safety; change the __store declaration to use the
concrete RouterStateStore type produced by createServerStore and parameterize it
with RouterState<TRouteTree> (i.e., use
RouterStateStore<RouterState<TRouteTree>> instead of casting to Store<any>),
remove the cast, and ensure usages rely on the strongly typed .state and
.setState provided by createServerStore so the compiler enforces correct types.
In `@packages/router-core/src/utils/batch.ts`:
- Around line 5-16: The current batch implementation uses the module-level
isServer flag and ignores any runtime override on the router, so calling batch
will use storeBatch even when router.options.isServer requests server behavior;
update batch to accept or consult a runtime override (e.g., an optional second
parameter or check a provided runtime flag) and prefer router.options.isServer
when present, or expose a wrapper used by callers that passes
router.options.isServer into batch; specifically, modify the export function
batch<T>(fn: () => T[, runtimeIsServer?: boolean]) to check runtimeIsServer ||
isServer before calling storeBatch (or change call sites to pass
router.options.isServer into batch), ensuring symbols batch, isServer,
storeBatch and router.options.isServer are used to determine the correct branch
at runtime.
| type RouterStateStore<TState> = { | ||
| state: TState | ||
| setState: (updater: (prev: TState) => TState) => void | ||
| } | ||
|
|
||
| function createServerStore<TState>( | ||
| initialState: TState, | ||
| ): RouterStateStore<TState> { | ||
| let state = initialState | ||
|
|
||
| return { | ||
| get state() { | ||
| return state | ||
| }, | ||
| set state(next) { | ||
| state = next | ||
| }, | ||
| setState: (updater: (prev: TState) => TState) => { | ||
| state = updater(state) | ||
| }, | ||
| } as RouterStateStore<TState> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the Store type definition and __store usage patterns
rg -n "type Store<|interface Store<" packages/router-core/src/ -A 10Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Find all __store usages to see what methods are called
rg -n "\.__store\." packages/router-core/src/ -B 2 -A 2Repository: TanStack/router
Length of output: 5763
🏁 Script executed:
# Check the RouterStateStore type definition and createServerStore function
cat -n packages/router-core/src/router.ts | sed -n '902,923p'Repository: TanStack/router
Length of output: 667
🏁 Script executed:
# Look for the __store assignment and its type annotation
rg -n "__store" packages/router-core/src/router.ts -B 5 -A 5 | head -50Repository: TanStack/router
Length of output: 1499
🏁 Script executed:
# Find the Store type/class definition
rg -n "class Store<|interface Store<|type Store<|export.*Store" packages/router-core/src/ -A 15 | head -100Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if Store is imported from elsewhere
rg -n "^import.*Store|^export.*Store" packages/router-core/src/router.ts -B 2 -A 2Repository: TanStack/router
Length of output: 230
🏁 Script executed:
# Search for Store definition in the entire packages/router-core
fd -e ts -e tsx . packages/router-core/src/ | xargs rg -l "class Store|interface Store" | head -5Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if .subscribe is called on __store or router store
rg -n "\.subscribe\(|subscribe.*store|store.*subscribe" packages/router-core/src/ -iRepository: TanStack/router
Length of output: 417
🏁 Script executed:
# Check what methods are actually called on __store across all usages
rg -n "\.__store\.[a-zA-Z_$]+" packages/router-core/src/ -o --replace '$0' | sort | uniq -c | sort -rnRepository: TanStack/router
Length of output: 1183
🏁 Script executed:
# Search for Store method calls to identify the actual API
rg -n "subscribe|notify|getState|setState" packages/router-core/src/router.ts -iRepository: TanStack/router
Length of output: 1191
🏁 Script executed:
# Check if Store is used elsewhere to understand its full API
rg -n "new Store\(" packages/router-core/src/ -B 2 -A 5 | head -40Repository: TanStack/router
Length of output: 720
Unsound Store cast weakens type safety for the server store.
createServerStore returns RouterStateStore<TState> but is coerced to Store<any> via cast, which hides the type mismatch and violates TypeScript strict mode requirements. While current usage only calls .state and .setState() (both implemented in RouterStateStore), the cast removes compiler protections. Per coding guidelines requiring extensive type safety, change __store to RouterStateStore<RouterState<TRouteTree>> and remove the cast.
♻️ Proposed refactor
- __store!: Store<RouterState<TRouteTree>>
+ __store!: RouterStateStore<RouterState<TRouteTree>>
- this.__store = createServerStore(
- getInitialRouterState(this.latestLocation),
- ) as unknown as Store<any>
+ this.__store = createServerStore(
+ getInitialRouterState(this.latestLocation),
+ )🤖 Prompt for AI Agents
In `@packages/router-core/src/router.ts` around lines 902 - 923, The server store
is being unsafely cast to a generic Store which weakens type safety; change the
__store declaration to use the concrete RouterStateStore type produced by
createServerStore and parameterize it with RouterState<TRouteTree> (i.e., use
RouterStateStore<RouterState<TRouteTree>> instead of casting to Store<any>),
remove the cast, and ensure usages rely on the strongly typed .state and
.setState provided by createServerStore so the compiler enforces correct types.
| // `@tanstack/store`'s `batch` is for reactive notification batching. | ||
| // On the server we don't subscribe/render reactively, so a lightweight | ||
| // implementation that just executes is enough. | ||
| export function batch<T>(fn: () => T): T { | ||
| if (isServer) { | ||
| return fn() | ||
| } | ||
|
|
||
| let result!: T | ||
| storeBatch(() => { | ||
| result = fn() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -a -t f 'isServer\.(ts|tsx|js|mjs|cjs)$' | head -n 20Repository: TanStack/router
Length of output: 41
🏁 Script executed:
rg -t ts -t tsx 'isServer' packages/router-core/src/utils/batch.ts -B 10 -A 5Repository: TanStack/router
Length of output: 86
🏁 Script executed:
rg 'isServer' packages/router-core/src/utils/batch.ts -B 10 -A 5Repository: TanStack/router
Length of output: 482
🏁 Script executed:
rg -l 'isServer' packages/router-core/src --type ts | head -20Repository: TanStack/router
Length of output: 426
🏁 Script executed:
fd -t f . packages/router-core/src/isServer | sortRepository: TanStack/router
Length of output: 195
🏁 Script executed:
cat packages/router-core/src/utils.ts | head -100Repository: TanStack/router
Length of output: 2732
🏁 Script executed:
rg 'export.*isServer' packages/router-core/src/isServer/Repository: TanStack/router
Length of output: 309
🏁 Script executed:
rg 'options.*isServer|isServer.*option' packages/router-core/src --type ts -B 3 -A 3Repository: TanStack/router
Length of output: 956
🏁 Script executed:
rg 'batch\(' packages/router-core/src --type ts -B 3 -A 3 | head -60Repository: TanStack/router
Length of output: 3336
🏁 Script executed:
cat packages/router-core/package.json | rg -A 20 'exports' | head -40Repository: TanStack/router
Length of output: 584
🏁 Script executed:
rg 'isServer' packages/router-core/package.jsonRepository: TanStack/router
Length of output: 1818
🏁 Script executed:
rg -A 50 '"./isServer"' packages/router-core/package.json | head -80Repository: TanStack/router
Length of output: 1628
🏁 Script executed:
rg 'new Router\|createRouter' packages/router-core/src --type ts -B 3 -A 5 | head -40Repository: TanStack/router
Length of output: 41
🏁 Script executed:
rg 'options\.isServer\s*[=:]|isServer\s*:\s*' packages/router-core/src --type ts -B 2 -A 2Repository: TanStack/router
Length of output: 295
🏁 Script executed:
rg 'isServer.*undefined|development' packages/router-core/src -B 2 -A 2 | head -30Repository: TanStack/router
Length of output: 1697
🏁 Script executed:
rg 'batch\(' packages/router-core/src -B 5 -A 5 | grep -A 5 -B 5 'this\.isServer\|router\.'Repository: TanStack/router
Length of output: 3417
Verify that batch respects forced server mode.
batch branches only on module-level isServer, which is set at build time via conditional exports. If router.options.isServer is explicitly set to force server behavior at runtime (where module-level isServer is false), this will still route through @tanstack/store batching instead of the lightweight path. Either confirm that the module-level isServer definition already accounts for runtime overrides, or consider whether batch call sites should pass a runtime override flag to respect router.options.isServer.
🤖 Prompt for AI Agents
In `@packages/router-core/src/utils/batch.ts` around lines 5 - 16, The current
batch implementation uses the module-level isServer flag and ignores any runtime
override on the router, so calling batch will use storeBatch even when
router.options.isServer requests server behavior; update batch to accept or
consult a runtime override (e.g., an optional second parameter or check a
provided runtime flag) and prefer router.options.isServer when present, or
expose a wrapper used by callers that passes router.options.isServer into batch;
specifically, modify the export function batch<T>(fn: () => T[,
runtimeIsServer?: boolean]) to check runtimeIsServer || isServer before calling
storeBatch (or change call sites to pass router.options.isServer into batch),
ensuring symbols batch, isServer, storeBatch and router.options.isServer are
used to determine the correct branch at runtime.
5c5a9c8 to
db465e1
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.