diff --git a/packages/react-devtools-shared/src/__tests__/editing-test.js b/packages/react-devtools-shared/src/__tests__/editing-test.js index 750e13e38d5d..532b538429e4 100644 --- a/packages/react-devtools-shared/src/__tests__/editing-test.js +++ b/packages/react-devtools-shared/src/__tests__/editing-test.js @@ -82,7 +82,12 @@ describe('editing interface', () => { shallow="initial" /> , - + , ), ); @@ -259,6 +264,14 @@ describe('editing interface', () => { }, after: 'initial', }); + renamePath(hostComponentID, ['data-foo'], ['data-bar']); + expect({ + foo: inputRef.current.dataset.foo, + bar: inputRef.current.dataset.bar, + }).toEqual({ + foo: undefined, + bar: 'test', + }); }); // @reactVersion >= 16.9 diff --git a/packages/react-devtools-shared/src/__tests__/profilingCharts-test.js b/packages/react-devtools-shared/src/__tests__/profilingCharts-test.js index e54af61d6209..da20511a650e 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingCharts-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingCharts-test.js @@ -298,4 +298,164 @@ describe('profiling charts', () => { `); }); }); + + describe('components behind filtered fibers should not report false re-renders', () => { + it('should not report a component as re-rendered when its filtered parent bailed out', () => { + let triggerUpdate; + + function Count() { + const [count, setCount] = React.useState(0); + triggerUpdate = () => setCount(c => c + 1); + Scheduler.unstable_advanceTime(5); + return count; + } + + function Greeting() { + Scheduler.unstable_advanceTime(3); + return 'Hello'; + } + + function App() { + Scheduler.unstable_advanceTime(1); + return ( + + +
+ +
+
+ ); + } + + utils.act(() => store.profilerStore.startProfiling()); + utils.act(() => render()); + + // Verify tree structure: div is filtered, so Greeting appears as child of App + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + + + `); + + // Trigger a state update in Count. Should not cause Greeting to re-render. + utils.act(() => triggerUpdate()); + + utils.act(() => store.profilerStore.stopProfiling()); + + const rootID = store.roots[0]; + const {chartData} = getFlamegraphChartData(rootID, 1); + const allNodes = chartData.rows.flat(); + + expect(allNodes).toEqual([ + expect.objectContaining({name: 'App', didRender: false}), + expect.objectContaining({name: 'Greeting', didRender: false}), + expect.objectContaining({name: 'Count', didRender: true}), + ]); + }); + + it('should not report a component as re-rendered when behind a filtered fragment', () => { + let triggerUpdate; + + function Count() { + const [count, setCount] = React.useState(0); + triggerUpdate = () => setCount(c => c + 1); + Scheduler.unstable_advanceTime(5); + return count; + } + + function Greeting() { + Scheduler.unstable_advanceTime(3); + return 'Hello'; + } + + function App() { + Scheduler.unstable_advanceTime(1); + return ( + + + + + + + ); + } + + utils.act(() => store.profilerStore.startProfiling()); + utils.act(() => render()); + + // Fragment with null key is filtered, so Greeting appears as child of App + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + + + `); + + // Trigger a state update in Count + utils.act(() => triggerUpdate()); + + utils.act(() => store.profilerStore.stopProfiling()); + + const rootID = store.roots[0]; + const {chartData} = getFlamegraphChartData(rootID, 1); + const allNodes = chartData.rows.flat(); + + expect(allNodes).toEqual([ + expect.objectContaining({name: 'App', didRender: false}), + expect.objectContaining({name: 'Greeting', didRender: false}), + expect.objectContaining({name: 'Count', didRender: true}), + ]); + }); + + it('should correctly report sibling components that did not re-render', () => { + let triggerUpdate; + + function Count() { + const [count, setCount] = React.useState(0); + triggerUpdate = () => setCount(c => c + 1); + Scheduler.unstable_advanceTime(5); + return count; + } + + function Greeting() { + Scheduler.unstable_advanceTime(3); + return 'Hello'; + } + + function App() { + Scheduler.unstable_advanceTime(1); + return ( + + + + + ); + } + + utils.act(() => store.profilerStore.startProfiling()); + utils.act(() => render()); + + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + + + `); + + utils.act(() => triggerUpdate()); + + utils.act(() => store.profilerStore.stopProfiling()); + + const rootID = store.roots[0]; + const {chartData} = getFlamegraphChartData(rootID, 1); + const allNodes = chartData.rows.flat(); + + expect(allNodes).toEqual([ + expect.objectContaining({name: 'App', didRender: false}), + expect.objectContaining({name: 'Greeting', didRender: false}), + expect.objectContaining({name: 'Count', didRender: true}), + ]); + }); + }); }); diff --git a/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js b/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js index f08629dd05df..5879032bada1 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingCommitTreeBuilder-test.js @@ -191,4 +191,73 @@ describe('commit tree', () => { expect(commitTrees[1].nodes.size).toBe(2); // + }); }); + + describe('Suspense', () => { + it('should handle transitioning from fallback back to content during profiling', async () => { + let resolvePromise; + let promise = null; + let childTreeBaseDuration = 10; + + function Wrapper({children}) { + Scheduler.unstable_advanceTime(2); + return children; + } + + function Child() { + Scheduler.unstable_advanceTime(childTreeBaseDuration); + if (promise !== null) { + React.use(promise); + } + return ; + } + + function GrandChild() { + Scheduler.unstable_advanceTime(5); + return null; + } + + function Fallback() { + Scheduler.unstable_advanceTime(2); + return null; + } + + function App() { + Scheduler.unstable_advanceTime(1); + return ( + }> + + + + + ); + } + + utils.act(() => store.profilerStore.startProfiling()); + + // Commit 1: Mount with primary + utils.act(() => render()); + + // Commit 2: Suspend, show fallback + promise = new Promise(resolve => (resolvePromise = resolve)); + await utils.actAsync(() => render()); + + // Commit 3: Resolve suspended promise, show primary content with a different duration. + childTreeBaseDuration = 20; + promise = null; + await utils.actAsync(resolvePromise); + utils.act(() => render()); + + utils.act(() => store.profilerStore.stopProfiling()); + + const rootID = store.roots[0]; + const dataForRoot = store.profilerStore.getDataForRoot(rootID); + const numCommits = dataForRoot.commitData.length; + for (let commitIndex = 0; commitIndex < numCommits; commitIndex++) { + store.profilerStore.profilingCache.getCommitTree({ + commitIndex, + rootID, + }); + } + }); + }); }); diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index 5e39a0fe68f9..871b44827e77 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -3572,4 +3572,48 @@ describe('Store', () => { `); }); + + // @reactVersion >= 19 + it('can reconcile newly visible Activity with filtered, stable children', async () => { + const Activity = React.Activity || React.unstable_Activity; + + function IgnoreMe({children}) { + return children; + } + + function Component({children}) { + return
{children}
; + } + + await actAsync( + async () => + (store.componentFilters = [createDisplayNameFilter('^IgnoreMe', true)]), + ); + + const children = ( + + Left + + ); + + await actAsync(() => { + render({children}); + }); + + expect(store).toMatchInlineSnapshot(` + [root] + + `); + + await actAsync(() => { + render({children}); + }); + + expect(store).toMatchInlineSnapshot(` + [root] + ▾ + ▾ +
+ `); + }); }); diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index bc1b2a590d59..3214c8392285 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -2088,6 +2088,10 @@ export function attach( return changedKeys; } + /** + * Returns true iff nextFiber actually performed any work and produced an update. + * For generic components, like Function or Class components, prevFiber is not considered. + */ function didFiberRender(prevFiber: Fiber, nextFiber: Fiber): boolean { switch (nextFiber.tag) { case ClassComponent: @@ -4520,7 +4524,10 @@ export function attach( pushOperation(convertedTreeBaseDuration); } - if (prevFiber == null || didFiberRender(prevFiber, fiber)) { + if ( + prevFiber == null || + (prevFiber !== fiber && didFiberRender(prevFiber, fiber)) + ) { if (actualDuration != null) { // The actual duration reported by React includes time spent working on children. // This is useful information, but it's also useful to be able to exclude child durations. @@ -5150,11 +5157,13 @@ export function attach( elementType === ElementTypeMemo || elementType === ElementTypeForwardRef ) { - // Otherwise if this is a traced ancestor, flag for the nearest host descendant(s). - traceNearestHostComponentUpdate = didFiberRender( - prevFiber, - nextFiber, - ); + if (prevFiber !== nextFiber) { + // Otherwise if this is a traced ancestor, flag for the nearest host descendant(s). + traceNearestHostComponentUpdate = didFiberRender( + prevFiber, + nextFiber, + ); + } } } } @@ -5174,18 +5183,20 @@ export function attach( previousSuspendedBy = fiberInstance.suspendedBy; // Update the Fiber so we that we always keep the current Fiber on the data. fiberInstance.data = nextFiber; - if ( - mostRecentlyInspectedElement !== null && - (mostRecentlyInspectedElement.id === fiberInstance.id || - // If we're inspecting a Root, we inspect the Screen. - // Invalidating any Root invalidates the Screen too. - (mostRecentlyInspectedElement.type === ElementTypeRoot && - nextFiber.tag === HostRoot)) && - didFiberRender(prevFiber, nextFiber) - ) { - // If this Fiber has updated, clear cached inspected data. - // If it is inspected again, it may need to be re-run to obtain updated hooks values. - hasElementUpdatedSinceLastInspected = true; + if (prevFiber !== nextFiber) { + if ( + mostRecentlyInspectedElement !== null && + (mostRecentlyInspectedElement.id === fiberInstance.id || + // If we're inspecting a Root, we inspect the Screen. + // Invalidating any Root invalidates the Screen too. + (mostRecentlyInspectedElement.type === ElementTypeRoot && + nextFiber.tag === HostRoot)) && + didFiberRender(prevFiber, nextFiber) + ) { + // If this Fiber has updated, clear cached inspected data. + // If it is inspected again, it may need to be re-run to obtain updated hooks values. + hasElementUpdatedSinceLastInspected = true; + } } // Push a new DevTools instance parent while reconciling this subtree. reconcilingParent = fiberInstance; @@ -5405,6 +5416,17 @@ export function attach( // We're hiding the children. Remove them from the Frontend unmountRemainingChildren(); } + } else if (prevWasHidden && !nextIsHidden) { + // Since we don't mount hidden children and unmount children when hiding, + // we need to enter the mount path when revealing. + const nextChildSet = nextFiber.child; + if (nextChildSet !== null) { + mountChildrenRecursively( + nextChildSet, + traceNearestHostComponentUpdate, + ); + updateFlags |= ShouldResetChildren | ShouldResetSuspenseChildren; + } } else if ( nextFiber.tag === SuspenseComponent && OffscreenComponent !== -1 && @@ -5559,10 +5581,12 @@ export function attach( } recordConsoleLogs(fiberInstance, componentLogsEntry); - const isProfilingSupported = - nextFiber.hasOwnProperty('treeBaseDuration'); - if (isProfilingSupported) { - recordProfilingDurations(fiberInstance, prevFiber); + if (!isInDisconnectedSubtree) { + const isProfilingSupported = + nextFiber.hasOwnProperty('treeBaseDuration'); + if (isProfilingSupported) { + recordProfilingDurations(fiberInstance, prevFiber); + } } } } @@ -7852,17 +7876,20 @@ export function attach( } break; case 'props': - if (instance === null) { - if (typeof overridePropsRenamePath === 'function') { - overridePropsRenamePath(fiber, oldPath, newPath); - } - } else { - fiber.pendingProps = copyWithRename( - instance.props, - oldPath, - newPath, - ); - instance.forceUpdate(); + switch (fiber.tag) { + case ClassComponent: + fiber.pendingProps = copyWithRename( + instance.props, + oldPath, + newPath, + ); + instance.forceUpdate(); + break; + default: + if (typeof overridePropsRenamePath === 'function') { + overridePropsRenamePath(fiber, oldPath, newPath); + } + break; } break; case 'state':