From c492f97541486458ce21653d2669d53d380f0538 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 28 Jan 2025 22:42:50 -0800 Subject: [PATCH] [Fiber] Support Suspense boundaries anywhere (excluding hydration) (#32163) This is a follow up to https://github.com/facebook/react/pull/32069 In the prior change I updated Fizz to allow you to render Suspense boundaries at any level within a react-dom application by treating the document body as the default render scope. This change updates Fiber to provide similar semantics. Note that this update still does not deliver hydration so unifying the Fizz and Fiber implementations in a single App is not possible yet. The implementation required a rework of the getHostSibling and getHostParent algorithms. Now most HostSingletons are invisible from a host positioning perspective. Head is special in that it is a valid host scope so when you have Placements inside of it, it will act as the parent. But body, and html, will not directly participate in host positioning. Additionally to support flipping to a fallback html, head, and body tag in a Suspense fallback I updated the offscreen hiding/unhide logic to pierce through singletons when lookin for matching hidable nod boundaries anywhere (excluding hydration) --- .../src/client/ReactFiberConfigDOM.js | 135 +++--- .../react-dom/src/__tests__/ReactDOM-test.js | 408 ++++++++++++++++++ .../src/__tests__/ReactDOMFloat-test.js | 37 +- .../src/ReactFiberBeginWork.js | 21 +- .../src/ReactFiberCommitHostEffects.js | 114 +++-- .../src/ReactFiberCommitWork.js | 110 +++-- .../src/ReactFiberConfigWithNoSingletons.js | 2 +- .../src/forks/ReactFiberConfig.custom.js | 2 +- 8 files changed, 654 insertions(+), 175 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 42cf0a8f849d7..9e3574885fa6e 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -799,24 +799,37 @@ export function appendChildToContainer( container: Container, child: Instance | TextInstance, ): void { - let parentNode; - if (container.nodeType === COMMENT_NODE) { - parentNode = (container.parentNode: any); - if (supportsMoveBefore) { - // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. - parentNode.moveBefore(child, container); - } else { - parentNode.insertBefore(child, container); + let parentNode: Document | Element; + switch (container.nodeType) { + case COMMENT_NODE: { + parentNode = (container.parentNode: any); + if (supportsMoveBefore) { + // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. + parentNode.moveBefore(child, container); + } else { + parentNode.insertBefore(child, container); + } + return; } - } else { - parentNode = container; - if (supportsMoveBefore) { - // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. - parentNode.moveBefore(child, null); - } else { - parentNode.appendChild(child); + case DOCUMENT_NODE: { + parentNode = (container: any).body; + break; } + default: { + if (container.nodeName === 'HTML') { + parentNode = (container.ownerDocument.body: any); + } else { + parentNode = (container: any); + } + } + } + if (supportsMoveBefore) { + // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. + parentNode.moveBefore(child, null); + } else { + parentNode.appendChild(child); } + // This container might be used for a portal. // If something inside a portal is clicked, that click should bubble // through the React tree. However, on Mobile Safari the click would @@ -853,21 +866,35 @@ export function insertInContainerBefore( child: Instance | TextInstance, beforeChild: Instance | TextInstance | SuspenseInstance, ): void { - if (container.nodeType === COMMENT_NODE) { - if (supportsMoveBefore) { - // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. - (container.parentNode: any).moveBefore(child, beforeChild); - } else { - (container.parentNode: any).insertBefore(child, beforeChild); + let parentNode: Document | Element; + switch (container.nodeType) { + case COMMENT_NODE: { + parentNode = (container.parentNode: any); + break; } - } else { - if (supportsMoveBefore) { - // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. - container.moveBefore(child, beforeChild); - } else { - container.insertBefore(child, beforeChild); + case DOCUMENT_NODE: { + const ownerDocument: Document = (container: any); + parentNode = (ownerDocument.body: any); + break; + } + default: { + if (container.nodeName === 'HTML') { + parentNode = (container.ownerDocument.body: any); + } else { + parentNode = (container: any); + } } } + if (supportsMoveBefore) { + // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. + parentNode.moveBefore(child, beforeChild); + } else { + parentNode.insertBefore(child, beforeChild); + } +} + +export function isSingletonScope(type: string): boolean { + return type === 'head'; } function createEvent(type: DOMEventName, bubbles: boolean): Event { @@ -913,11 +940,22 @@ export function removeChildFromContainer( container: Container, child: Instance | TextInstance | SuspenseInstance, ): void { - if (container.nodeType === COMMENT_NODE) { - (container.parentNode: any).removeChild(child); - } else { - container.removeChild(child); + let parentNode: Document | Element; + switch (container.nodeType) { + case COMMENT_NODE: + parentNode = (container.parentNode: any); + break; + case DOCUMENT_NODE: + parentNode = (container: any).body; + break; + default: + if (container.nodeName === 'HTML') { + parentNode = (container.ownerDocument.body: any); + } else { + parentNode = (container: any); + } } + parentNode.removeChild(child); } export function clearSuspenseBoundary( @@ -965,10 +1003,15 @@ export function clearSuspenseBoundaryFromContainer( ): void { if (container.nodeType === COMMENT_NODE) { clearSuspenseBoundary((container.parentNode: any), suspenseInstance); - } else if (container.nodeType === ELEMENT_NODE) { - clearSuspenseBoundary((container: any), suspenseInstance); + } else if (container.nodeType === DOCUMENT_NODE) { + clearSuspenseBoundary((container: any).body, suspenseInstance); + } else if (container.nodeName === 'HTML') { + clearSuspenseBoundary( + (container.ownerDocument.body: any), + suspenseInstance, + ); } else { - // Document nodes should never contain suspense boundaries. + clearSuspenseBoundary((container: any), suspenseInstance); } // Retry if any event replaying was blocked on this. retryIfBlockedOn(container); @@ -2299,30 +2342,6 @@ export function releaseSingletonInstance(instance: Instance): void { detachDeletedInstance(instance); } -export function clearSingleton(instance: Instance): void { - const element: Element = (instance: any); - let node = element.firstChild; - while (node) { - const nextNode = node.nextSibling; - const nodeName = node.nodeName; - if ( - isMarkedHoistable(node) || - nodeName === 'HEAD' || - nodeName === 'BODY' || - nodeName === 'SCRIPT' || - nodeName === 'STYLE' || - (nodeName === 'LINK' && - ((node: any): HTMLLinkElement).rel.toLowerCase() === 'stylesheet') - ) { - // retain these nodes - } else { - element.removeChild(node); - } - node = nextNode; - } - return; -} - // ------------------- // Resources // ------------------- diff --git a/packages/react-dom/src/__tests__/ReactDOM-test.js b/packages/react-dom/src/__tests__/ReactDOM-test.js index 92571ad69e10c..ef0dd50258122 100644 --- a/packages/react-dom/src/__tests__/ReactDOM-test.js +++ b/packages/react-dom/src/__tests__/ReactDOM-test.js @@ -566,4 +566,412 @@ describe('ReactDOM', () => { ' in App (at **)', ]); }); + + it('should render root host components into body scope when the container is a Document', async () => { + function App({phase}) { + return ( + <> + {phase < 1 ? null :
..before
} + {phase < 3 ?
before
: null} + {phase < 2 ? null :
before..
} + + + {phase < 1 ? null : } + {phase < 3 ? : null} + {phase < 2 ? null : } + + + {phase < 1 ? null :
..inside
} + {phase < 3 ?
inside
: null} + {phase < 2 ? null :
inside..
} + + + {phase < 1 ? null :
..after
} + {phase < 3 ?
after
: null} + {phase < 2 ? null :
after..
} + + ); + } + + const root = ReactDOMClient.createRoot(document); + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
before
inside
after
', + ); + + // @TODO remove this warning check when we loosen the tag nesting restrictions to allow arbitrary tags at the + // root of the application + assertConsoleErrorDev(['In HTML,
cannot be a child of <#document>']); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
..before
before
..inside
inside
..after
after
', + ); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
..before
before
before..
..inside
inside
inside..
..after
after
after..
', + ); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
..before
before..
..inside
inside..
..after
after..
', + ); + + await act(() => { + root.unmount(); + }); + expect(document.documentElement.outerHTML).toBe( + '', + ); + }); + + it('should render root host components into body scope when the container is a the tag', async () => { + function App({phase}) { + return ( + <> + {phase < 1 ? null :
..before
} + {phase < 3 ?
before
: null} + {phase < 2 ? null :
before..
} + + {phase < 1 ? null : } + {phase < 3 ? : null} + {phase < 2 ? null : } + + + {phase < 1 ? null :
..inside
} + {phase < 3 ?
inside
: null} + {phase < 2 ? null :
inside..
} + + {phase < 1 ? null :
..after
} + {phase < 3 ?
after
: null} + {phase < 2 ? null :
after..
} + + ); + } + + const root = ReactDOMClient.createRoot(document.documentElement); + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
before
inside
after
', + ); + + // @TODO remove this warning check when we loosen the tag nesting restrictions to allow arbitrary tags at the + // root of the application + assertConsoleErrorDev(['In HTML,
cannot be a child of ']); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
..before
before
..inside
inside
..after
after
', + ); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
..before
before
before..
..inside
inside
inside..
..after
after
after..
', + ); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
..before
before..
..inside
inside..
..after
after..
', + ); + + await act(() => { + root.unmount(); + }); + expect(document.documentElement.outerHTML).toBe( + '', + ); + }); + + it('should render root host components into body scope when the container is a the tag', async () => { + function App({phase}) { + return ( + <> + {phase < 1 ? null :
..before
} + {phase < 3 ?
before
: null} + {phase < 2 ? null :
before..
} + + {phase < 1 ? null : } + {phase < 3 ? : null} + {phase < 2 ? null : } + + {phase < 1 ? null :
..inside
} + {phase < 3 ?
inside
: null} + {phase < 2 ? null :
inside..
} + {phase < 1 ? null :
..after
} + {phase < 3 ?
after
: null} + {phase < 2 ? null :
after..
} + + ); + } + + const root = ReactDOMClient.createRoot(document.body); + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
before
inside
after
', + ); + + // @TODO remove this warning check when we loosen the tag nesting restrictions to allow arbitrary tags at the + // root of the application + assertConsoleErrorDev(['In HTML, cannot be a child of ']); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
..before
before
..inside
inside
..after
after
', + ); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
..before
before
before..
..inside
inside
inside..
..after
after
after..
', + ); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
..before
before..
..inside
inside..
..after
after..
', + ); + + await act(() => { + root.unmount(); + }); + expect(document.documentElement.outerHTML).toBe( + '', + ); + }); + + it('should render children of into the document head even when the container is inside the document body', async () => { + function App({phase}) { + return ( + <> +
before
+ + {phase < 1 ? null : } + {phase < 3 ? : null} + {phase < 2 ? null : } + +
after
+ + ); + } + + const container = document.createElement('main'); + document.body.append(container); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
before
after
', + ); + + // @TODO remove this warning check when we loosen the tag nesting restrictions to allow arbitrary tags at the + // root of the application + assertConsoleErrorDev(['In HTML, cannot be a child of
']); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
before
after
', + ); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
before
after
', + ); + + await act(() => { + root.render(); + }); + expect(document.documentElement.outerHTML).toBe( + '
before
after
', + ); + + await act(() => { + root.unmount(); + }); + expect(document.documentElement.outerHTML).toBe( + '
', + ); + }); + + it('can render a Suspense boundary above the tag', async () => { + let suspendOnNewPromise; + let resolveCurrentPromise; + let currentPromise; + function createNewPromise() { + currentPromise = new Promise(r => { + resolveCurrentPromise = r; + }); + return currentPromise; + } + createNewPromise(); + function Comp() { + const [promise, setPromise] = React.useState(currentPromise); + suspendOnNewPromise = () => { + setPromise(createNewPromise()); + }; + React.use(promise); + return null; + } + + const fallback = ( + + +
fallback
+ + + ); + + const main = ( + + + + + +
+ +
+ + + ); + + let suspendOnNewMessage; + let currentMessage; + let resolveCurrentMessage; + function createNewMessage() { + currentMessage = new Promise(r => { + resolveCurrentMessage = r; + }); + return currentMessage; + } + createNewMessage(); + resolveCurrentMessage('hello world'); + function Message() { + const [pendingMessage, setPendingMessage] = + React.useState(currentMessage); + suspendOnNewMessage = () => { + setPendingMessage(createNewMessage()); + }; + return React.use(pendingMessage); + } + + function App() { + return ( + + + {main} + + ); + } + + const root = ReactDOMClient.createRoot(document); + await act(() => { + root.render(); + }); + // The initial render is blocked by promiseA so we see the fallback Document + expect(document.documentElement.outerHTML).toBe( + '
fallback
', + ); + + await act(() => { + resolveCurrentPromise(); + }); + // When promiseA resolves we see the primary Document + expect(document.documentElement.outerHTML).toBe( + '
hello world
', + ); + + await act(() => { + suspendOnNewPromise(); + }); + // When we switch to rendering ComponentB synchronously we have to put the Document back into fallback + // The primary content remains hidden until promiseB resolves + expect(document.documentElement.outerHTML).toBe( + '
hello world
fallback
', + ); + + await act(() => { + resolveCurrentPromise(); + }); + // When promiseB resolves we see the new primary content inside the primary Document + // style attributes stick around after being unhidden by the Suspense boundary + expect(document.documentElement.outerHTML).toBe( + '
hello world
', + ); + + await act(() => { + React.startTransition(() => { + suspendOnNewPromise(); + }); + }); + expect(document.documentElement.outerHTML).toBe( + '
hello world
', + ); + + await act(() => { + resolveCurrentPromise(); + }); + expect(document.documentElement.outerHTML).toBe( + '
hello world
', + ); + + await act(() => { + suspendOnNewMessage(); + }); + // When we update the message itself we will be causing updates on the primary content of the Suspense boundary. + // The reason we also test for this is to make sure we don't double acquire the document singletons while + // disappearing and reappearing layout effects + expect(document.documentElement.outerHTML).toBe( + '
hello world
fallback
', + ); + + await act(() => { + resolveCurrentMessage('hello you!'); + }); + expect(document.documentElement.outerHTML).toBe( + '
hello you!
', + ); + + await act(() => { + React.startTransition(() => { + suspendOnNewMessage(); + }); + }); + expect(document.documentElement.outerHTML).toBe( + '
hello you!
', + ); + + await act(() => { + resolveCurrentMessage('goodbye!'); + }); + expect(document.documentElement.outerHTML).toBe( + '
goodbye!
', + ); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index 5f50ceb207aba..bdd50c9d4f469 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -31,7 +31,6 @@ let hasErrored = false; let fatalError = undefined; let renderOptions; let waitForAll; -let waitForThrow; let assertLog; let Scheduler; let clientAct; @@ -76,7 +75,6 @@ describe('ReactDOMFloat', () => { const InternalTestUtils = require('internal-test-utils'); waitForAll = InternalTestUtils.waitForAll; - waitForThrow = InternalTestUtils.waitForThrow; assertLog = InternalTestUtils.assertLog; clientAct = InternalTestUtils.act; assertConsoleErrorDev = InternalTestUtils.assertConsoleErrorDev; @@ -507,14 +505,7 @@ describe('ReactDOMFloat', () => { , ); - let aggregateError = await waitForThrow(); - expect(aggregateError.errors.length).toBe(2); - expect(aggregateError.errors[0].message).toContain( - 'Invalid insertion of NOSCRIPT', - ); - expect(aggregateError.errors[1].message).toContain( - 'The node to be removed is not a child of this node', - ); + await waitForAll([]); assertConsoleErrorDev([ [ 'Cannot render