From 701a308113c40aca22a47b2cc84f8f14618a3334 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 1 Nov 2019 13:49:08 -0700 Subject: [PATCH] Don't show empty (no work) commits in Profiler --- .../src/__tests__/profilerStore-test.js | 40 +++++++++++++++++++ .../src/backend/renderer.js | 14 ++++++- .../src/backend/types.js | 3 ++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/profilerStore-test.js b/packages/react-devtools-shared/src/__tests__/profilerStore-test.js index 6fcd1efd31eaf..cae3ff41a5300 100644 --- a/packages/react-devtools-shared/src/__tests__/profilerStore-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilerStore-test.js @@ -79,4 +79,44 @@ describe('ProfilerStore', () => { store.profilerStore.profilingData = fauxProfilingData; expect(store.profilerStore.profilingData).toBe(fauxProfilingData); }); + + // This test covers current broken behavior (arguably) with the synthetic event system. + it('should filter empty commits', () => { + const inputRef = React.createRef(); + const ControlledInput = () => { + const [name, setName] = React.useState('foo'); + const handleChange = event => setName(event.target.value); + return ; + }; + + const container = document.createElement('div'); + + // This element has to be in the for the event system to work. + document.body.appendChild(container); + + // It's important that this test uses legacy sync mode. + // The root API does not trigger this particular failing case. + ReactDOM.render(, container); + + utils.act(() => store.profilerStore.startProfiling()); + + // Sets a value in a way that React doesn't see, + // so that a subsequent "change" event will trigger the event handler. + const setUntrackedValue = Object.getOwnPropertyDescriptor( + HTMLInputElement.prototype, + 'value', + ).set; + + const target = inputRef.current; + setUntrackedValue.call(target, 'bar'); + target.dispatchEvent(new Event('input', {bubbles: true, cancelable: true})); + expect(target.value).toBe('bar'); + + utils.act(() => store.profilerStore.stopProfiling()); + + // Only one commit should have been recorded (in response to the "change" event). + const root = store.roots[0]; + const data = store.profilerStore.getDataForRoot(root); + expect(data.commitData).toHaveLength(1); + }); }); diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 287ec898e3f87..fb1a10aa7f5a2 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -1643,6 +1643,7 @@ export function attach( } } } + if (shouldIncludeInTree) { const isProfilingSupported = nextFiber.hasOwnProperty('treeBaseDuration'); if (isProfilingSupported) { @@ -1742,6 +1743,15 @@ export function attach( 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. + const didBailoutAtRoot = + root.current === root.current.alternate || + (alternate !== null && + alternate.expirationTime === 0 && + alternate.childExpirationTime === 0); + currentRootID = getFiberID(getPrimaryFiber(current)); // Before the traversals, remember to start tracking @@ -1758,7 +1768,7 @@ export function attach( // where some v16 renderers support profiling and others don't. const isProfilingSupported = root.memoizedInteractions != null; - if (isProfiling && isProfilingSupported) { + if (isProfiling && isProfilingSupported && !didBailoutAtRoot) { // If profiling is active, store commit time and duration, and the current interactions. // The frontend may request this information after profiling has stopped. currentCommitProfilingMetadata = { @@ -1802,7 +1812,7 @@ export function attach( mountFiberRecursively(current, null, false, false); } - if (isProfiling && isProfilingSupported) { + if (isProfiling && isProfilingSupported && !didBailoutAtRoot) { const commitProfilingMetadata = ((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).get( currentRootID, ); diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 118049abc2d87..044c153cceeab 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -284,6 +284,9 @@ export type DevToolsHook = { onCommitFiberRoot: ( rendererID: RendererID, fiber: Object, + // Added in v16.9 to support Profiler priority labels commitPriority?: number, + // Added in v16.9 to support Fast Refresh + didError?: boolean, ) => void, };