Skip to content

Commit

Permalink
Fix DevTools profiling bailout check
Browse files Browse the repository at this point in the history
I recently changed DevTools to filter certain types of empty commits (facebook#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.
  • Loading branch information
Brian Vaughn committed Jan 2, 2020
1 parent b05cd61 commit 60aa3fa
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -118,5 +118,6 @@ describe('ProfilerStore', () => {
const root = store.roots[0];
const data = store.profilerStore.getDataForRoot(root);
expect(data.commitData).toHaveLength(1);
expect(data.operations).toHaveLength(1);
});
});
24 changes: 21 additions & 3 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1021,12 +1021,29 @@ export function attach(
pendingSimulatedUnmountedIDs.length === 0 &&
pendingUnmountedRootID === null
) {
// If we're currently profiling, send an "operations" method even if there are no mutations to the tree.
// The frontend needs this no-op info to know how to reconstruct the tree for each commit,
// even if a particular commit didn't change the shape of the tree.
// If we aren't profiling, it's safe to just bail without doing any more wokr.
if (!isProfiling) {
return;
}

const current = root.current;
const alternate = current.alternate;

// Certain types of updates bail out at the root without doing any actual render work.
// React should probably not call the DevTools commit hook in this case,
// but if it does- we can detect it and filter them out from the profiler.
// NOTE: Keep this logic in sync with the one in handleCommitFiberRoot()
const didBailoutAtRoot =
alternate !== null &&
alternate.expirationTime === 0 &&
alternate.childExpirationTime === 0;

// If we are currently profiling, we should only skip flushing updates we've filtered.
// Failing to do this could cause errors when rebuilding the commit tree,
// since it's important for operations indices and commit data indices to align.
if (didBailoutAtRoot) {
return;
}
}

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

0 comments on commit 60aa3fa

Please sign in to comment.