Skip to content

Commit 60aa3fa

Browse files
author
Brian Vaughn
committed
Fix DevTools profiling bailout check
I recently changed DevTools to filter certain types of empty commits (#17253) to avoid showing them in the Profiler UI. I believe this change lead to an increase in the number of 'Could not find node with id...' errors being reported for the Profiler. This commit updates flushPendingEvents() to mirror the bailout check I added previously. This prevents one cause of operations and commit data mismatching that could cause this error. I am convinced there is another case still that I need to fix though.
1 parent b05cd61 commit 60aa3fa

File tree

2 files changed

+22
-3
lines changed

2 files changed

+22
-3
lines changed

packages/react-devtools-shared/src/__tests__/profilerStore-test.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,5 +118,6 @@ describe('ProfilerStore', () => {
118118
const root = store.roots[0];
119119
const data = store.profilerStore.getDataForRoot(root);
120120
expect(data.commitData).toHaveLength(1);
121+
expect(data.operations).toHaveLength(1);
121122
});
122123
});

packages/react-devtools-shared/src/backend/renderer.js

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1021,12 +1021,29 @@ export function attach(
10211021
pendingSimulatedUnmountedIDs.length === 0 &&
10221022
pendingUnmountedRootID === null
10231023
) {
1024-
// If we're currently profiling, send an "operations" method even if there are no mutations to the tree.
1025-
// The frontend needs this no-op info to know how to reconstruct the tree for each commit,
1026-
// even if a particular commit didn't change the shape of the tree.
1024+
// If we aren't profiling, it's safe to just bail without doing any more wokr.
10271025
if (!isProfiling) {
10281026
return;
10291027
}
1028+
1029+
const current = root.current;
1030+
const alternate = current.alternate;
1031+
1032+
// Certain types of updates bail out at the root without doing any actual render work.
1033+
// React should probably not call the DevTools commit hook in this case,
1034+
// but if it does- we can detect it and filter them out from the profiler.
1035+
// NOTE: Keep this logic in sync with the one in handleCommitFiberRoot()
1036+
const didBailoutAtRoot =
1037+
alternate !== null &&
1038+
alternate.expirationTime === 0 &&
1039+
alternate.childExpirationTime === 0;
1040+
1041+
// If we are currently profiling, we should only skip flushing updates we've filtered.
1042+
// Failing to do this could cause errors when rebuilding the commit tree,
1043+
// since it's important for operations indices and commit data indices to align.
1044+
if (didBailoutAtRoot) {
1045+
return;
1046+
}
10301047
}
10311048

10321049
const numUnmountIDs =
@@ -1758,6 +1775,7 @@ export function attach(
17581775
// Certain types of updates bail out at the root without doing any actual render work.
17591776
// React should probably not call the DevTools commit hook in this case,
17601777
// but if it does- we can detect it and filter them out from the profiler.
1778+
// NOTE: Keep this logic in sync with the one in flushPendingEvents()
17611779
const didBailoutAtRoot =
17621780
alternate !== null &&
17631781
alternate.expirationTime === 0 &&

0 commit comments

Comments
 (0)