Skip to content

Commit 1e7100e

Browse files
committed
Process completion callbacks immediately after completing a root
We need to process completion callbacks in two places. The first is intuitive: right after a root completes. It might seem like that is sufficient. But if a completion callback is scheduled on an already completed root, it's possible we won't complete that root again. So we also need to process completion callbacks whenever we skip over an already completed root.
1 parent 3904f31 commit 1e7100e

File tree

2 files changed

+93
-36
lines changed

2 files changed

+93
-36
lines changed

src/renderers/shared/fiber/ReactFiberScheduler.js

Lines changed: 35 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -399,33 +399,10 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
399399
// no longer blocked, return the time at which it completed so that we
400400
// can commit it.
401401
if (isRootBlocked(root, completedAt)) {
402-
// Process pending completion callbacks so that they are called at
403-
// the end of the current batch.
404-
const completionCallbacks = root.completionCallbacks;
405-
if (completionCallbacks !== null) {
406-
processUpdateQueue(
407-
completionCallbacks,
408-
null,
409-
null,
410-
null,
411-
completedAt,
412-
);
413-
const callbackList = completionCallbacks.callbackList;
414-
if (callbackList !== null) {
415-
// Add new callbacks to list of completion callbacks
416-
if (rootCompletionCallbackList === null) {
417-
rootCompletionCallbackList = callbackList;
418-
} else {
419-
for (let i = 0; i < callbackList.length; i++) {
420-
rootCompletionCallbackList.push(callbackList[i]);
421-
}
422-
}
423-
completionCallbacks.callbackList = null;
424-
if (completionCallbacks.first === null) {
425-
root.completionCallbacks = null;
426-
}
427-
}
428-
}
402+
// We usually process completion callbacks right after a root is
403+
// completed. But this root already completed, and it's possible that
404+
// we received new completion callbacks since then.
405+
processCompletionCallbacks(root, completedAt);
429406
return Done;
430407
}
431408

@@ -435,6 +412,33 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
435412
return expirationTime;
436413
}
437414

415+
function processCompletionCallbacks(
416+
root: FiberRoot,
417+
completedAt: ExpirationTime,
418+
) {
419+
// Process pending completion callbacks so that they are called at
420+
// the end of the current batch.
421+
const completionCallbacks = root.completionCallbacks;
422+
if (completionCallbacks !== null) {
423+
processUpdateQueue(completionCallbacks, null, null, null, completedAt);
424+
const callbackList = completionCallbacks.callbackList;
425+
if (callbackList !== null) {
426+
// Add new callbacks to list of completion callbacks
427+
if (rootCompletionCallbackList === null) {
428+
rootCompletionCallbackList = callbackList;
429+
} else {
430+
for (let i = 0; i < callbackList.length; i++) {
431+
rootCompletionCallbackList.push(callbackList[i]);
432+
}
433+
}
434+
completionCallbacks.callbackList = null;
435+
if (completionCallbacks.first === null) {
436+
root.completionCallbacks = null;
437+
}
438+
}
439+
}
440+
}
441+
438442
function commitAllHostEffects() {
439443
while (nextEffect !== null) {
440444
if (__DEV__) {
@@ -820,6 +824,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
820824
// The root is not blocked, so we can commit it now.
821825
pendingCommit = workInProgress;
822826
}
827+
processCompletionCallbacks(root, nextRenderExpirationTime);
823828
return null;
824829
}
825830
}
@@ -1602,12 +1607,9 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
16021607
}
16031608
break;
16041609
case TaskPriority:
1605-
invariant(
1606-
isBatchingUpdates,
1607-
'Task updates can only be scheduled as a nested update or ' +
1608-
'inside batchedUpdates. This error is likely caused by a ' +
1609-
'bug in React. Please file an issue.',
1610-
);
1610+
if (!isPerformingWork && !isBatchingUpdates) {
1611+
performWork(TaskPriority, null);
1612+
}
16111613
break;
16121614
default:
16131615
// This update is async. Schedule a callback.

src/renderers/shared/fiber/__tests__/ReactIncrementalRoot-test.js

Lines changed: 58 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,62 @@ describe('ReactIncrementalRoot', () => {
9595
expect(root.getChildren()).toEqual([]);
9696
});
9797

98-
it(
99-
'does not work on on a blocked tree if the expiration time is greater than the blocked update',
100-
);
98+
it('does not work on on a blocked tree if the expiration time is greater than the blocked update', () => {
99+
let ops = [];
100+
function Foo(props) {
101+
ops.push('Foo: ' + props.children);
102+
return <span prop={props.children} />;
103+
}
104+
const root = ReactNoop.createRoot();
105+
root.prerender(<Foo>A</Foo>);
106+
ReactNoop.flush();
107+
108+
expect(ops).toEqual(['Foo: A']);
109+
expect(root.getChildren()).toEqual([]);
110+
111+
// workB has a later expiration time
112+
ReactNoop.expire(1000);
113+
root.prerender(<Foo>B</Foo>);
114+
ReactNoop.flush();
115+
116+
// Should not have re-rendered the root at the later expiration time
117+
expect(ops).toEqual(['Foo: A']);
118+
expect(root.getChildren()).toEqual([]);
119+
});
120+
121+
it('commits earlier work without committing later work', () => {
122+
const root = ReactNoop.createRoot();
123+
const work1 = root.prerender(<span prop="A" />);
124+
ReactNoop.flush();
125+
126+
expect(root.getChildren()).toEqual([]);
127+
128+
// Second prerender has a later expiration time
129+
ReactNoop.expire(1000);
130+
root.prerender(<span prop="B" />);
131+
132+
work1.commit();
133+
134+
// Should not have re-rendered the root at the later expiration time
135+
expect(root.getChildren()).toEqual([span('A')]);
136+
});
137+
138+
it('flushes ealier work if later work is committed', () => {
139+
let ops = [];
140+
const root = ReactNoop.createRoot();
141+
const work1 = root.prerender(<span prop="A" />);
142+
// Second prerender has a later expiration time
143+
ReactNoop.expire(1000);
144+
const work2 = root.prerender(<span prop="B" />);
145+
146+
work1.then(() => ops.push('complete 1'));
147+
work2.then(() => ops.push('complete 2'));
148+
149+
work2.commit();
150+
151+
// Because the later prerender was committed, the earlier one should have
152+
// committed, too.
153+
expect(root.getChildren()).toEqual([span('B')]);
154+
expect(ops).toEqual(['complete 1', 'complete 2']);
155+
});
101156
});

0 commit comments

Comments
 (0)