fix(start): initialize serialization adapters for early client server…#7708
Open
schiller-manuel wants to merge 1 commit into
Open
fix(start): initialize serialization adapters for early client server…#7708schiller-manuel wants to merge 1 commit into
schiller-manuel wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces ChangesPre-hydration serialization adapter fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit 85cabb9
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview2 package(s) bumped directly, 12 bumped as dependents. 🟩 Patch bumps
|
Contributor
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-client-core/src/client/hydrateStart.ts (1)
21-30: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClone the adapter list before merging router adapters.
serializationAdaptersreferences cachedstartOptions.serializationAdapters, sopush()mutates module-level Start options used later by client RPC serialization. Build a combined array instead, ideally deduping router adapters already present after retries/HMR.Proposed fix
- const startOptions = (await initStartOptions()) as AnyStartInstanceOptions - const serializationAdapters = startOptions.serializationAdapters - - if (router.options.serializationAdapters) { - serializationAdapters.push(...router.options.serializationAdapters) - } + const startOptions = + ((await initStartOptions()) ?? {}) as AnyStartInstanceOptions + const startSerializationAdapters = startOptions.serializationAdapters ?? [] + const routerSerializationAdapters = + router.options.serializationAdapters?.filter( + (adapter) => !startSerializationAdapters.includes(adapter), + ) ?? [] + const serializationAdapters = [ + ...startSerializationAdapters, + ...routerSerializationAdapters, + ] router.update({ basepath: process.env.TSS_ROUTER_BASEPATH, - ...{ serializationAdapters }, + serializationAdapters, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-client-core/src/client/hydrateStart.ts` around lines 21 - 30, Clone the adapter list in hydrateStart before merging router adapters: startOptions.serializationAdapters is being reused directly, so the push in the router.options.serializationAdapters branch mutates cached Start options. In hydrateStart, build a new combined array from startOptions.serializationAdapters and router.options.serializationAdapters, and pass that copy into router.update. Also ensure duplicate adapters are not re-added when hydrateStart runs again (for retries/HMR) by deduping based on adapter identity before updating the router.
🧹 Nitpick comments (2)
e2e/react-start/serialization-adapters/src/client.tsx (1)
39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the rejection as
unknownbefore reading error fields.
Promise.catchgives you an untyped rejection here, soerror?.name/error?.messagesidestep the strict-safety rule for this.tsxfile. Narrow it first and build the error payload from a checked shape. As per coding guidelines,**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Proposed fix
- .catch((error) => { + .catch((error: unknown) => { + const name = error instanceof Error ? error.name : 'Error' + const message = error instanceof Error ? error.message : String(error) + window.__serializationAdapterClientEntryResult = { status: 'error', - name: error?.name ?? 'Error', - message: error?.message ?? String(error), + name, + message, } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/react-start/serialization-adapters/src/client.tsx` around lines 39 - 44, The Promise.catch handler in client.tsx is reading `error?.name` and `error?.message` from an untyped rejection, which violates strict type safety; update the catch in the client entry flow to treat the rejection as `unknown` first, then narrow it with a checked shape before building `window.__serializationAdapterClientEntryResult`. Use the existing catch block in the client entry logic to extract `name` and `message` only after validation, and fall back safely for non-error values.Source: Coding guidelines
e2e/react-start/serialization-adapters/tests/app.spec.ts (1)
86-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
window as anyin the test predicate.This drops type safety in the one place the spec reads the browser-side contract it is asserting. A small local type for the result shape keeps the wait predicate and the final assertion aligned. As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Proposed fix
+type ClientEntryServerFnResult = + | { + status: 'success' + ping: string + shout: string + whisper: string + } + | { + status: 'error' + name: string + message: string + } + const result = await page .waitForFunction( - () => (window as any).__serializationAdapterClientEntryResult, + () => + ( + window as Window & { + __serializationAdapterClientEntryResult?: ClientEntryServerFnResult + } + ).__serializationAdapterClientEntryResult, ) - .then((handle) => handle.jsonValue()) + .then((handle) => handle.jsonValue() as Promise<ClientEntryServerFnResult>)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/react-start/serialization-adapters/tests/app.spec.ts` around lines 86 - 90, The test predicate in app.spec.ts is bypassing type safety by using window as any when reading __serializationAdapterClientEntryResult. Replace that cast with a small local TypeScript type that describes the browser-side result shape, and use it both in the waitForFunction predicate and the final jsonValue/assertion so they stay aligned. Keep the change scoped to the existing page.waitForFunction flow and the __serializationAdapterClientEntryResult contract.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/start-client-core/src/client-rpc/serverFnFetcher.ts`:
- Around line 114-116: The seroval plugin cache in serverFnFetcher should not be
initialized with a one-time fallback because it can lock in stale adapters from
the first call. Update the logic around getDefaultSerovalPlugins and
serovalPlugins so it recomputes or refreshes based on the current startOptions
instead of using ||=, and avoid module-level shared state that can leak across
concurrent calls. Make sure the behavior still works with hydrateStart mutating
serializationAdapters before later fetches.
In `@packages/start-client-core/src/getDefaultSerovalPlugins.ts`:
- Around line 9-11: `getDefaultSerovalPlugins` currently declares `start` as
`AnyStartInstanceOptions | undefined`, which still forces callers to pass an
argument and breaks the zero-arg uses in `staticFunctionMiddleware`. Update the
function signature in `getDefaultSerovalPlugins` to make `start` optional with
`start?: AnyStartInstanceOptions`, and keep the existing handling inside the
function compatible with an omitted value.
In `@packages/start-server-core/src/server-functions-handler.ts`:
- Around line 70-72: The plugin cache in handleServerAction is incorrectly
shared across requests, so it only reflects the first
startOptions/serializationAdapters it sees. Remove the module-level
serovalPlugins reuse and make the plugin list request-scoped by computing it
from the current startOptions inside handleServerAction for each call, or
otherwise key the cache by the incoming options. Keep the fix centered around
handleServerAction and getDefaultSerovalPlugins so each request gets the correct
server plugin set.
---
Outside diff comments:
In `@packages/start-client-core/src/client/hydrateStart.ts`:
- Around line 21-30: Clone the adapter list in hydrateStart before merging
router adapters: startOptions.serializationAdapters is being reused directly, so
the push in the router.options.serializationAdapters branch mutates cached Start
options. In hydrateStart, build a new combined array from
startOptions.serializationAdapters and router.options.serializationAdapters, and
pass that copy into router.update. Also ensure duplicate adapters are not
re-added when hydrateStart runs again (for retries/HMR) by deduping based on
adapter identity before updating the router.
---
Nitpick comments:
In `@e2e/react-start/serialization-adapters/src/client.tsx`:
- Around line 39-44: The Promise.catch handler in client.tsx is reading
`error?.name` and `error?.message` from an untyped rejection, which violates
strict type safety; update the catch in the client entry flow to treat the
rejection as `unknown` first, then narrow it with a checked shape before
building `window.__serializationAdapterClientEntryResult`. Use the existing
catch block in the client entry logic to extract `name` and `message` only after
validation, and fall back safely for non-error values.
In `@e2e/react-start/serialization-adapters/tests/app.spec.ts`:
- Around line 86-90: The test predicate in app.spec.ts is bypassing type safety
by using window as any when reading __serializationAdapterClientEntryResult.
Replace that cast with a small local TypeScript type that describes the
browser-side result shape, and use it both in the waitForFunction predicate and
the final jsonValue/assertion so they stay aligned. Keep the change scoped to
the existing page.waitForFunction flow and the
__serializationAdapterClientEntryResult contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a3f9a71-fce9-4c62-9871-929251eaefb8
📒 Files selected for processing (14)
.changeset/calm-adapters-serialize.mde2e/react-start/serialization-adapters/src/client-entry-server-functions.tse2e/react-start/serialization-adapters/src/client.tsxe2e/react-start/serialization-adapters/tests/app.spec.tspackages/router-core/src/index.tspackages/start-client-core/src/client-rpc/createClientRpc.tspackages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-client-core/src/client/hydrateStart.tspackages/start-client-core/src/createServerFn.tspackages/start-client-core/src/getDefaultSerovalPlugins.tspackages/start-client-core/src/getStartOptions.tspackages/start-client-core/src/global.tspackages/start-server-core/src/createStartHandler.tspackages/start-server-core/src/server-functions-handler.ts
💤 Files with no reviewable changes (1)
- packages/start-client-core/src/global.ts
Comment on lines
+114
to
+116
| startOptions: AnyStartInstanceOptions | undefined, | ||
| ) { | ||
| if (!serovalPlugins) { | ||
| serovalPlugins = getDefaultSerovalPlugins() | ||
| } | ||
| serovalPlugins ||= getDefaultSerovalPlugins(startOptions) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't freeze client seroval plugins on the first call.
serovalPlugins ||= ... snapshots the adapters from the first invocation. packages/start-client-core/src/client/hydrateStart.ts:19-32 later mutates startOptions.serializationAdapters, so any pre-hydration call can leave this fetch path permanently missing adapters added during hydration. Since this function is async, the module-level cache also creates cross-call leakage if different startOptions are observed concurrently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/start-client-core/src/client-rpc/serverFnFetcher.ts` around lines
114 - 116, The seroval plugin cache in serverFnFetcher should not be initialized
with a one-time fallback because it can lock in stale adapters from the first
call. Update the logic around getDefaultSerovalPlugins and serovalPlugins so it
recomputes or refreshes based on the current startOptions instead of using ||=,
and avoid module-level shared state that can leak across concurrent calls. Make
sure the behavior still works with hydrateStart mutating serializationAdapters
before later fetches.
Comment on lines
+9
to
+11
| export function getDefaultSerovalPlugins( | ||
| start: AnyStartInstanceOptions | undefined, | ||
| ): Array<Plugin<any, any>> { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type=ts '\bgetDefaultSerovalPlugins\s*\(' packagesRepository: TanStack/router
Length of output: 773
🏁 Script executed:
#!/bin/bash
sed -n '1,80p' packages/start-client-core/src/getDefaultSerovalPlugins.ts
printf '\n---\n'
sed -n '88,132p' packages/start-static-server-functions/src/staticFunctionMiddleware.ts
printf '\n---\n'
sed -n '100,130p' packages/start-client-core/src/client-rpc/serverFnFetcher.ts
printf '\n---\n'
sed -n '60,90p' packages/start-server-core/src/server-functions-handler.tsRepository: TanStack/router
Length of output: 3520
Make start optional. AnyStartInstanceOptions | undefined still requires an argument, so the zero-arg calls in packages/start-static-server-functions/src/staticFunctionMiddleware.ts:100 and :123 don’t match this signature. start?: AnyStartInstanceOptions aligns with the existing call sites.
Proposed fix
export function getDefaultSerovalPlugins(
- start: AnyStartInstanceOptions | undefined,
+ start?: AnyStartInstanceOptions,
): Array<Plugin<any, any>> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function getDefaultSerovalPlugins( | |
| start: AnyStartInstanceOptions | undefined, | |
| ): Array<Plugin<any, any>> { | |
| export function getDefaultSerovalPlugins( | |
| start?: AnyStartInstanceOptions, | |
| ): Array<Plugin<any, any>> { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/start-client-core/src/getDefaultSerovalPlugins.ts` around lines 9 -
11, `getDefaultSerovalPlugins` currently declares `start` as
`AnyStartInstanceOptions | undefined`, which still forces callers to pass an
argument and breaks the zero-arg uses in `staticFunctionMiddleware`. Update the
function signature in `getDefaultSerovalPlugins` to make `start` optional with
`start?: AnyStartInstanceOptions`, and keep the existing handling inside the
function compatible with an omitted value.
Comment on lines
70
to
+72
| // Initialize serovalPlugins lazily (cached at module level) | ||
| if (!serovalPlugins) { | ||
| serovalPlugins = getDefaultSerovalPlugins() | ||
| serovalPlugins = getDefaultSerovalPlugins(startOptions) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the server plugin list request-scoped.
handleServerAction now receives startOptions per call, but this cache only honors the first one. Later requests or handler instances with different serializationAdapters will still use the first plugin set.
Proposed fix
- if (!serovalPlugins) {
- serovalPlugins = getDefaultSerovalPlugins(startOptions)
- }
+ const serovalPlugins = getDefaultSerovalPlugins(startOptions)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Initialize serovalPlugins lazily (cached at module level) | |
| if (!serovalPlugins) { | |
| serovalPlugins = getDefaultSerovalPlugins() | |
| serovalPlugins = getDefaultSerovalPlugins(startOptions) | |
| // Initialize serovalPlugins lazily (cached at module level) | |
| const serovalPlugins = getDefaultSerovalPlugins(startOptions) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/start-server-core/src/server-functions-handler.ts` around lines 70 -
72, The plugin cache in handleServerAction is incorrectly shared across
requests, so it only reflects the first startOptions/serializationAdapters it
sees. Remove the module-level serovalPlugins reuse and make the plugin list
request-scoped by computing it from the current startOptions inside
handleServerAction for each call, or otherwise key the cache by the incoming
options. Keep the fix centered around handleServerAction and
getDefaultSerovalPlugins so each request gets the correct server plugin set.
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem serialization-payload (vue) |
6.8 MB | 9.5 MB | -28.77% |
| ❌ | Memory | mem serialization-payload (solid) |
6.8 MB | 7.1 MB | -3.3% |
| ⚡ | Memory | mem aborted-requests (solid) |
2.4 MB | 1.9 MB | +24.03% |
| ⚡ | Memory | mem peak-large-page (solid) |
3.9 MB | 3.4 MB | +14.19% |
| ⚡ | Memory | mem aborted-requests (vue) |
1,021 KB | 935.4 KB | +9.15% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix-7706 (85cabb9) with main (ba52d2b)1
Footnotes
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We updated staticFunctionMiddleware.ts to pass getStartOptions() to both calls of getDefaultSerovalPlugins, fixing the TypeScript error introduced when the PR changed the function signature to require a start argument. This call site in @tanstack/start-static-server-functions was the only one not updated alongside the refactor in @tanstack/start-client-core.
Tip
✅ We verified this fix by re-running @tanstack/start-static-server-functions:build, @tanstack/start-static-server-functions:test:types.
Suggested Fix changes
diff --git a/packages/start-client-core/src/index.tsx b/packages/start-client-core/src/index.tsx
index 5bf09c44..b322d19d 100644
--- a/packages/start-client-core/src/index.tsx
+++ b/packages/start-client-core/src/index.tsx
@@ -130,6 +130,7 @@ export type { Register } from '@tanstack/router-core'
export { getRouterInstance } from './getRouterInstance'
export { getDefaultSerovalPlugins } from './getDefaultSerovalPlugins'
+export { getStartOptions } from './getStartOptions'
export { getGlobalStartContext } from './getGlobalStartContext'
export { safeObjectMerge, createNullProtoObject } from './safeObjectMerge'
export { trackPostProcessPromise } from './client-rpc/serverFnFetcher'
diff --git a/packages/start-static-server-functions/src/staticFunctionMiddleware.ts b/packages/start-static-server-functions/src/staticFunctionMiddleware.ts
index e6681fce..f8cc6f06 100644
--- a/packages/start-static-server-functions/src/staticFunctionMiddleware.ts
+++ b/packages/start-static-server-functions/src/staticFunctionMiddleware.ts
@@ -3,6 +3,7 @@ import path from 'node:path'
import {
createMiddleware,
getDefaultSerovalPlugins,
+ getStartOptions,
} from '@tanstack/start-client-core'
import { fromJSON, toJSONAsync } from 'seroval'
@@ -97,7 +98,7 @@ async function addItemToCache({
result: response.result,
context: response.context.sendContext,
},
- { plugins: getDefaultSerovalPlugins() },
+ { plugins: getDefaultSerovalPlugins(getStartOptions()) },
),
)
await fs.writeFile(filePath, stringifiedResult, 'utf-8')
@@ -120,7 +121,9 @@ const fetchItem = async ({
method: 'GET',
})
.then((r) => r.json())
- .then((d) => fromJSON(d, { plugins: getDefaultSerovalPlugins() }))
+ .then((d) =>
+ fromJSON(d, { plugins: getDefaultSerovalPlugins(getStartOptions()) }),
+ )
return result
}
Or Apply changes locally with:
npx nx-cloud apply-locally XqTW-a3TK
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… functions
fixes #7706
Summary by CodeRabbit
Bug Fixes
Tests