From 36bfe03ef6287f16ebb3aba12848ad0e8daff394 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Tue, 11 Mar 2025 18:31:13 -0700 Subject: [PATCH 1/2] fix: Support React Suspense in collections --- .../@react-aria/collections/src/Document.ts | 235 ++++++++++-------- .../stories/Table.stories.tsx | 81 +++++- .../react-aria-components/test/Table.test.js | 92 ++++++- 3 files changed, 303 insertions(+), 105 deletions(-) diff --git a/packages/@react-aria/collections/src/Document.ts b/packages/@react-aria/collections/src/Document.ts index 893d664fe0a..e2c8f14d033 100644 --- a/packages/@react-aria/collections/src/Document.ts +++ b/packages/@react-aria/collections/src/Document.ts @@ -118,7 +118,6 @@ export class BaseNode { } appendChild(child: ElementNode) { - this.ownerDocument.startTransaction(); if (child.parentNode) { child.parentNode.removeChild(child); } @@ -141,13 +140,6 @@ export class BaseNode { this.lastChild = child; this.ownerDocument.markDirty(this); - if (child.hasSetProps) { - // Only add the node to the collection if we already received props for it. - // Otherwise wait until then so we have the correct id for the node. - this.ownerDocument.addNode(child); - } - - this.ownerDocument.endTransaction(); this.ownerDocument.queueUpdate(); } @@ -156,7 +148,6 @@ export class BaseNode { return this.appendChild(newNode); } - this.ownerDocument.startTransaction(); if (newNode.parentNode) { newNode.parentNode.removeChild(newNode); } @@ -175,12 +166,6 @@ export class BaseNode { newNode.parentNode = referenceNode.parentNode; this.invalidateChildIndices(referenceNode); - - if (newNode.hasSetProps) { - this.ownerDocument.addNode(newNode); - } - - this.ownerDocument.endTransaction(); this.ownerDocument.queueUpdate(); } @@ -188,8 +173,6 @@ export class BaseNode { if (child.parentNode !== this || !this.ownerDocument.isMounted) { return; } - - this.ownerDocument.startTransaction(); if (child.nextSibling) { this.invalidateChildIndices(child.nextSibling); @@ -213,13 +196,44 @@ export class BaseNode { child.previousSibling = null; child.index = 0; - this.ownerDocument.removeNode(child); - this.ownerDocument.endTransaction(); + this.ownerDocument.markDirty(child); this.ownerDocument.queueUpdate(); } addEventListener() {} removeEventListener() {} + + get previousVisibleSibling(): ElementNode | null { + let node = this.previousSibling; + while (node && node.isHidden) { + node = node.previousSibling; + } + return node; + } + + get nextVisibleSibling(): ElementNode | null { + let node = this.nextSibling; + while (node && node.isHidden) { + node = node.nextSibling; + } + return node; + } + + get firstVisibleChild(): ElementNode | null { + let node = this.firstChild; + while (node && node.isHidden) { + node = node.nextSibling; + } + return node; + } + + get lastVisibleChild(): ElementNode | null { + let node = this.lastChild; + while (node && node.isHidden) { + node = node.previousSibling; + } + return node; + } } /** @@ -229,16 +243,14 @@ export class BaseNode { export class ElementNode extends BaseNode { nodeType = 8; // COMMENT_NODE (we'd use ELEMENT_NODE but React DevTools will fail to get its dimensions) node: CollectionNode; + isMutated = true; private _index: number = 0; hasSetProps = false; + isHidden = false; constructor(type: string, ownerDocument: Document) { super(ownerDocument); this.node = new CollectionNode(type, `react-aria-${++ownerDocument.nodeId}`); - // Start a transaction so that no updates are emitted from the collection - // until the props for this node are set. We don't know the real id for the - // node until then, so we need to avoid emitting collections in an inconsistent state. - this.ownerDocument.startTransaction(); } get index() { @@ -258,30 +270,45 @@ export class ElementNode extends BaseNode { return 0; } + /** + * Lazily gets a mutable instance of a Node. If the node has already + * been cloned during this update cycle, it just returns the existing one. + */ + private getMutableNode(): Mutable> { + if (!this.isMutated) { + this.node = this.node.clone(); + this.isMutated = true; + } + + this.ownerDocument.markDirty(this); + return this.node; + } + updateNode() { - let node = this.ownerDocument.getMutableNode(this); + let nextSibling = this.nextVisibleSibling; + let node = this.getMutableNode(); node.index = this.index; node.level = this.level; node.parentKey = this.parentNode instanceof ElementNode ? this.parentNode.node.key : null; - node.prevKey = this.previousSibling?.node.key ?? null; - node.nextKey = this.nextSibling?.node.key ?? null; + node.prevKey = this.previousVisibleSibling?.node.key ?? null; + node.nextKey = nextSibling?.node.key ?? null; node.hasChildNodes = !!this.firstChild; - node.firstChildKey = this.firstChild?.node.key ?? null; - node.lastChildKey = this.lastChild?.node.key ?? null; + node.firstChildKey = this.firstVisibleChild?.node.key ?? null; + node.lastChildKey = this.lastVisibleChild?.node.key ?? null; // Update the colIndex of sibling nodes if this node has a colSpan. - if ((node.colSpan != null || node.colIndex != null) && this.nextSibling) { + if ((node.colSpan != null || node.colIndex != null) && nextSibling) { // This queues the next sibling for update, which means this happens recursively. let nextColIndex = (node.colIndex ?? node.index) + (node.colSpan ?? 1); - if (nextColIndex !== this.nextSibling.node.colIndex) { - let siblingNode = this.ownerDocument.getMutableNode(this.nextSibling); + if (nextColIndex !== nextSibling.node.colIndex) { + let siblingNode = nextSibling.getMutableNode(); siblingNode.colIndex = nextColIndex; } } } setProps(obj: any, ref: ForwardedRef, rendered?: any, render?: (node: Node) => ReactElement) { - let node = this.ownerDocument.getMutableNode(this); + let node = this.getMutableNode(); let {value, textValue, id, ...props} = obj; props.ref = ref; node.props = props; @@ -300,19 +327,44 @@ export class ElementNode extends BaseNode { node.colSpan = props.colSpan; } - // If this is the first time props have been set, end the transaction started in the constructor - // so this node can be emitted. - if (!this.hasSetProps) { - this.ownerDocument.addNode(this); - this.ownerDocument.endTransaction(); - this.hasSetProps = true; - } - + this.hasSetProps = true; this.ownerDocument.queueUpdate(); } get style() { - return {}; + // React sets display: none to hide elements during Suspense. + // We'll handle this by setting the element to hidden and invalidating + // its siblings/parent. Hidden elements remain in the Document, but + // are removed from the Collection. + let element = this; + return { + get display() { + return element.isHidden ? 'none' : ''; + }, + set display(value) { + let isHidden = value === 'none'; + if (element.isHidden !== isHidden) { + // Mark parent node dirty if this element is currently the first or last visible child. + if (element.parentNode?.firstVisibleChild === element || element.parentNode?.lastVisibleChild === element) { + element.ownerDocument.markDirty(element.parentNode); + } + + // Mark sibling visible elements dirty. + let prev = element.previousVisibleSibling; + let next = element.nextVisibleSibling; + if (prev) { + element.ownerDocument.markDirty(prev); + } + if (next) { + element.ownerDocument.markDirty(next); + } + + // Mark self dirty. + element.isHidden = isHidden; + element.ownerDocument.markDirty(element); + } + } + }; } hasAttribute() {} @@ -334,10 +386,8 @@ export class Document = BaseCollection> extend nodesByProps = new WeakMap>(); isMounted = true; private collection: C; - private collectionMutated: boolean; - private mutatedNodes: Set> = new Set(); + private nextCollection: C | null = null; private subscriptions: Set<() => void> = new Set(); - private transactionCount = 0; private queuedRender = false; private inSubscription = false; @@ -345,7 +395,7 @@ export class Document = BaseCollection> extend // @ts-ignore super(null); this.collection = collection; - this.collectionMutated = true; + this.nextCollection = collection; } get isConnected() { @@ -356,82 +406,64 @@ export class Document = BaseCollection> extend return new ElementNode(type, this); } - /** - * Lazily gets a mutable instance of a Node. If the node has already - * been cloned during this update cycle, it just returns the existing one. - */ - getMutableNode(element: ElementNode): Mutable> { - let node = element.node; - if (!this.mutatedNodes.has(element)) { - node = element.node.clone(); - this.mutatedNodes.add(element); - element.node = node; - } - this.markDirty(element); - return node; - } - private getMutableCollection() { - if (!this.isSSR && !this.collectionMutated) { - this.collection = this.collection.clone(); - this.collectionMutated = true; + if (this.isSSR) { + return this.collection; } - return this.collection; + if (!this.nextCollection) { + this.nextCollection = this.collection.clone(); + } + + return this.nextCollection; } markDirty(node: BaseNode) { this.dirtyNodes.add(node); } - startTransaction() { - this.transactionCount++; - } - - endTransaction() { - this.transactionCount--; - } + private addNode(element: ElementNode) { + if (element.isHidden) { + return; + } - addNode(element: ElementNode) { let collection = this.getMutableCollection(); if (!collection.getItem(element.node.key)) { - collection.addNode(element.node); - for (let child of element) { this.addNode(child); } } - this.markDirty(element); + collection.addNode(element.node); } - removeNode(node: ElementNode) { + private removeNode(node: ElementNode) { for (let child of node) { this.removeNode(child); } let collection = this.getMutableCollection(); collection.removeNode(node.node.key); - this.markDirty(node); } /** Finalizes the collection update, updating all nodes and freezing the collection. */ getCollection(): C { - if (this.transactionCount > 0) { - return this.collection; + // If in a subscription update, return a clone of the existing collection. + // This ensures React will queue a render. React will call getCollection again + // during render, at which point all the updates will be complete and we can return + // the new collection. + if (this.inSubscription) { + return this.collection.clone(); } - this.updateCollection(); - // Reset queuedRender to false when getCollection is called during render. - if (!this.inSubscription) { - this.queuedRender = false; - } + this.queuedRender = false; + this.updateCollection(); return this.collection; } - updateCollection() { + private updateCollection() { // First, update the indices of dirty element children. for (let element of this.dirtyNodes) { element.updateChildIndices(); @@ -439,36 +471,33 @@ export class Document = BaseCollection> extend // Next, update dirty collection nodes. for (let element of this.dirtyNodes) { - if (element instanceof ElementNode && element.isConnected) { - element.updateNode(); + if (element instanceof ElementNode) { + if (element.isConnected && !element.isHidden) { + element.updateNode(); + this.addNode(element); + } else { + this.removeNode(element); + } + + element.isMutated = false; } } this.dirtyNodes.clear(); // Finally, update the collection. - if (this.mutatedNodes.size || this.collectionMutated) { - let collection = this.getMutableCollection(); - for (let element of this.mutatedNodes) { - if (element.isConnected) { - collection.addNode(element.node); - } - } - - this.mutatedNodes.clear(); - collection.commit(this.firstChild?.node.key ?? null, this.lastChild?.node.key ?? null, this.isSSR); + if (this.nextCollection) { + this.nextCollection.commit(this.firstVisibleChild?.node.key ?? null, this.lastVisibleChild?.node.key ?? null, this.isSSR); + this.collection = this.nextCollection; + this.nextCollection = null; } - - this.collectionMutated = false; } queueUpdate() { - // Don't emit any updates if there is a transaction in progress. - // queueUpdate should be called again after the transaction. - if (this.dirtyNodes.size === 0 || this.transactionCount > 0 || this.queuedRender) { + if (this.dirtyNodes.size === 0 || this.queuedRender) { return; } - + // Only trigger subscriptions once during an update, when the first item changes. // React's useSyncExternalStore will call getCollection immediately, to check whether the snapshot changed. // If so, React will queue a render to happen after the current commit to our fake DOM finishes. diff --git a/packages/react-aria-components/stories/Table.stories.tsx b/packages/react-aria-components/stories/Table.stories.tsx index 64f059d9b5b..6faf890739a 100644 --- a/packages/react-aria-components/stories/Table.stories.tsx +++ b/packages/react-aria-components/stories/Table.stories.tsx @@ -14,7 +14,7 @@ import {action} from '@storybook/addon-actions'; import {Button, Cell, Checkbox, CheckboxProps, Collection, Column, ColumnProps, ColumnResizer, Dialog, DialogTrigger, DropIndicator, Heading, Menu, MenuTrigger, Modal, ModalOverlay, Popover, ResizableTableContainer, Row, Table, TableBody, TableHeader, TableLayout, useDragAndDrop, Virtualizer} from 'react-aria-components'; import {isTextDropItem} from 'react-aria'; import {MyMenuItem} from './utils'; -import React, {useMemo, useRef} from 'react'; +import React, {Suspense, useMemo, useRef, useState} from 'react'; import styles from '../example/index.css'; import {UNSTABLE_TableLoadingIndicator} from '../src/Table'; import {useAsyncList, useListData} from 'react-stately'; @@ -980,3 +980,82 @@ export const OnLoadMoreTableVirtualizedResizeWrapperStory = { render: OnLoadMoreTableVirtualizedResizeWrapper, name: 'Virtualized Table with async loading, resizable table container wrapper' }; + +interface Launch { + id: number, + mission_name: string, + launch_year: number +} + +const items: Launch[] = [ + {id: 0, mission_name: 'FalconSat', launch_year: 2006}, + {id: 1, mission_name: 'DemoSat', launch_year: 2007}, + {id: 2, mission_name: 'Trailblazer', launch_year: 2008}, + {id: 3, mission_name: 'RatSat', launch_year: 2009} +]; + +function makePromise(items: Launch[]) { + return new Promise(resolve => setTimeout(() => resolve(items), 1000)); +} + +function TableSuspense({reactTransition = false}) { + let [promise, setPromise] = useState(() => makePromise(items.slice(0, 2))); + let [isPending, startTransition] = React.useTransition(); + return ( +
+ + + Name + Year + + + + Loading... + + }> + + + +
+ +
+ ); +} + +function LocationsTableBody({promise}) { + let items = React.use(promise); + + return items.map(item => ( + + {item.mission_name} + {item.launch_year} + + )); +} + +export const TableWithSuspense = { + render: React.use != null ? TableSuspense : () => 'This story requires React 19.', + args: { + reactTransition: false + }, + parameters: { + description: { + data: 'Expected behavior: With reactTransition=false, rows should be replaced by loading indicator when pressing button. With reactTransition=true, existing rows should remain and loading should appear inside the button.' + } + } +}; diff --git a/packages/react-aria-components/test/Table.test.js b/packages/react-aria-components/test/Table.test.js index 8afc22b248c..3d72ef98283 100644 --- a/packages/react-aria-components/test/Table.test.js +++ b/packages/react-aria-components/test/Table.test.js @@ -25,7 +25,8 @@ import userEvent from '@testing-library/user-event'; let { RenderEmptyStateStory: EmptyLoadingTable, TableLoadingBodyWrapperStory: LoadingMoreTable, - TableCellColSpanWithVariousSpansExample: TableCellColSpan + TableCellColSpanWithVariousSpansExample: TableCellColSpan, + TableWithSuspense } = composeStories(stories); function MyColumn(props) { @@ -2056,4 +2057,93 @@ describe('Table', () => { expect(checkbox).toBeInTheDocument(); }); }); + + describe('Suspense', () => { + it('should support React Suspense without transitions', async () => { + // Only supported in React 19. + if (!React.use) { + return; + } + + // Render must be wrapped in an awaited act because the table suspends. + let tree = await act(() => render()); + + let rows = tree.getAllByRole('row'); + expect(rows).toHaveLength(2); + expect(rows[1]).toHaveTextContent('Loading...'); + + await act(async () => jest.runAllTimers()); + + rows = tree.getAllByRole('row'); + expect(rows).toHaveLength(3); + expect(rows[1]).toHaveTextContent('FalconSat'); + expect(rows[2]).toHaveTextContent('DemoSat'); + + let button = tree.getByRole('button'); + let promise = act(() => button.click()); + + rows = tree.getAllByRole('row'); + expect(rows).toHaveLength(2); + expect(rows[1]).toHaveTextContent('Loading...'); + + // Make react think we've awaited the promise. + // We can't actually await until after we runAllTimers, which is + // what resolves the promise above. + promise.then(() => {}); + // eslint-disable-next-line + jest.runAllTimers() + await promise; + + rows = tree.getAllByRole('row'); + expect(rows).toHaveLength(5); + expect(rows[1]).toHaveTextContent('FalconSat'); + expect(rows[2]).toHaveTextContent('DemoSat'); + expect(rows[3]).toHaveTextContent('Trailblazer'); + expect(rows[4]).toHaveTextContent('RatSat'); + }); + + it('should support React Suspense with transitions', async () => { + // Only supported in React 19. + if (!React.use) { + return; + } + + let tree = await act(() => render()); + + let rows = tree.getAllByRole('row'); + expect(rows).toHaveLength(2); + expect(rows[1]).toHaveTextContent('Loading...'); + + await act(async () => jest.runAllTimers()); + + rows = tree.getAllByRole('row'); + expect(rows).toHaveLength(3); + expect(rows[1]).toHaveTextContent('FalconSat'); + expect(rows[2]).toHaveTextContent('DemoSat'); + + let button = tree.getByRole('button'); + expect(button).toHaveTextContent('Load more'); + + let promise = act(() => button.click()); + + rows = tree.getAllByRole('row'); + expect(rows).toHaveLength(3); + expect(rows[1]).toHaveTextContent('FalconSat'); + expect(rows[2]).toHaveTextContent('DemoSat'); + + promise.then(() => {}); + // eslint-disable-next-line + jest.runAllTimers(); + await promise; + + expect(button).toHaveTextContent('Load more'); + + rows = tree.getAllByRole('row'); + expect(rows).toHaveLength(5); + expect(rows[1]).toHaveTextContent('FalconSat'); + expect(rows[2]).toHaveTextContent('DemoSat'); + expect(rows[3]).toHaveTextContent('Trailblazer'); + expect(rows[4]).toHaveTextContent('RatSat'); + }); + }); }); From ff049bd68b5fbac77575d96938e6856bdf0936f4 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Tue, 11 Mar 2025 18:59:22 -0700 Subject: [PATCH 2/2] Fix SSR --- packages/@react-aria/collections/src/Document.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/@react-aria/collections/src/Document.ts b/packages/@react-aria/collections/src/Document.ts index e2c8f14d033..4c5a2467804 100644 --- a/packages/@react-aria/collections/src/Document.ts +++ b/packages/@react-aria/collections/src/Document.ts @@ -407,10 +407,6 @@ export class Document = BaseCollection> extend } private getMutableCollection() { - if (this.isSSR) { - return this.collection; - } - if (!this.nextCollection) { this.nextCollection = this.collection.clone(); } @@ -463,7 +459,7 @@ export class Document = BaseCollection> extend return this.collection; } - private updateCollection() { + updateCollection() { // First, update the indices of dirty element children. for (let element of this.dirtyNodes) { element.updateChildIndices(); @@ -488,8 +484,10 @@ export class Document = BaseCollection> extend // Finally, update the collection. if (this.nextCollection) { this.nextCollection.commit(this.firstVisibleChild?.node.key ?? null, this.lastVisibleChild?.node.key ?? null, this.isSSR); - this.collection = this.nextCollection; - this.nextCollection = null; + if (!this.isSSR) { + this.collection = this.nextCollection; + this.nextCollection = null; + } } }