From a34aa05e6991b6ec82986b56b786bcd497ed8aa1 Mon Sep 17 00:00:00 2001 From: lauren Date: Wed, 18 Dec 2024 20:09:09 -0500 Subject: [PATCH 1/5] [ci] Allow build artifacts to be downloaded from any branch (#31846) This was previously scoped to just commits on `main` but this restriction is unnecessary. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31846). * #31848 * #31847 * __->__ #31846 --- .../download-build-artifacts.js | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/scripts/release/shared-commands/download-build-artifacts.js b/scripts/release/shared-commands/download-build-artifacts.js index 65e2baa9a948b..18c2b80543256 100644 --- a/scripts/release/shared-commands/download-build-artifacts.js +++ b/scripts/release/shared-commands/download-build-artifacts.js @@ -39,18 +39,16 @@ function getWorkflowId() { async function getWorkflowRun(commit) { const res = await exec( - `curl -L ${GITHUB_HEADERS} https://api.github.com/repos/${OWNER}/${REPO}/actions/workflows/${getWorkflowId()}/runs?head_sha=${commit}&branch=main&exclude_pull_requests=true` + `curl -L ${GITHUB_HEADERS} https://api.github.com/repos/${OWNER}/${REPO}/actions/workflows/${getWorkflowId()}/runs?head_sha=${commit}` ); const json = JSON.parse(res.stdout); - let workflowRun; - if (json.total_count === 1) { - workflowRun = json.workflow_runs[0]; - } else { - workflowRun = json.workflow_runs.find( - run => run.head_sha === commit && run.head_branch === 'main' - ); - } + const workflowRun = json.workflow_runs.find( + run => + run.head_sha === commit && + run.status === 'completed' && + run.conclusion === 'success' + ); if (workflowRun == null || workflowRun.id == null) { console.log( @@ -68,14 +66,9 @@ async function getArtifact(workflowRunId, artifactName) { ); const json = JSON.parse(res.stdout); - let artifact; - if (json.total_count === 1) { - artifact = json.artifacts[0]; - } else { - artifact = json.artifacts.find( - _artifact => _artifact.name === artifactName - ); - } + const artifact = json.artifacts.find( + _artifact => _artifact.name === artifactName + ); if (artifact == null) { console.log( From 74e39ce2a1eec803936db8a29349f6fda176cce7 Mon Sep 17 00:00:00 2001 From: lauren Date: Wed, 18 Dec 2024 20:09:50 -0500 Subject: [PATCH 2/5] [ci] Validate downloaded build artifact (#31847) Adds validation to download-build-artifacts to confirm that the downloaded artifact matches what was requested. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31847). * #31848 * __->__ #31847 * #31846 --- .../shared-commands/download-build-artifacts.js | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/scripts/release/shared-commands/download-build-artifacts.js b/scripts/release/shared-commands/download-build-artifacts.js index 18c2b80543256..85cbb8b9fa176 100644 --- a/scripts/release/shared-commands/download-build-artifacts.js +++ b/scripts/release/shared-commands/download-build-artifacts.js @@ -3,7 +3,7 @@ const {join} = require('path'); const theme = require('../theme'); const {exec} = require('child-process-promise'); -const {existsSync} = require('fs'); +const {existsSync, readFileSync} = require('fs'); const {logPromise} = require('../utils'); if (process.env.GH_TOKEN == null) { @@ -80,7 +80,7 @@ async function getArtifact(workflowRunId, artifactName) { return artifact; } -async function processArtifact(artifact, releaseChannel) { +async function processArtifact(artifact, commit, releaseChannel) { // Download and extract artifact const cwd = join(__dirname, '..', '..', '..'); await exec(`rm -rf ./build`, {cwd}); @@ -117,6 +117,17 @@ async function processArtifact(artifact, releaseChannel) { await exec(`cp -r ./build/${sourceDir} ./build/node_modules`, { cwd, }); + + // Validate artifact + const buildSha = readFileSync('./build/COMMIT_SHA', 'utf8').replace( + /[\u0000-\u001F\u007F-\u009F]/g, + '' + ); + if (buildSha !== commit) { + throw new Error( + `Requested commit sha does not match downloaded artifact. Expected: ${commit}, got: ${buildSha}` + ); + } } async function downloadArtifactsFromGitHub(commit, releaseChannel) { @@ -141,7 +152,7 @@ async function downloadArtifactsFromGitHub(commit, releaseChannel) { workflowRun.id, 'artifacts_combined' ); - await processArtifact(artifact, releaseChannel); + await processArtifact(artifact, commit, releaseChannel); return; } else { console.log( From 7de040ccfa49f4128f0f5c6a7a81c019b4a381b8 Mon Sep 17 00:00:00 2001 From: lauren Date: Wed, 18 Dec 2024 20:10:03 -0500 Subject: [PATCH 3/5] [ci] Don't cancel runs if more than one branch triggers CI (#31848) This might primarily only affect those using Sapling for React development, but pushing the same commit to multiple branches shouldn't cancel the run --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31848). * __->__ #31848 * #31847 * #31846 --- .github/workflows/compiler_playground.yml | 2 +- .github/workflows/compiler_rust.yml | 2 +- .github/workflows/compiler_typescript.yml | 2 +- .github/workflows/runtime_build_and_test.yml | 2 +- .github/workflows/shared_lint.yml | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/compiler_playground.yml b/.github/workflows/compiler_playground.yml index 81c6c3ee42a27..68d14a7661315 100644 --- a/.github/workflows/compiler_playground.yml +++ b/.github/workflows/compiler_playground.yml @@ -9,7 +9,7 @@ on: - .github/workflows/compiler_playground.yml concurrency: - group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + group: ${{ github.workflow }}-${{ github.ref_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true env: diff --git a/.github/workflows/compiler_rust.yml b/.github/workflows/compiler_rust.yml index 50d42a37763e3..3937f92355d02 100644 --- a/.github/workflows/compiler_rust.yml +++ b/.github/workflows/compiler_rust.yml @@ -16,7 +16,7 @@ on: - compiler/*.toml concurrency: - group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + group: ${{ github.workflow }}-${{ github.ref_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true env: diff --git a/.github/workflows/compiler_typescript.yml b/.github/workflows/compiler_typescript.yml index 7ec6297038479..95f8f1f26e112 100644 --- a/.github/workflows/compiler_typescript.yml +++ b/.github/workflows/compiler_typescript.yml @@ -9,7 +9,7 @@ on: - .github/workflows/compiler_typescript.yml concurrency: - group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + group: ${{ github.workflow }}-${{ github.ref_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true env: diff --git a/.github/workflows/runtime_build_and_test.yml b/.github/workflows/runtime_build_and_test.yml index b9cbe4d447bbb..a14b5b376ce5e 100644 --- a/.github/workflows/runtime_build_and_test.yml +++ b/.github/workflows/runtime_build_and_test.yml @@ -8,7 +8,7 @@ on: - compiler/** concurrency: - group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + group: ${{ github.workflow }}-${{ github.ref_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true env: diff --git a/.github/workflows/shared_lint.yml b/.github/workflows/shared_lint.yml index 36c1df9eb86f9..4b077eff65ab5 100644 --- a/.github/workflows/shared_lint.yml +++ b/.github/workflows/shared_lint.yml @@ -6,7 +6,7 @@ on: pull_request: concurrency: - group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + group: ${{ github.workflow }}-${{ github.ref_name }}-${{ github.event.pull_request.number || github.run_id }} cancel-in-progress: true env: From 17520b638190a20e745fe53299813b29b52dfc4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 18 Dec 2024 23:53:54 -0500 Subject: [PATCH 4/5] [Fiber] Mark hydrated components in tertiary color (green) (#31829) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a follow up to #31752. This keeps track in the commit phase whether this subtree was hydrated. If it was, then we mark those components in the Components track as green. Just like the phase itself is marked as green. If the boundary client rendered we instead mark it as "errored" and its children given the plain primary render color (blue). I also collect the hydration error for this case so we can include its message in the details view. (Unfortunately this doesn't support newlines atm.) Most of the time this happens in separate commits for each boundary but it is possible to force a client render in the same pass as a hydration. Such as if an update flows into a boundary that has been put into fallback state after it was initially attempted. Screenshot 2024-12-18 at 12 06 54 AM --- .../src/ReactFiberBeginWork.js | 1 + .../src/ReactFiberCommitWork.js | 81 ++++++++++++++++ .../src/ReactFiberCompleteWork.js | 10 +- .../src/ReactFiberHydrationContext.js | 11 ++- .../src/ReactFiberPerformanceTrack.js | 97 ++++++++++++++++++- .../src/ReactFiberSuspenseComponent.js | 3 + .../src/ReactFiberWorkLoop.js | 15 +++ 7 files changed, 208 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 71affb7ca7a44..0ad2635ed48b1 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1933,6 +1933,7 @@ const SUSPENDED_MARKER: SuspenseState = { dehydrated: null, treeContext: null, retryLane: NoLane, + hydrationErrors: null, }; function mountSuspenseOffscreenState(renderLanes: Lanes): OffscreenState { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 83922deed4b26..43b726e981007 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -97,6 +97,7 @@ import { FormReset, Cloned, PerformedWork, + ForceClientRender, } from './ReactFiberFlags'; import { commitStartTime, @@ -113,6 +114,7 @@ import { import { logComponentRender, logComponentEffect, + logSuspenseBoundaryClientRendered, } from './ReactFiberPerformanceTrack'; import {ConcurrentMode, NoMode, ProfileMode} from './ReactTypeOfMode'; import {deferHiddenCallbacks} from './ReactFiberClassUpdateQueue'; @@ -2689,6 +2691,8 @@ function recursivelyTraversePassiveMountEffects( } } +let inHydratedSubtree = false; + function commitPassiveMountOnFiber( finishedRoot: FiberRoot, finishedWork: Fiber, @@ -2713,6 +2717,7 @@ function commitPassiveMountOnFiber( finishedWork, ((finishedWork.actualStartTime: any): number), endTime, + inHydratedSubtree, ); } @@ -2741,6 +2746,17 @@ function commitPassiveMountOnFiber( } case HostRoot: { const prevEffectDuration = pushNestedEffectDurations(); + + const wasInHydratedSubtree = inHydratedSubtree; + if (enableProfilerTimer && enableComponentPerformanceTrack) { + // Detect if this was a hydration commit by look at if the previous state was + // dehydrated and this wasn't a forced client render. + inHydratedSubtree = + finishedWork.alternate !== null && + (finishedWork.alternate.memoizedState: RootState).isDehydrated && + (finishedWork.flags & ForceClientRender) === NoFlags; + } + recursivelyTraversePassiveMountEffects( finishedRoot, finishedWork, @@ -2748,6 +2764,11 @@ function commitPassiveMountOnFiber( committedTransitions, endTime, ); + + if (enableProfilerTimer && enableComponentPerformanceTrack) { + inHydratedSubtree = wasInHydratedSubtree; + } + if (flags & Passive) { let previousCache: Cache | null = null; if (finishedWork.alternate !== null) { @@ -2841,6 +2862,64 @@ function commitPassiveMountOnFiber( } break; } + case SuspenseComponent: { + const wasInHydratedSubtree = inHydratedSubtree; + if (enableProfilerTimer && enableComponentPerformanceTrack) { + const prevState: SuspenseState | null = + finishedWork.alternate !== null + ? finishedWork.alternate.memoizedState + : null; + const nextState: SuspenseState | null = finishedWork.memoizedState; + if ( + prevState !== null && + prevState.dehydrated !== null && + (nextState === null || nextState.dehydrated === null) + ) { + // This was dehydrated but is no longer dehydrated. We may have now either hydrated it + // or client rendered it. + const deletions = finishedWork.deletions; + if ( + deletions !== null && + deletions.length > 0 && + deletions[0].tag === DehydratedFragment + ) { + // This was an abandoned hydration that deleted the dehydrated fragment. That means we + // are not hydrating this Suspense boundary. + inHydratedSubtree = false; + const hydrationErrors = prevState.hydrationErrors; + // If there were no hydration errors, that suggests that this was an intentional client + // rendered boundary. Such as postpone. + if (hydrationErrors !== null) { + const startTime: number = (finishedWork.actualStartTime: any); + logSuspenseBoundaryClientRendered( + finishedWork, + startTime, + endTime, + hydrationErrors, + ); + } + } else { + // If any children committed they were hydrated. + inHydratedSubtree = true; + } + } else { + inHydratedSubtree = false; + } + } + + recursivelyTraversePassiveMountEffects( + finishedRoot, + finishedWork, + committedLanes, + committedTransitions, + endTime, + ); + + if (enableProfilerTimer && enableComponentPerformanceTrack) { + inHydratedSubtree = wasInHydratedSubtree; + } + break; + } case LegacyHiddenComponent: { if (enableLegacyHidden) { recursivelyTraversePassiveMountEffects( @@ -3074,6 +3153,7 @@ export function reconnectPassiveEffects( finishedWork, ((finishedWork.actualStartTime: any): number), endTime, + inHydratedSubtree, ); } @@ -3317,6 +3397,7 @@ function commitAtomicPassiveEffects( finishedWork, ((finishedWork.actualStartTime: any): number), endTime, + inHydratedSubtree, ); } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index b302b498fa14d..f53b313980295 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -926,9 +926,13 @@ function completeDehydratedSuspenseBoundary( // Successfully completed this tree. If this was a forced client render, // there may have been recoverable errors during first hydration // attempt. If so, add them to a queue so we can log them in the - // commit phase. - upgradeHydrationErrorsToRecoverable(); - + // commit phase. We also add them to prev state so we can get to them + // from the Suspense Boundary. + const hydrationErrors = upgradeHydrationErrorsToRecoverable(); + if (current !== null && current.memoizedState !== null) { + const prevState: SuspenseState = current.memoizedState; + prevState.hydrationErrors = hydrationErrors; + } // Fall through to normal Suspense path return true; } diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.js b/packages/react-reconciler/src/ReactFiberHydrationContext.js index 1d2f69852a9db..b4d948e735276 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.js @@ -280,6 +280,7 @@ function tryHydrateSuspense(fiber: Fiber, nextInstance: any) { dehydrated: suspenseInstance, treeContext: getSuspendedTreeContext(), retryLane: OffscreenLane, + hydrationErrors: null, }; fiber.memoizedState = suspenseState; // Store the dehydrated fragment as a child fiber. @@ -701,14 +702,18 @@ function resetHydrationState(): void { didSuspendOrErrorDEV = false; } -export function upgradeHydrationErrorsToRecoverable(): void { - if (hydrationErrors !== null) { +export function upgradeHydrationErrorsToRecoverable(): Array< + CapturedValue, +> | null { + const queuedErrors = hydrationErrors; + if (queuedErrors !== null) { // Successfully completed a forced client render. The errors that occurred // during the hydration attempt are now recovered. We will log them in // commit phase, once the entire tree has finished. - queueRecoverableErrors(hydrationErrors); + queueRecoverableErrors(queuedErrors); hydrationErrors = null; } + return queuedErrors; } function getIsHydrating(): boolean { diff --git a/packages/react-reconciler/src/ReactFiberPerformanceTrack.js b/packages/react-reconciler/src/ReactFiberPerformanceTrack.js index d94fd5ee480b3..61bfd5cf7f844 100644 --- a/packages/react-reconciler/src/ReactFiberPerformanceTrack.js +++ b/packages/react-reconciler/src/ReactFiberPerformanceTrack.js @@ -11,6 +11,8 @@ import type {Fiber} from './ReactInternalTypes'; import type {Lanes} from './ReactFiberLane'; +import type {CapturedValue} from './ReactCapturedValue'; + import getComponentNameFromFiber from './getComponentNameFromFiber'; import { @@ -123,6 +125,7 @@ export function logComponentRender( fiber: Fiber, startTime: number, endTime: number, + wasHydrated: boolean, ): void { const name = getComponentNameFromFiber(fiber); if (name === null) { @@ -138,11 +141,17 @@ export function logComponentRender( } reusableComponentDevToolDetails.color = selfTime < 0.5 - ? 'primary-light' + ? wasHydrated + ? 'tertiary-light' + : 'primary-light' : selfTime < 10 - ? 'primary' + ? wasHydrated + ? 'tertiary' + : 'primary' : selfTime < 100 - ? 'primary-dark' + ? wasHydrated + ? 'tertiary-dark' + : 'primary-dark' : 'error'; reusableComponentOptions.start = startTime; reusableComponentOptions.end = endTime; @@ -150,6 +159,44 @@ export function logComponentRender( } } +export function logSuspenseBoundaryClientRendered( + fiber: Fiber, + startTime: number, + endTime: number, + errors: Array>, +): void { + if (supportsUserTiming) { + const properties = []; + if (__DEV__) { + for (let i = 0; i < errors.length; i++) { + const capturedValue = errors[i]; + const error = capturedValue.value; + const message = + typeof error === 'object' && + error !== null && + typeof error.message === 'string' + ? // eslint-disable-next-line react-internal/safe-string-coercion + String(error.message) + : // eslint-disable-next-line react-internal/safe-string-coercion + String(error); + properties.push(['Error', message]); + } + } + performance.measure('Suspense', { + start: startTime, + end: endTime, + detail: { + devtools: { + color: 'error', + track: COMPONENTS_TRACK, + tooltipText: 'Hydration failed', + properties, + }, + }, + }); + } +} + export function logComponentEffect( fiber: Fiber, startTime: number, @@ -387,6 +434,48 @@ export function logSuspendedWithDelayPhase( } } +export function logRecoveredRenderPhase( + startTime: number, + endTime: number, + lanes: Lanes, + recoverableErrors: Array>, + hydrationFailed: boolean, +): void { + if (supportsUserTiming) { + const properties = []; + if (__DEV__) { + for (let i = 0; i < recoverableErrors.length; i++) { + const capturedValue = recoverableErrors[i]; + const error = capturedValue.value; + const message = + typeof error === 'object' && + error !== null && + typeof error.message === 'string' + ? // eslint-disable-next-line react-internal/safe-string-coercion + String(error.message) + : // eslint-disable-next-line react-internal/safe-string-coercion + String(error); + properties.push(['Recoverable Error', message]); + } + } + performance.measure('Recovered', { + start: startTime, + end: endTime, + detail: { + devtools: { + color: 'primary-dark', + track: reusableLaneDevToolDetails.track, + trackGroup: LANES_TRACK_GROUP, + tooltipText: hydrationFailed + ? 'Hydration Failed' + : 'Recovered after Error', + properties, + }, + }, + }); + } +} + export function logErroredRenderPhase( startTime: number, endTime: number, @@ -396,7 +485,7 @@ export function logErroredRenderPhase( reusableLaneDevToolDetails.color = 'error'; reusableLaneOptions.start = startTime; reusableLaneOptions.end = endTime; - performance.measure('Errored Render', reusableLaneOptions); + performance.measure('Errored', reusableLaneOptions); } } diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index 1f558a44903e2..16a376fe6c7ae 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -12,6 +12,7 @@ import type {Fiber} from './ReactInternalTypes'; import type {SuspenseInstance} from './ReactFiberConfig'; import type {Lane} from './ReactFiberLane'; import type {TreeContext} from './ReactFiberTreeContext'; +import type {CapturedValue} from './ReactCapturedValue'; import {SuspenseComponent, SuspenseListComponent} from './ReactWorkTags'; import {NoFlags, DidCapture} from './ReactFiberFlags'; @@ -49,6 +50,8 @@ export type SuspenseState = { // OffscreenLane is the default for dehydrated boundaries. // NoLane is the default for normal boundaries, which turns into "normal" pri. retryLane: Lane, + // Stashed Errors that happened while attempting to hydrate this boundary. + hydrationErrors: Array> | null, }; export type SuspenseListTailMode = 'collapsed' | 'hidden' | void; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 7377f605a8811..2e8b443d22caa 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -23,6 +23,7 @@ import type { } from './ReactFiberTracingMarkerComponent'; import type {OffscreenInstance} from './ReactFiberActivityComponent'; import type {Resource} from './ReactFiberConfig'; +import type {RootState} from './ReactFiberRoot'; import { enableCreateEventHandleAPI, @@ -59,6 +60,7 @@ import { logRenderPhase, logInterruptedRenderPhase, logSuspendedRenderPhase, + logRecoveredRenderPhase, logErroredRenderPhase, logInconsistentRender, logSuspendedWithDelayPhase, @@ -3183,6 +3185,19 @@ function commitRootImpl( completedRenderEndTime, lanes, ); + } else if (recoverableErrors !== null) { + const hydrationFailed = + finishedWork !== null && + finishedWork.alternate !== null && + (finishedWork.alternate.memoizedState: RootState).isDehydrated && + (finishedWork.flags & ForceClientRender) !== NoFlags; + logRecoveredRenderPhase( + completedRenderStartTime, + completedRenderEndTime, + lanes, + recoverableErrors, + hydrationFailed, + ); } else { logRenderPhase(completedRenderStartTime, completedRenderEndTime, lanes); } From a9bbe34622885ef5667d33236d580fe7321c0d8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 19 Dec 2024 00:03:40 -0500 Subject: [PATCH 5/5] [Flight] Reject any new Chunks not yet discovered at the time of reportGlobalError (#31851) Same as #31840 but for the Flight Client. --- .../react-client/src/ReactFlightClient.js | 16 ++++- .../src/ReactNoopFlightClient.js | 3 +- .../src/__tests__/ReactFlightDOM-test.js | 24 +------- .../__tests__/ReactFlightDOMBrowser-test.js | 14 +---- .../src/__tests__/ReactFlightDOMEdge-test.js | 59 +++++++++++++++---- .../src/__tests__/ReactFlightDOMNode-test.js | 11 +--- 6 files changed, 70 insertions(+), 57 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 8e00df7c1c2e9..30d340ac56700 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -307,6 +307,8 @@ export type Response = { _rowTag: number, // 0 indicates that we're currently parsing the row ID _rowLength: number, // remaining bytes in the row. 0 indicates that we're looking for a newline. _buffer: Array, // chunks received so far as part of this row + _closed: boolean, + _closedReason: mixed, _tempRefs: void | TemporaryReferenceSet, // the set temporary references can be resolved from _timeOrigin: number, // Profiling-only _debugRootOwner?: null | ReactComponentInfo, // DEV-only @@ -358,7 +360,7 @@ function createBlockedChunk(response: Response): BlockedChunk { function createErrorChunk( response: Response, - error: Error | Postpone, + error: mixed, ): ErroredChunk { // $FlowFixMe[invalid-constructor] Flow doesn't support functions as constructors return new ReactPromise(ERRORED, null, error, response); @@ -639,6 +641,8 @@ function initializeModuleChunk(chunk: ResolvedModuleChunk): void { // Report that any missing chunks in the model is now going to throw this // error upon read. Also notify any pending promises. export function reportGlobalError(response: Response, error: Error): void { + response._closed = true; + response._closedReason = error; response._chunks.forEach(chunk => { // If this chunk was already resolved or errored, it won't // trigger an error but if it wasn't then we need to @@ -913,7 +917,13 @@ function getChunk(response: Response, id: number): SomeChunk { const chunks = response._chunks; let chunk = chunks.get(id); if (!chunk) { - chunk = createPendingChunk(response); + if (response._closed) { + // We have already errored the response and we're not going to get + // anything more streaming in so this will immediately error. + chunk = createErrorChunk(response, response._closedReason); + } else { + chunk = createPendingChunk(response); + } chunks.set(id, chunk); } return chunk; @@ -1640,6 +1650,8 @@ function ResponseInstance( this._rowTag = 0; this._rowLength = 0; this._buffer = []; + this._closed = false; + this._closedReason = null; this._tempRefs = temporaryReferences; if (enableProfilerTimer && enableComponentPerformanceTrack) { this._timeOrigin = 0; diff --git a/packages/react-noop-renderer/src/ReactNoopFlightClient.js b/packages/react-noop-renderer/src/ReactNoopFlightClient.js index 346433404e235..3f71a13872f2b 100644 --- a/packages/react-noop-renderer/src/ReactNoopFlightClient.js +++ b/packages/react-noop-renderer/src/ReactNoopFlightClient.js @@ -24,7 +24,7 @@ type Source = Array; const decoderOptions = {stream: true}; -const {createResponse, processBinaryChunk, getRoot, close} = ReactFlightClient({ +const {createResponse, processBinaryChunk, getRoot} = ReactFlightClient({ createStringDecoder() { return new TextDecoder(); }, @@ -74,7 +74,6 @@ function read(source: Source, options: ReadOptions): Thenable { for (let i = 0; i < source.length; i++) { processBinaryChunk(response, source[i], 0); } - close(response); return getRoot(response); } diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js index 1cd688632c41e..0b16b3b32114d 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -2902,16 +2902,7 @@ describe('ReactFlightDOM', () => { abortFizz('bam'); }); - if (__DEV__) { - expect(errors).toEqual([new Error('Connection closed.')]); - } else { - // This is likely a bug. In Dev we get a connection closed error - // because the debug info creates a chunk that has a pending status - // and when the stream finishes we error if any chunks are still pending. - // In production there is no debug info so the missing chunk is never instantiated - // because nothing triggers model evaluation before the stream completes - expect(errors).toEqual(['bam']); - } + expect(errors).toEqual([new Error('Connection closed.')]); const container = document.createElement('div'); await readInto(container, fizzReadable); @@ -3066,17 +3057,8 @@ describe('ReactFlightDOM', () => { }); // one error per boundary - if (__DEV__) { - const err = new Error('Connection closed.'); - expect(errors).toEqual([err, err, err]); - } else { - // This is likely a bug. In Dev we get a connection closed error - // because the debug info creates a chunk that has a pending status - // and when the stream finishes we error if any chunks are still pending. - // In production there is no debug info so the missing chunk is never instantiated - // because nothing triggers model evaluation before the stream completes - expect(errors).toEqual(['boom', 'boom', 'boom']); - } + const err = new Error('Connection closed.'); + expect(errors).toEqual([err, err, err]); const container = document.createElement('div'); await readInto(container, fizzReadable); diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js index 6ac9609f5f108..3eccca1a9ba70 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -2574,17 +2574,7 @@ describe('ReactFlightDOMBrowser', () => { root.render(); }); - if (__DEV__) { - expect(errors).toEqual([new Error('Connection closed.')]); - expect(container.innerHTML).toBe(''); - } else { - // This is likely a bug. In Dev we get a connection closed error - // because the debug info creates a chunk that has a pending status - // and when the stream finishes we error if any chunks are still pending. - // In production there is no debug info so the missing chunk is never instantiated - // because nothing triggers model evaluation before the stream completes - expect(errors).toEqual([]); - expect(container.innerHTML).toBe('
loading...
'); - } + expect(errors).toEqual([new Error('Connection closed.')]); + expect(container.innerHTML).toBe(''); }); }); diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index f88e51a878ac9..f2814f250a2df 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -1245,20 +1245,59 @@ describe('ReactFlightDOMEdge', () => { ), ); fizzController.abort('bam'); - if (__DEV__) { - expect(errors).toEqual([new Error('Connection closed.')]); - } else { - // This is likely a bug. In Dev we get a connection closed error - // because the debug info creates a chunk that has a pending status - // and when the stream finishes we error if any chunks are still pending. - // In production there is no debug info so the missing chunk is never instantiated - // because nothing triggers model evaluation before the stream completes - expect(errors).toEqual(['bam']); - } + expect(errors).toEqual([new Error('Connection closed.')]); // Should still match the result when parsed const result = await readResult(ssrStream); const div = document.createElement('div'); div.innerHTML = result; expect(div.textContent).toBe('loading...'); }); + + // @gate enableHalt + it('should abort parsing an incomplete prerender payload', async () => { + const infinitePromise = new Promise(() => {}); + const controller = new AbortController(); + const errors = []; + const {pendingResult} = await serverAct(async () => { + // destructure trick to avoid the act scope from awaiting the returned value + return { + pendingResult: ReactServerDOMStaticServer.unstable_prerender( + {promise: infinitePromise}, + webpackMap, + { + signal: controller.signal, + onError(err) { + errors.push(err); + }, + }, + ), + }; + }); + + controller.abort(); + const {prelude} = await pendingResult; + + expect(errors).toEqual([]); + + const response = ReactServerDOMClient.createFromReadableStream(prelude, { + serverConsumerManifest: { + moduleMap: {}, + moduleLoading: {}, + }, + }); + + // Wait for the stream to finish and therefore abort before we try to .then the response. + await 0; + + const result = await response; + + let error = null; + try { + await result.promise; + } catch (x) { + error = x; + } + expect(error).not.toBe(null); + expect(error.message).toBe('Connection closed.'); + }); }); diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMNode-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMNode-test.js index 20f461708ae65..43e36348327ff 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMNode-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMNode-test.js @@ -508,16 +508,7 @@ describe('ReactFlightDOMNode', () => { ), ); ssrStream.abort('bam'); - if (__DEV__) { - expect(errors).toEqual([new Error('Connection closed.')]); - } else { - // This is likely a bug. In Dev we get a connection closed error - // because the debug info creates a chunk that has a pending status - // and when the stream finishes we error if any chunks are still pending. - // In production there is no debug info so the missing chunk is never instantiated - // because nothing triggers model evaluation before the stream completes - expect(errors).toEqual(['bam']); - } + expect(errors).toEqual([new Error('Connection closed.')]); // Should still match the result when parsed const result = await readResult(ssrStream); const div = document.createElement('div');