Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ import {
SUSPENSE_TREE_OPERATION_REMOVE,
SUSPENSE_TREE_OPERATION_REORDER_CHILDREN,
SUSPENSE_TREE_OPERATION_RESIZE,
SUSPENSE_TREE_OPERATION_SUSPENDERS,
UNKNOWN_SUSPENDERS_NONE,
UNKNOWN_SUSPENDERS_REASON_PRODUCTION,
UNKNOWN_SUSPENDERS_REASON_OLD_VERSION,
Expand Down Expand Up @@ -2016,6 +2017,7 @@ export function attach(
const pendingOperations: OperationsArray = [];
const pendingRealUnmountedIDs: Array<FiberInstance['id']> = [];
const pendingRealUnmountedSuspenseIDs: Array<FiberInstance['id']> = [];
const pendingSuspenderChanges: Set<FiberInstance['id']> = new Set();
let pendingOperationsQueue: Array<OperationsArray> | null = [];
const pendingStringTable: Map<string, StringTableEntry> = new Map();
let pendingStringTableLength: number = 0;
Expand Down Expand Up @@ -2047,6 +2049,7 @@ export function attach(
pendingOperations.length === 0 &&
pendingRealUnmountedIDs.length === 0 &&
pendingRealUnmountedSuspenseIDs.length === 0 &&
pendingSuspenderChanges.size === 0 &&
pendingUnmountedRootID === null
);
}
Expand Down Expand Up @@ -2113,6 +2116,7 @@ export function attach(
pendingRealUnmountedIDs.length +
(pendingUnmountedRootID === null ? 0 : 1);
const numUnmountSuspenseIDs = pendingRealUnmountedSuspenseIDs.length;
const numSuspenderChanges = pendingSuspenderChanges.size;

const operations = new Array<number>(
// Identify which renderer this update is coming from.
Expand All @@ -2128,7 +2132,10 @@ export function attach(
// [TREE_OPERATION_REMOVE, removedIDLength, ...ids]
(numUnmountIDs > 0 ? 2 + numUnmountIDs : 0) +
// Regular operations
pendingOperations.length,
pendingOperations.length +
// All suspender changes are batched in a single message.
// [SUSPENSE_TREE_OPERATION_SUSPENDERS, suspenderChangesLength, ...[id, hasUniqueSuspenders]]
(numSuspenderChanges > 0 ? 2 + numSuspenderChanges * 2 : 0),
);

// Identify which renderer this update is coming from.
Expand Down Expand Up @@ -2191,19 +2198,39 @@ export function attach(
i++;
}
}
// Fill in the rest of the operations.

// Fill in pending operations.
for (let j = 0; j < pendingOperations.length; j++) {
operations[i + j] = pendingOperations[j];
}
i += pendingOperations.length;

// Suspender changes might affect newly mounted nodes that we already recorded
// in pending operations.
if (numSuspenderChanges > 0) {
operations[i++] = SUSPENSE_TREE_OPERATION_SUSPENDERS;
operations[i++] = numSuspenderChanges;
pendingSuspenderChanges.forEach(fiberIdWithChanges => {
const suspense = idToSuspenseNodeMap.get(fiberIdWithChanges);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This look up seems unnecessary. We don't do that elsewhere.

I believe it's also possible that it gets batched such that you can end up deleting it later before it's sent which would then trigger the error below.

You really know that it only goes one direction so it has to be true. Don't really need it in the protocol.

If you do need it, then you should just add it to the set but really the Set is unnecessary too since it can just be an array in order just like the others are.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do need it, then you should just add it to the set but really the Set is unnecessary too since it can just be an array in order just like the others are.

I don't trust that during one reconciler pass we can't simultaneously add unique suspenders and remove a node. In that case I need to remove it from the pending change set anyway so a Set makes sense. I could ignore suspender changes for removed nodes but that may hide different bugs.

I believe it's also possible that it gets batched such that you can end up deleting it later before it's sent which would then trigger the error below.

When you record a delete, you're supposed to remove it from the change set. That's what we're currently doing when we also remove the node from ID-map: https://github.com/eps1lon/react/blob/b3281e3b8043e48d78cf4d13e14f7e5a220b3e28/packages/react-devtools-shared/src/backend/fiber/renderer.js#L2752-L2753

We're still erroring in the store if we get a change for a removed node but I prefer the early error in the backend.

if (suspense === undefined) {
// Probably forgot to cleanup pendingSuspenderChanges when this node was removed.
throw new Error(
`Could not send suspender changes for "${fiberIdWithChanges}" since the Fiber no longer exists.`,
);
}
operations[i++] = fiberIdWithChanges;
operations[i++] = suspense.hasUniqueSuspenders ? 1 : 0;
});
}

// Let the frontend know about tree operations.
flushOrQueueOperations(operations);

// Reset all of the pending state now that we've told the frontend about it.
pendingOperations.length = 0;
pendingRealUnmountedIDs.length = 0;
pendingRealUnmountedSuspenseIDs.length = 0;
pendingSuspenderChanges.clear();
pendingUnmountedRootID = null;
pendingStringTable.clear();
pendingStringTableLength = 0;
Expand Down Expand Up @@ -2688,6 +2715,19 @@ export function attach(
}
}

function recordSuspenseSuspenders(suspenseNode: SuspenseNode): void {
if (__DEBUG__) {
console.log('recordSuspenseSuspenders()', suspenseNode);
}
const fiberInstance = suspenseNode.instance;
if (fiberInstance.kind !== FIBER_INSTANCE) {
// TODO: Suspender updates of filtered Suspense nodes are currently dropped.
return;
}

pendingSuspenderChanges.add(fiberInstance.id);
}

function recordSuspenseUnmount(suspenseInstance: SuspenseNode): void {
if (__DEBUG__) {
console.log(
Expand All @@ -2709,6 +2749,7 @@ export function attach(
// and later arrange them in the correct order.
pendingRealUnmountedSuspenseIDs.push(id);

pendingSuspenderChanges.delete(id);
idToSuspenseNodeMap.delete(id);
}

Expand Down Expand Up @@ -2779,6 +2820,7 @@ export function attach(
) {
// This didn't exist in the parent before, so let's mark this boundary as having a unique suspender.
parentSuspenseNode.hasUniqueSuspenders = true;
recordSuspenseSuspenders(parentSuspenseNode);
}
}
// We have observed at least one known reason this might have been suspended.
Expand Down Expand Up @@ -2820,6 +2862,9 @@ export function attach(
// We have found a child boundary that depended on the unblocked I/O.
// It can now be marked as having unique suspenders. We can skip its children
// since they'll still be blocked by this one.
if (!node.hasUniqueSuspenders) {
recordSuspenseSuspenders(node);
}
node.hasUniqueSuspenders = true;
node.hasUnknownSuspenders = false;
} else if (node.firstChild !== null) {
Expand Down Expand Up @@ -3522,6 +3567,9 @@ export function attach(
// Unfortunately if we don't have any DEV time debug info or debug thenables then
// we have no meta data to show. However, we still mark this Suspense boundary as
// participating in the loading sequence since apparently it can suspend.
if (!suspenseNode.hasUniqueSuspenders) {
recordSuspenseSuspenders(suspenseNode);
}
suspenseNode.hasUniqueSuspenders = true;
// We have not seen any reason yet for why this suspense node might have been
// suspended but it clearly has been at some point. If we later discover a reason
Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-shared/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const SUSPENSE_TREE_OPERATION_ADD = 8;
export const SUSPENSE_TREE_OPERATION_REMOVE = 9;
export const SUSPENSE_TREE_OPERATION_REORDER_CHILDREN = 10;
export const SUSPENSE_TREE_OPERATION_RESIZE = 11;
export const SUSPENSE_TREE_OPERATION_SUSPENDERS = 12;

export const PROFILING_FLAG_BASIC_SUPPORT = 0b01;
export const PROFILING_FLAG_TIMELINE_SUPPORT = 0b10;
Expand Down
53 changes: 49 additions & 4 deletions packages/react-devtools-shared/src/devtools/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
SUSPENSE_TREE_OPERATION_REMOVE,
SUSPENSE_TREE_OPERATION_REORDER_CHILDREN,
SUSPENSE_TREE_OPERATION_RESIZE,
SUSPENSE_TREE_OPERATION_SUSPENDERS,
} from '../constants';
import {ElementTypeRoot} from '../frontend/types';
import {
Expand Down Expand Up @@ -879,8 +880,13 @@ export default class Store extends EventEmitter<{
return null;
}

/**
* @param rootID
* @param uniqueSuspendersOnly Filters out boundaries without unique suspenders
*/
getSuspendableDocumentOrderSuspense(
rootID: Element['id'] | void,
uniqueSuspendersOnly: boolean,
): $ReadOnlyArray<SuspenseNode['id']> {
if (rootID === undefined) {
return [];
Expand All @@ -892,7 +898,7 @@ export default class Store extends EventEmitter<{
if (!this.supportsTogglingSuspense(root.id)) {
return [];
}
const suspenseTreeList: SuspenseNode['id'][] = [];
const list: SuspenseNode['id'][] = [];
const suspense = this.getSuspenseByID(root.id);
if (suspense !== null) {
const stack = [suspense];
Expand All @@ -901,9 +907,11 @@ export default class Store extends EventEmitter<{
if (current === undefined) {
continue;
}
// Include the root even if we won't suspend it.
// Include the root even if we won't show it suspended (because that's just blank).
// You should be able to see what suspended the shell.
suspenseTreeList.push(current.id);
if (!uniqueSuspendersOnly || current.hasUniqueSuspenders) {
list.push(current.id);
}
// Add children in reverse order to maintain document order
for (let j = current.children.length - 1; j >= 0; j--) {
const childSuspense = this.getSuspenseByID(current.children[j]);
Expand All @@ -914,7 +922,7 @@ export default class Store extends EventEmitter<{
}
}

return suspenseTreeList;
return list;
}

getRendererIDForElement(id: number): number | null {
Expand Down Expand Up @@ -1580,6 +1588,7 @@ export default class Store extends EventEmitter<{
children: [],
name,
rects,
hasUniqueSuspenders: false,
});

hasSuspenseTreeChanged = true;
Expand Down Expand Up @@ -1749,6 +1758,42 @@ export default class Store extends EventEmitter<{

break;
}
case SUSPENSE_TREE_OPERATION_SUSPENDERS: {
const changeLength = operations[i + 1];
i += 2;

for (let changeIndex = 0; changeIndex < changeLength; changeIndex++) {
const id = operations[i];
const hasUniqueSuspenders = operations[i + 1] === 1;
const suspense = this._idToSuspense.get(id);

if (suspense === undefined) {
this._throwAndEmitError(
Error(
`Cannot update suspenders of suspense node "${id}" because no matching node was found in the Store.`,
),
);

break;
}

i += 2;

if (__DEBUG__) {
const previousHasUniqueSuspenders = suspense.hasUniqueSuspenders;
debug(
'Suspender changes',
`Suspense node ${id} unique suspenders set to ${String(hasUniqueSuspenders)} (was ${String(previousHasUniqueSuspenders)})`,
);
}

suspense.hasUniqueSuspenders = hasUniqueSuspenders;
}

hasSuspenseTreeChanged = true;

break;
}
default:
this._throwAndEmitError(
new UnsupportedBridgeOperationError(
Expand Down
14 changes: 7 additions & 7 deletions packages/react-devtools-shared/src/devtools/views/ButtonIcon.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ type Props = {
type: IconType,
};

const materialIconsViewBox = '0 -960 960 960';
const panelIcons = '0 -960 960 820';
export default function ButtonIcon({className = '', type}: Props): React.Node {
let pathData = null;
let viewBox = '0 0 24 24';
Expand Down Expand Up @@ -131,27 +131,27 @@ export default function ButtonIcon({className = '', type}: Props): React.Node {
break;
case 'panel-left-close':
pathData = PATH_MATERIAL_PANEL_LEFT_CLOSE;
viewBox = materialIconsViewBox;
viewBox = panelIcons;
break;
case 'panel-left-open':
pathData = PATH_MATERIAL_PANEL_LEFT_OPEN;
viewBox = materialIconsViewBox;
viewBox = panelIcons;
break;
case 'panel-right-close':
pathData = PATH_MATERIAL_PANEL_RIGHT_CLOSE;
viewBox = materialIconsViewBox;
viewBox = panelIcons;
break;
case 'panel-right-open':
pathData = PATH_MATERIAL_PANEL_RIGHT_OPEN;
viewBox = materialIconsViewBox;
viewBox = panelIcons;
break;
case 'panel-bottom-open':
pathData = PATH_MATERIAL_PANEL_BOTTOM_OPEN;
viewBox = materialIconsViewBox;
viewBox = panelIcons;
break;
case 'panel-bottom-close':
pathData = PATH_MATERIAL_PANEL_BOTTOM_CLOSE;
viewBox = materialIconsViewBox;
viewBox = panelIcons;
break;
case 'suspend':
pathData = PATH_SUSPEND;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
SUSPENSE_TREE_OPERATION_REMOVE,
SUSPENSE_TREE_OPERATION_REORDER_CHILDREN,
SUSPENSE_TREE_OPERATION_RESIZE,
SUSPENSE_TREE_OPERATION_SUSPENDERS,
} from 'react-devtools-shared/src/constants';
import {
parseElementDisplayNameFromBackend,
Expand Down Expand Up @@ -452,6 +453,18 @@ function updateTree(
break;
}

case SUSPENSE_TREE_OPERATION_SUSPENDERS: {
const changesLength = ((operations[i + 1]: any): number);

if (__DEBUG__) {
const changes = operations.slice(i + 2, i + 2 + changesLength * 2);
debug('Suspender changes', `[${changes.join(',')}]`);
}

i += 2 + changesLength * 2;
break;
}

default:
throw Error(`Unsupported Bridge operation "${operation}"`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
display: flex;
flex-direction: row;
padding: 0.25rem;
align-items: center;
}

.SuspenseTimelineInput {
Expand Down
Loading
Loading