From 378167a6555dcefa562f2e71a9faca9120c8a66e Mon Sep 17 00:00:00 2001 From: sebmarkbage Date: Fri, 29 Sep 2023 22:29:39 +0000 Subject: [PATCH] [Fizz] Don't double replay elements when it's the postpone point (#27440) The `resumeElement` function wasn't actually doing the correct thing because it was resuming the element itself but what the child slot means is that we're supposed to resume in the direct child of the element. This is difficult to check for since it's all the possible branches that the element can render into, so instead we just check this in renderNode. It means the hottest path always checks the task which is unfortunate. And also, resuming using the correct nextSegmentId. Fixes two bugs surfaced by this test. --------- Co-authored-by: Josh Story DiffTrain build for [a6ed60a8eb0626e5f84d0bdbb62c0b61219150d3](https://github.com/facebook/react/commit/a6ed60a8eb0626e5f84d0bdbb62c0b61219150d3) --- compiled/facebook-www/REVISION | 2 +- compiled/facebook-www/ReactART-dev.modern.js | 2 +- compiled/facebook-www/ReactART-prod.modern.js | 4 +- .../ReactDOMServer-dev.classic.js | 164 ++--- .../facebook-www/ReactDOMServer-dev.modern.js | 164 ++--- .../ReactDOMServer-prod.classic.js | 563 ++++++++---------- .../ReactDOMServer-prod.modern.js | 563 ++++++++---------- .../ReactDOMServerStreaming-dev.modern.js | 162 ++--- .../ReactDOMServerStreaming-prod.modern.js | 541 ++++++++--------- 9 files changed, 850 insertions(+), 1315 deletions(-) diff --git a/compiled/facebook-www/REVISION b/compiled/facebook-www/REVISION index d1ffb5bbf9..e92d24da47 100644 --- a/compiled/facebook-www/REVISION +++ b/compiled/facebook-www/REVISION @@ -1 +1 @@ -c7ba8c098889b6dc47fa9c807bbba3975a658584 +a6ed60a8eb0626e5f84d0bdbb62c0b61219150d3 diff --git a/compiled/facebook-www/ReactART-dev.modern.js b/compiled/facebook-www/ReactART-dev.modern.js index 34a61158e6..4add791090 100644 --- a/compiled/facebook-www/ReactART-dev.modern.js +++ b/compiled/facebook-www/ReactART-dev.modern.js @@ -69,7 +69,7 @@ function _assertThisInitialized(self) { return self; } -var ReactVersion = "18.3.0-www-modern-10178652"; +var ReactVersion = "18.3.0-www-modern-5845cabd"; var LegacyRoot = 0; var ConcurrentRoot = 1; diff --git a/compiled/facebook-www/ReactART-prod.modern.js b/compiled/facebook-www/ReactART-prod.modern.js index 2550ca9349..848359d4fe 100644 --- a/compiled/facebook-www/ReactART-prod.modern.js +++ b/compiled/facebook-www/ReactART-prod.modern.js @@ -9772,7 +9772,7 @@ var slice = Array.prototype.slice, return null; }, bundleType: 0, - version: "18.3.0-www-modern-df7dbab9", + version: "18.3.0-www-modern-d368bffd", rendererPackageName: "react-art" }; var internals$jscomp$inline_1284 = { @@ -9803,7 +9803,7 @@ var internals$jscomp$inline_1284 = { scheduleRoot: null, setRefreshHandler: null, getCurrentFiber: null, - reconcilerVersion: "18.3.0-www-modern-df7dbab9" + reconcilerVersion: "18.3.0-www-modern-d368bffd" }; if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) { var hook$jscomp$inline_1285 = __REACT_DEVTOOLS_GLOBAL_HOOK__; diff --git a/compiled/facebook-www/ReactDOMServer-dev.classic.js b/compiled/facebook-www/ReactDOMServer-dev.classic.js index 31d89145fd..65bf693fe6 100644 --- a/compiled/facebook-www/ReactDOMServer-dev.classic.js +++ b/compiled/facebook-www/ReactDOMServer-dev.classic.js @@ -19,7 +19,7 @@ if (__DEV__) { var React = require("react"); var ReactDOM = require("react-dom"); -var ReactVersion = "18.3.0-www-classic-93baf717"; +var ReactVersion = "18.3.0-www-classic-bdb0331d"; // This refers to a WWW module. var warningWWW = require("warning"); @@ -10566,11 +10566,7 @@ function replaySuspenseBoundary( try { // We use the safe form because we don't handle suspending here. Only error handling. - if (typeof childSlots === "number") { - resumeNode(request, task, childSlots, content, -1); - } else { - renderNode(request, task, content, -1); - } + renderNode(request, task, content, -1); if (task.replay.pendingTasks === 1 && task.replay.nodes.length > 0) { throw new Error( @@ -10625,57 +10621,28 @@ function replaySuspenseBoundary( task.keyPath = prevKeyPath; } - var fallbackKeyPath = [keyPath[0], "Suspense Fallback", keyPath[2]]; - var suspendedFallbackTask; // We create suspended task for the fallback because we don't want to actually work + var fallbackKeyPath = [keyPath[0], "Suspense Fallback", keyPath[2]]; // We create suspended task for the fallback because we don't want to actually work // on it yet in case we finish the main content, so we queue for later. - if (typeof fallbackSlots === "number") { - // Resuming directly in the fallback. - var resumedSegment = createPendingSegment( - request, - 0, - null, - task.formatContext, - false, - false - ); - resumedSegment.id = fallbackSlots; - resumedSegment.parentFlushed = true; - suspendedFallbackTask = createRenderTask( - request, - null, - fallback, - -1, - parentBoundary, - resumedSegment, - fallbackAbortSet, - fallbackKeyPath, - task.formatContext, - task.legacyContext, - task.context, - task.treeContext - ); - } else { - var fallbackReplay = { - nodes: fallbackNodes, - slots: fallbackSlots, - pendingTasks: 0 - }; - suspendedFallbackTask = createReplayTask( - request, - null, - fallbackReplay, - fallback, - -1, - parentBoundary, - fallbackAbortSet, - fallbackKeyPath, - task.formatContext, - task.legacyContext, - task.context, - task.treeContext - ); - } + var fallbackReplay = { + nodes: fallbackNodes, + slots: fallbackSlots, + pendingTasks: 0 + }; + var suspendedFallbackTask = createReplayTask( + request, + null, + fallbackReplay, + fallback, + -1, + parentBoundary, + fallbackAbortSet, + fallbackKeyPath, + task.formatContext, + task.legacyContext, + task.context, + task.treeContext + ); { suspendedFallbackTask.componentStack = task.componentStack; @@ -11433,53 +11400,6 @@ function resumeNode(request, task, segmentId, node, childIndex) { } } -function resumeElement( - request, - task, - keyPath, - segmentId, - prevThenableState, - type, - props, - ref -) { - var prevReplay = task.replay; - var blockedBoundary = task.blockedBoundary; - var resumedSegment = createPendingSegment( - request, - 0, - null, - task.formatContext, - false, - false - ); - resumedSegment.id = segmentId; - resumedSegment.parentFlushed = true; - - try { - // Convert the current ReplayTask to a RenderTask. - var renderTask = task; - renderTask.replay = null; - renderTask.blockedSegment = resumedSegment; - renderElement(request, task, keyPath, prevThenableState, type, props, ref); - resumedSegment.status = COMPLETED; - - if (blockedBoundary === null) { - request.completedRootSegment = resumedSegment; - } else { - queueCompletedSegment(blockedBoundary, resumedSegment); - - if (blockedBoundary.parentFlushed) { - request.partialBoundaries.push(blockedBoundary); - } - } - } finally { - // Restore to a ReplayTask. - task.replay = prevReplay; - task.blockedSegment = null; - } -} - function replayElement( request, task, @@ -11524,29 +11444,15 @@ function replayElement( }; try { - if (typeof childSlots === "number") { - // Matched a resumable element. - resumeElement( - request, - task, - keyPath, - childSlots, - prevThenableState, - type, - props, - ref - ); - } else { - renderElement( - request, - task, - keyPath, - prevThenableState, - type, - props, - ref - ); - } + renderElement( + request, + task, + keyPath, + prevThenableState, + type, + props, + ref + ); if ( task.replay.pendingTasks === 1 && @@ -11682,8 +11588,14 @@ function renderNodeDestructiveImpl( node, childIndex ) { - // Stash the node we're working on. We'll pick up from this task in case + if (task.replay !== null && typeof task.replay.slots === "number") { + // TODO: Figure out a cheaper place than this hot path to do this check. + var resumeSegmentID = task.replay.slots; + resumeNode(request, task, resumeSegmentID, node, childIndex); + return; + } // Stash the node we're working on. We'll pick up from this task in case // something suspends. + task.node = node; task.childIndex = childIndex; // Handle object types diff --git a/compiled/facebook-www/ReactDOMServer-dev.modern.js b/compiled/facebook-www/ReactDOMServer-dev.modern.js index d1133f9f19..9d1b2e46a9 100644 --- a/compiled/facebook-www/ReactDOMServer-dev.modern.js +++ b/compiled/facebook-www/ReactDOMServer-dev.modern.js @@ -19,7 +19,7 @@ if (__DEV__) { var React = require("react"); var ReactDOM = require("react-dom"); -var ReactVersion = "18.3.0-www-modern-10178652"; +var ReactVersion = "18.3.0-www-modern-5845cabd"; // This refers to a WWW module. var warningWWW = require("warning"); @@ -10325,11 +10325,7 @@ function replaySuspenseBoundary( try { // We use the safe form because we don't handle suspending here. Only error handling. - if (typeof childSlots === "number") { - resumeNode(request, task, childSlots, content, -1); - } else { - renderNode(request, task, content, -1); - } + renderNode(request, task, content, -1); if (task.replay.pendingTasks === 1 && task.replay.nodes.length > 0) { throw new Error( @@ -10384,57 +10380,28 @@ function replaySuspenseBoundary( task.keyPath = prevKeyPath; } - var fallbackKeyPath = [keyPath[0], "Suspense Fallback", keyPath[2]]; - var suspendedFallbackTask; // We create suspended task for the fallback because we don't want to actually work + var fallbackKeyPath = [keyPath[0], "Suspense Fallback", keyPath[2]]; // We create suspended task for the fallback because we don't want to actually work // on it yet in case we finish the main content, so we queue for later. - if (typeof fallbackSlots === "number") { - // Resuming directly in the fallback. - var resumedSegment = createPendingSegment( - request, - 0, - null, - task.formatContext, - false, - false - ); - resumedSegment.id = fallbackSlots; - resumedSegment.parentFlushed = true; - suspendedFallbackTask = createRenderTask( - request, - null, - fallback, - -1, - parentBoundary, - resumedSegment, - fallbackAbortSet, - fallbackKeyPath, - task.formatContext, - task.legacyContext, - task.context, - task.treeContext - ); - } else { - var fallbackReplay = { - nodes: fallbackNodes, - slots: fallbackSlots, - pendingTasks: 0 - }; - suspendedFallbackTask = createReplayTask( - request, - null, - fallbackReplay, - fallback, - -1, - parentBoundary, - fallbackAbortSet, - fallbackKeyPath, - task.formatContext, - task.legacyContext, - task.context, - task.treeContext - ); - } + var fallbackReplay = { + nodes: fallbackNodes, + slots: fallbackSlots, + pendingTasks: 0 + }; + var suspendedFallbackTask = createReplayTask( + request, + null, + fallbackReplay, + fallback, + -1, + parentBoundary, + fallbackAbortSet, + fallbackKeyPath, + task.formatContext, + task.legacyContext, + task.context, + task.treeContext + ); { suspendedFallbackTask.componentStack = task.componentStack; @@ -11181,53 +11148,6 @@ function resumeNode(request, task, segmentId, node, childIndex) { } } -function resumeElement( - request, - task, - keyPath, - segmentId, - prevThenableState, - type, - props, - ref -) { - var prevReplay = task.replay; - var blockedBoundary = task.blockedBoundary; - var resumedSegment = createPendingSegment( - request, - 0, - null, - task.formatContext, - false, - false - ); - resumedSegment.id = segmentId; - resumedSegment.parentFlushed = true; - - try { - // Convert the current ReplayTask to a RenderTask. - var renderTask = task; - renderTask.replay = null; - renderTask.blockedSegment = resumedSegment; - renderElement(request, task, keyPath, prevThenableState, type, props, ref); - resumedSegment.status = COMPLETED; - - if (blockedBoundary === null) { - request.completedRootSegment = resumedSegment; - } else { - queueCompletedSegment(blockedBoundary, resumedSegment); - - if (blockedBoundary.parentFlushed) { - request.partialBoundaries.push(blockedBoundary); - } - } - } finally { - // Restore to a ReplayTask. - task.replay = prevReplay; - task.blockedSegment = null; - } -} - function replayElement( request, task, @@ -11272,29 +11192,15 @@ function replayElement( }; try { - if (typeof childSlots === "number") { - // Matched a resumable element. - resumeElement( - request, - task, - keyPath, - childSlots, - prevThenableState, - type, - props, - ref - ); - } else { - renderElement( - request, - task, - keyPath, - prevThenableState, - type, - props, - ref - ); - } + renderElement( + request, + task, + keyPath, + prevThenableState, + type, + props, + ref + ); if ( task.replay.pendingTasks === 1 && @@ -11430,8 +11336,14 @@ function renderNodeDestructiveImpl( node, childIndex ) { - // Stash the node we're working on. We'll pick up from this task in case + if (task.replay !== null && typeof task.replay.slots === "number") { + // TODO: Figure out a cheaper place than this hot path to do this check. + var resumeSegmentID = task.replay.slots; + resumeNode(request, task, resumeSegmentID, node, childIndex); + return; + } // Stash the node we're working on. We'll pick up from this task in case // something suspends. + task.node = node; task.childIndex = childIndex; // Handle object types diff --git a/compiled/facebook-www/ReactDOMServer-prod.classic.js b/compiled/facebook-www/ReactDOMServer-prod.classic.js index 8bb0158049..d752d46b20 100644 --- a/compiled/facebook-www/ReactDOMServer-prod.classic.js +++ b/compiled/facebook-www/ReactDOMServer-prod.classic.js @@ -2289,16 +2289,16 @@ function hoistStylesheetDependency(stylesheet) { function createRenderState(resumableState, generateStaticMarkup) { var idPrefix = resumableState.idPrefix; resumableState = idPrefix + "P:"; - var JSCompiler_object_inline_segmentPrefix_1581 = idPrefix + "S:"; + var JSCompiler_object_inline_segmentPrefix_1566 = idPrefix + "S:"; idPrefix += "B:"; - var JSCompiler_object_inline_preconnects_1593 = new Set(), - JSCompiler_object_inline_fontPreloads_1594 = new Set(), - JSCompiler_object_inline_highImagePreloads_1595 = new Set(), - JSCompiler_object_inline_styles_1596 = new Map(), - JSCompiler_object_inline_bootstrapScripts_1597 = new Set(), - JSCompiler_object_inline_scripts_1598 = new Set(), - JSCompiler_object_inline_bulkPreloads_1599 = new Set(), - JSCompiler_object_inline_preloads_1600 = { + var JSCompiler_object_inline_preconnects_1578 = new Set(), + JSCompiler_object_inline_fontPreloads_1579 = new Set(), + JSCompiler_object_inline_highImagePreloads_1580 = new Set(), + JSCompiler_object_inline_styles_1581 = new Map(), + JSCompiler_object_inline_bootstrapScripts_1582 = new Set(), + JSCompiler_object_inline_scripts_1583 = new Set(), + JSCompiler_object_inline_bulkPreloads_1584 = new Set(), + JSCompiler_object_inline_preloads_1585 = { images: new Map(), stylesheets: new Map(), scripts: new Map(), @@ -2306,7 +2306,7 @@ function createRenderState(resumableState, generateStaticMarkup) { }; return { placeholderPrefix: resumableState, - segmentPrefix: JSCompiler_object_inline_segmentPrefix_1581, + segmentPrefix: JSCompiler_object_inline_segmentPrefix_1566, boundaryPrefix: idPrefix, startInlineScript: "