From 9af69eac99001f22be91b59614350ad7438a6f00 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 2 Nov 2018 14:27:14 -0700 Subject: [PATCH] Hacky fix for actualDuration in a suspended concurrent tree --- .../src/ReactFiberScheduler.js | 10 ++ .../ReactSuspensePlaceholder-test.internal.js | 128 ++++++++++++------ 2 files changed, 97 insertions(+), 41 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index c60b8dc5ab69a..bdb9dd1ed667c 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1286,6 +1286,16 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { // Record the time spent rendering before an error was thrown. // This avoids inaccurate Profiler durations in the case of a suspended render. stopProfilerTimerIfRunningAndRecordDelta(nextUnitOfWork, true); + + // HACK Also propagate actualDuration for the time spent in the fiber that errored. + // This avoids inaccurate Profiler durations in the case of a suspended render. + // This happens automatically for sync renders, because of resetChildExpirationTime. + if (nextUnitOfWork.mode & ConcurrentMode) { + const returnFiber = nextUnitOfWork.return; + if (returnFiber !== null) { + returnFiber.actualDuration += nextUnitOfWork.actualDuration; + } + } } if (__DEV__) { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js index 5dc6e57a263bc..5ed6b717200c9 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js @@ -230,56 +230,102 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) { expect(root).toMatchRenderedOutput('AB2C'); }); - it('properly accounts for base durations when a suspended times out', () => { - // Order of parameters: id, phase, actualDuration, treeBaseDuration - const onRender = jest.fn(); + describe('profiler durations', () => { + let App; + let Fallback; + let Suspending; + let onRender; + + beforeEach(() => { + // Order of parameters: id, phase, actualDuration, treeBaseDuration + onRender = jest.fn(); + + Fallback = () => { + ReactTestRenderer.unstable_yield('Fallback'); + advanceTimeBy(5); + return 'Loading...'; + }; - const Fallback = () => { - advanceTimeBy(5); - return 'Loading...'; - }; + Suspending = () => { + ReactTestRenderer.unstable_yield('Suspending'); + advanceTimeBy(2); + return ; + }; - const Suspending = () => { - advanceTimeBy(2); - return ; - }; + App = () => { + ReactTestRenderer.unstable_yield('App'); + return ( + + }> + + + + ); + }; + }); - function App() { - return ( - - }> - - - - ); - } + it('properly accounts for base durations when a suspended times out in a sync tree', () => { + const root = ReactTestRenderer.create(); + expect(root.toJSON()).toEqual('Loading...'); + expect(onRender).toHaveBeenCalledTimes(2); - // Initial mount - const root = ReactTestRenderer.create(); - expect(root.toJSON()).toEqual('Loading...'); - expect(onRender).toHaveBeenCalledTimes(2); + // Initial mount should be 3ms– + // 2ms from Suspending, and 1ms from the AsyncText it renders. + expect(onRender.mock.calls[0][2]).toBe(3); + expect(onRender.mock.calls[0][3]).toBe(3); - // Initial mount should be 3ms– - // 2ms from Suspending, and 1ms from the AsyncText it renders. - expect(onRender.mock.calls[0][2]).toBe(3); - expect(onRender.mock.calls[0][3]).toBe(3); + // When the fallback UI is displayed, and the origina UI hidden, + // the baseDuration should only include the 5ms spent rendering Fallback, + // but the treeBaseDuration should include that and the 3ms spent in Suspending. + expect(onRender.mock.calls[1][2]).toBe(5); + expect(onRender.mock.calls[1][3]).toBe(8); - // When the fallback UI is displayed, and the origina UI hidden, - // the baseDuration should only include the 5ms spent rendering Fallback, - // but the treeBaseDuration should include that and the 3ms spent in Suspending. - expect(onRender.mock.calls[1][2]).toBe(5); - expect(onRender.mock.calls[1][3]).toBe(8); + jest.advanceTimersByTime(1000); - jest.advanceTimersByTime(1000); + expect(root.toJSON()).toEqual('Loaded'); + expect(onRender).toHaveBeenCalledTimes(3); - expect(root.toJSON()).toEqual('Loaded'); - expect(onRender).toHaveBeenCalledTimes(3); + // When the suspending data is resolved and our final UI is rendered, + // the baseDuration should only include the 1ms re-rendering AsyncText, + // but the treeBaseDuration should include that and the 2ms spent rendering Suspending. + expect(onRender.mock.calls[2][2]).toBe(1); + expect(onRender.mock.calls[2][3]).toBe(3); + }); - // When the suspending data is resolved and our final UI is rendered, - // the baseDuration should only include the 1ms re-rendering AsyncText, - // but the treeBaseDuration should include that and the 2ms spent rendering Suspending. - expect(onRender.mock.calls[2][2]).toBe(1); - expect(onRender.mock.calls[2][3]).toBe(3); + it('properly accounts for base durations when a suspended times out in a concurrent tree', () => { + const root = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); + + expect(root).toFlushAndYield([ + 'App', + 'Suspending', + 'Suspend! [Loaded]', + 'Fallback', + ]); + expect(root).toMatchRenderedOutput(null); + + jest.advanceTimersByTime(1000); + + expect(ReactTestRenderer).toHaveYielded(['Promise resolved [Loaded]']); + expect(root).toMatchRenderedOutput('Loading...'); + expect(onRender).toHaveBeenCalledTimes(1); + + // Initial mount only shows the "Loading..." Fallback. + // The treeBaseDuration then should be 5ms spent rendering Fallback, + // but the actualDuration should include that and the 3ms spent in Suspending. + expect(onRender.mock.calls[0][2]).toBe(8); + expect(onRender.mock.calls[0][3]).toBe(5); + + expect(root).toFlushAndYield(['Suspending', 'Loaded']); + expect(root).toMatchRenderedOutput('Loaded'); + expect(onRender).toHaveBeenCalledTimes(2); + + // When the suspending data is resolved and our final UI is rendered, + // both times should include the 3ms re-rendering Suspending and AsyncText. + expect(onRender.mock.calls[1][2]).toBe(3); + expect(onRender.mock.calls[1][3]).toBe(3); + }); }); }); }