[Flight] Add support for transporting Error.cause#536
Conversation
Greptile SummaryThis PR contains two logically distinct changes: 1. Flight: Add support for transporting
2. Fizz: Avoid unnecessary outlining for suspensey CSS during shell flush
Both changes include thorough test coverage. Confidence Score: 4/5
Important Files Changed
Last reviewed commit: d0fbb94 |
| if ('cause' in error) { | ||
| const cause: ReactClientValue = (error.cause: any); | ||
| const causeId = outlineModel(request, cause); | ||
| errorInfo.cause = serializeByValueID(causeId); | ||
| } |
There was a problem hiding this comment.
Cause serialization outside try-catch
In emitErrorChunk, the cause serialization is inside the try-catch block (lines 4263-4269), so if it fails, the error chunk is still emitted gracefully. Here in serializeErrorValue, the cause serialization is outside the try-catch. If outlineModel(request, cause) were to throw (e.g., due to an exotic cause value with throwing getters), this would propagate unhandled. Consider moving this block inside the existing try-catch for consistency with emitErrorChunk, or wrapping it in its own try-catch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-server/src/ReactFlightServer.js
Line: 4175:4179
Comment:
**Cause serialization outside try-catch**
In `emitErrorChunk`, the cause serialization is inside the try-catch block (lines 4263-4269), so if it fails, the error chunk is still emitted gracefully. Here in `serializeErrorValue`, the cause serialization is outside the try-catch. If `outlineModel(request, cause)` were to throw (e.g., due to an exotic cause value with throwing getters), this would propagate unhandled. Consider moving this block inside the existing try-catch for consistency with `emitErrorChunk`, or wrapping it in its own try-catch.
How can I resolve this? If you propose a fix, please make it concise.| (error: any).environmentName = env; | ||
| const causeInfo = errorInfo.cause; | ||
| if (typeof causeInfo === 'string') { | ||
| // TODO: What bad things can I do by controling the cause reference? |
There was a problem hiding this comment.
Typo in TODO comment
Minor: "controling" should be "controlling".
| // TODO: What bad things can I do by controling the cause reference? | |
| // TODO: What bad things can I do by controlling the cause reference? |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-client/src/ReactFlightClient.js
Line: 3564:3564
Comment:
**Typo in TODO comment**
Minor: "controling" should be "controlling".
```suggestion
// TODO: What bad things can I do by controlling the cause reference?
```
How can I resolve this? If you propose a fix, please make it concise.| if ('cause' in errorInfo) { | ||
| (error: any).cause = errorInfo.cause; | ||
| } |
There was a problem hiding this comment.
Cause reference not resolved on E-row path
When resolveErrorDev is called from the $Z inline path (via getOutlinedModel at line 2481), errorInfo.cause is already resolved through the Flight JSON reviver. However, when called from resolveErrorModel (line 3579), errorInfo comes from JSON.parse(row) without a reviver, so errorInfo.cause will be a raw Flight reference string like "$3" instead of the actual cause value.
This means for thrown errors (E-row path, e.g. errors caught by error boundaries), error.cause will be set to the unresolved string "$3" rather than the actual cause object. The owner field handles this correctly by manually resolving the reference via getOutlinedModel (line 3540-3543), but cause doesn't get the same treatment.
The test only covers the $Z path (error passed as a prop) and doesn't exercise the E-row path, so this issue isn't caught by the new test.
The cause should be resolved similarly to owner:
| if ('cause' in errorInfo) { | |
| (error: any).cause = errorInfo.cause; | |
| } | |
| if ('cause' in errorInfo) { | |
| const causeRef = errorInfo.cause; | |
| if (typeof causeRef === 'string' && causeRef[0] === '$') { | |
| (error: any).cause = getOutlinedModel(response, causeRef.slice(1), error, 'cause', createModel); | |
| } else { | |
| (error: any).cause = causeRef; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-client/src/ReactFlightClient.js
Line: 3562-3564
Comment:
**Cause reference not resolved on E-row path**
When `resolveErrorDev` is called from the `$Z` inline path (via `getOutlinedModel` at line 2481), `errorInfo.cause` is already resolved through the Flight JSON reviver. However, when called from `resolveErrorModel` (line 3579), `errorInfo` comes from `JSON.parse(row)` without a reviver, so `errorInfo.cause` will be a raw Flight reference string like `"$3"` instead of the actual cause value.
This means for thrown errors (E-row path, e.g. errors caught by error boundaries), `error.cause` will be set to the unresolved string `"$3"` rather than the actual cause object. The `owner` field handles this correctly by manually resolving the reference via `getOutlinedModel` (line 3540-3543), but `cause` doesn't get the same treatment.
The test only covers the `$Z` path (error passed as a prop) and doesn't exercise the E-row path, so this issue isn't caught by the new test.
The cause should be resolved similarly to `owner`:
```suggestion
if ('cause' in errorInfo) {
const causeRef = errorInfo.cause;
if (typeof causeRef === 'string' && causeRef[0] === '$') {
(error: any).cause = getOutlinedModel(response, causeRef.slice(1), error, 'cause', createModel);
} else {
(error: any).cause = causeRef;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.768bf09 to
ebccced
Compare
ebccced to
acb81d6
Compare
…yload (#35776) ## Summary Follow-up to vercel/next.js#89823 with the actual changes to React. Replaces the `JSON.parse` reviver callback in `initializeModelChunk` with a two-step approach: plain `JSON.parse()` followed by a recursive `reviveModel()` post-process (same as in Flight Reply Server). This yields a **~75% speedup** in RSC chunk deserialization. | Payload | Original (ms) | Walk (ms) | Speedup | |---------|---------------|-----------|---------| | Small (2 elements, 142B) | 0.0024 | 0.0007 | **+72%** | | Medium (~12 elements, 914B) | 0.0116 | 0.0031 | **+73%** | | Large (~90 elements, 16.7KB) | 0.1836 | 0.0451 | **+75%** | | XL (~200 elements, 25.7KB) | 0.3742 | 0.0913 | **+76%** | | Table (1000 rows, 110KB) | 3.0862 | 0.6887 | **+78%** | ## Problem `createFromJSONCallback` returns a reviver function passed as the second argument to `JSON.parse()`. This reviver is called for **every key-value pair** in the parsed JSON. While the logic inside the reviver is lightweight, the dominant cost is the **C++ → JavaScript boundary crossing** — V8's `JSON.parse` is implemented in C++, and calling back into JavaScript for every node incurs significant overhead. Even a trivial no-op reviver `(k, v) => v` makes `JSON.parse` **~4x slower** than bare `JSON.parse` without a reviver: ``` 108 KB payload: Bare JSON.parse: 0.60 ms Trivial reviver: 2.95 ms (+391%) ``` ## Change Replace the reviver with a two-step process: 1. `JSON.parse(resolvedModel)` — parse the entire payload in C++ with no callbacks 2. `reviveModel` — recursively walk the resulting object in pure JavaScript to apply RSC transformations The `reviveModel` function includes additional optimizations over the original reviver: - **Short-circuits plain strings**: only calls `parseModelString` when the string starts with `$`, skipping the vast majority of strings (class names, text content, etc.) - **Stays entirely in JavaScript** — no C++ boundary crossings during the walk ## Results You can find the related applications in the [Next.js PR ](vercel/next.js#89823 I've been testing this on Next.js applications. ### Table as Server Component with 1000 items Before: ``` "min": 13.782875000000786, "max": 22.23400000000038, "avg": 17.116868530000083, "p50": 17.10766700000022, "p75": 18.50787499999933, "p95": 20.426249999998618, "p99": 21.814125000000786 ``` After: ``` "min": 10.963916999999128, "max": 18.096083000000363, "avg": 13.543286884999988, "p50": 13.58350000000064, "p75": 14.871791999999914, "p95": 16.08429099999921, "p99": 17.591458000000785 ``` ### Table as Client Component with 1000 items Before: ``` "min": 3.888875000000553, "max": 9.044959000000745, "avg": 4.651271475000067, "p50": 4.555749999999534, "p75": 4.966624999999112, "p95": 5.47754200000054, "p99": 6.109499999998661 ```` After: ``` "min": 3.5986250000005384, "max": 5.374291000000085, "avg": 4.142990245000046, "p50": 4.10570799999914, "p75": 4.392041999999492, "p95": 4.740084000000934, "p99": 5.1652500000000146 ``` ### Nested Suspense Before: ``` Requests: 200 Min: 73ms Max: 106ms Avg: 78ms P50: 77ms P75: 80ms P95: 85ms P99: 94ms ``` After: ``` Requests: 200 Min: 56ms Max: 67ms Avg: 59ms P50: 58ms P75: 60ms P95: 65ms P99: 66ms ``` ### Even more nested Suspense (double-level Suspense) Before: ``` Requests: 200 Min: 159ms Max: 208ms Avg: 169ms P50: 167ms P75: 173ms P95: 183ms P99: 188ms ``` After: ``` Requests: 200 Min: 125ms Max: 170ms Avg: 134ms P50: 132ms P75: 138ms P95: 148ms P99: 160ms ``` ## How did you test this change? Ran it across many Next.js benchmark applications. The entire Next.js test suite passes with this change. --------- Co-authored-by: Hendrik Liebau <mail@hendrik-liebau.de>
…sh (#35824) When flushing the shell, stylesheets with precedence are emitted in the `<head>` which blocks paint regardless. Outlining a boundary solely because it has suspensey CSS provides no benefit during the shell flush and causes a higher-level fallback to be shown unnecessarily (e.g. "Middle Fallback" instead of "Inner Fallback"). This change passes a flushingInShell flag to hasSuspenseyContent so the host config can skip stylesheet-only suspensey content when flushing the shell. Suspensey images (used for ViewTransition animation reveals) still trigger outlining during the shell since their motivation is different. When flushing streamed completions the behavior is unchanged — suspensey CSS still causes outlining so the parent content can display sooner while the stylesheet loads.
This reverts commit cec2e82.
This reverts commit c4daaf4. We need to outline to simplify the default case where cause is an error
|
Upstream PR was closed or merged. Code is synced via branch mirror. |
Mirror of facebook/react#35810
Original author: eps1lon
Summary
Adds support for transporting
Error.cause(e.g.new Error(message, { cause }). React supports arbitrary values in.causesince.causedoesn't need to be anErrorinstance (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause#description)How did you test this change?