Skip to content

Commit 2a955dd

Browse files
author
Brian Vaughn
committed
Fixed invalid treeBaseDuration for hidden, suspended tree Fragment wrapper
1 parent 595b4f9 commit 2a955dd

File tree

2 files changed

+67
-4
lines changed

2 files changed

+67
-4
lines changed

packages/react-reconciler/src/ReactFiberBeginWork.js

+13-4
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ import {
6666
} from './ReactChildFiber';
6767
import {processUpdateQueue} from './ReactUpdateQueue';
6868
import {NoWork, Never} from './ReactFiberExpirationTime';
69-
import {ConcurrentMode, StrictMode} from './ReactTypeOfMode';
69+
import {ConcurrentMode, ProfileMode, StrictMode} from './ReactTypeOfMode';
7070
import {
7171
shouldSetTextContent,
7272
shouldDeprioritizeSubtree,
@@ -753,7 +753,7 @@ function mountLazyComponent(
753753
) {
754754
if (_current !== null) {
755755
// An lazy component only mounts if it suspended inside a non-
756-
// concurrent tree, in an inconsistent state. We want to tree it like
756+
// concurrent tree, in an inconsistent state. We want to treat it like
757757
// a new mount, even though an empty version of it already committed.
758758
// Disconnect the alternate pointers.
759759
_current.alternate = null;
@@ -839,7 +839,7 @@ function mountIncompleteClassComponent(
839839
) {
840840
if (_current !== null) {
841841
// An incomplete component only mounts if it suspended inside a non-
842-
// concurrent tree, in an inconsistent state. We want to tree it like
842+
// concurrent tree, in an inconsistent state. We want to treat it like
843843
// a new mount, even though an empty version of it already committed.
844844
// Disconnect the alternate pointers.
845845
_current.alternate = null;
@@ -896,7 +896,7 @@ function mountIndeterminateComponent(
896896
) {
897897
if (_current !== null) {
898898
// An indeterminate component only mounts if it suspended inside a non-
899-
// concurrent tree, in an inconsistent state. We want to tree it like
899+
// concurrent tree, in an inconsistent state. We want to treat it like
900900
// a new mount, even though an empty version of it already committed.
901901
// Disconnect the alternate pointers.
902902
_current.alternate = null;
@@ -1234,6 +1234,15 @@ function updateSuspenseComponent(
12341234
NoWork,
12351235
null,
12361236
);
1237+
1238+
if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
1239+
// Fiber treeBaseDuration must be at least as large as the sum of children treeBaseDurations.
1240+
// Otherwise the profiler's onRender metrics will be off,
1241+
// and the DevTools Profiler flamegraph will visually break as well.
1242+
primaryChildFragment.treeBaseDuration =
1243+
currentPrimaryChild.treeBaseDuration;
1244+
}
1245+
12371246
primaryChildFragment.effectTag |= Placement;
12381247
primaryChildFragment.child = currentPrimaryChild;
12391248
currentPrimaryChild.return = primaryChildFragment;

packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js

+54
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ runPlaceholderTests('ReactSuspensePlaceholder (persistence)', () =>
1616
);
1717

1818
function runPlaceholderTests(suiteLabel, loadReactNoop) {
19+
let advanceTimeBy;
20+
let mockNow;
21+
let Profiler;
1922
let React;
2023
let ReactTestRenderer;
2124
let ReactFeatureFlags;
@@ -27,13 +30,24 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) {
2730
describe(suiteLabel, () => {
2831
beforeEach(() => {
2932
jest.resetModules();
33+
34+
let currentTime = 0;
35+
mockNow = jest.fn().mockImplementation(() => currentTime);
36+
global.Date.now = mockNow;
37+
advanceTimeBy = amount => {
38+
currentTime += amount;
39+
};
40+
3041
ReactFeatureFlags = require('shared/ReactFeatureFlags');
3142
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
43+
ReactFeatureFlags.enableProfilerTimer = true;
3244
ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
3345
React = require('react');
3446
ReactTestRenderer = require('react-test-renderer');
47+
ReactTestRenderer.unstable_setNowImplementation(mockNow);
3548
ReactCache = require('react-cache');
3649

50+
Profiler = React.unstable_Profiler;
3751
Suspense = React.Suspense;
3852

3953
TextResource = ReactCache.unstable_createResource(([text, ms = 0]) => {
@@ -215,5 +229,45 @@ function runPlaceholderTests(suiteLabel, loadReactNoop) {
215229
// given a `hidden` prop.
216230
expect(root).toMatchRenderedOutput('AB2C');
217231
});
232+
233+
it('properly accounts for profiler base times when showing the fallback UI', () => {
234+
const onRender = jest.fn();
235+
236+
const Fallback = () => {
237+
advanceTimeBy(3);
238+
return 'Loading...';
239+
};
240+
241+
const Suspending = () => {
242+
advanceTimeBy(2);
243+
return <AsyncText ms={1000} text="Loaded" />;
244+
};
245+
246+
function App() {
247+
return (
248+
<Profiler id="root" onRender={onRender}>
249+
<Suspense maxDuration={500} fallback={<Fallback />}>
250+
<Suspending />
251+
</Suspense>
252+
</Profiler>
253+
);
254+
}
255+
256+
// Initial mount
257+
const root = ReactTestRenderer.create(<App />);
258+
expect(root.toJSON()).toEqual('Loading...');
259+
expect(onRender).toHaveBeenCalledTimes(2);
260+
expect(onRender.mock.calls[0][3]).toBe(2);
261+
262+
// The fallback commit treeBaseTime should include both the
263+
// 2ms spent in Suspending and the 3ms spent in Fallback.
264+
expect(onRender.mock.calls[1][3]).toBe(5);
265+
266+
jest.advanceTimersByTime(1000);
267+
268+
expect(root.toJSON()).toEqual('Loaded');
269+
expect(onRender).toHaveBeenCalledTimes(3);
270+
expect(onRender.mock.calls[2][3]).toBe(2);
271+
});
218272
});
219273
}

0 commit comments

Comments
 (0)