[Fiber] Schedule animation regardless of deferred work (#8187)
* Remove comments about implementation details * Schedule animation regardless of deferred work
Dan Abramov committed
Nov 2, 2016 at 20:46 UTC
c894679b75d603497530dba7d04deb1ed9df2d0e
3 files changed
+65
-132
scripts/fiber/tests-passing.txt
+7
-7
@@ -808,12 +808,12 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
808
* flushes scheduled deferred work fitting within deadline for many roots
809
* schedules more deferred work if it runs out of time
810
* schedules more deferred work if it runs out of time with many roots
811
-* FIXME: flushes late animation work in a deferred callback if it wins
812
-* FIXME: flushes late animation work in a deferred callback if it wins with many roots
813
-* FIXME: ignores late animation work in an animation callback if it wins
814
-* FIXME: ignores late animation work in an animation callback if it wins with many roots
815
-* FIXME: ignores all work in a deferred callback if it wins
816
-* FIXME: ignores all work in a deferred callback if it wins with many roots
811
+* flushes late animation work in a deferred callback if it wins
812
+* flushes late animation work in a deferred callback if it wins with many roots
813
+* flushes late animation work in an animation callback if it wins
814
+* flushes late animation work in an animation callback if it wins with many roots
815
+* flushes all work in a deferred callback if it wins
816
+* flushes all work in a deferred callback if it wins with many roots
817
* flushes root with late deferred work in an animation callback if it wins
818
* flushes all roots with animation work in an animation callback if it wins
819
* splits deferred work on multiple roots
@@ -828,7 +828,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
828
* performs animated work in deferred callback if it has time
829
* performs animated work and deferred work in deferred callback if it has time
830
* performs deferred and animated work work in deferred callback if it has time
831
-* FIXME: schedules animated work as if it was deferred in deferred callback if it runs out of time
831
+* schedules animated work in deferred callback if it runs out of time
832
* schedules animated work and deferred work in deferred callback if it runs out of time
833
* schedules deferred work and animated work in deferred callback if it runs out of time
834
src/renderers/shared/fiber/ReactFiberScheduler.js
+49
-28
@@ -82,6 +82,10 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
82
let nextScheduledRoot : ?FiberRoot = null;
83
let lastScheduledRoot : ?FiberRoot = null;
84
85
+ // Keep track of which host environment callbacks are scheduled
86
+ let isAnimationCallbackScheduled : boolean = false;
87
+ let isDeferredCallbackScheduled : boolean = false;
88
+
89
function findNextUnitOfWork() {
90
// Clear out roots with no more work on them.
91
while (nextScheduledRoot && nextScheduledRoot.current.pendingWorkPriority === NoWork) {
@@ -379,13 +383,17 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
383
nextUnitOfWork = findNextUnitOfWork();
384
}
385
} else {
382
- scheduleDeferredCallback(performDeferredWork);
386
+ if (!isDeferredCallbackScheduled) {
387
+ isDeferredCallbackScheduled = true;
388
+ scheduleDeferredCallback(performDeferredWork);
389
+ }
390
return;
391
}
392
}
393
}
394
395
function performDeferredWork(deadline) {
396
+ isDeferredCallbackScheduled = false;
397
performAndHandleErrors(LowPriority, deadline);
398
}
399
@@ -403,20 +411,21 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
411
root.current.pendingWorkPriority = priority;
412
}
413
406
- if (root.isScheduled) {
407
- // If we're already scheduled, we can bail out.
408
- return;
414
+ if (!root.isScheduled) {
415
+ root.isScheduled = true;
416
+ if (lastScheduledRoot) {
417
+ // Schedule ourselves to the end.
418
+ lastScheduledRoot.nextScheduledRoot = root;
419
+ lastScheduledRoot = root;
420
+ } else {
421
+ // We're the only work scheduled.
422
+ nextScheduledRoot = root;
423
+ lastScheduledRoot = root;
424
+ }
425
}
410
- root.isScheduled = true;
411
- if (lastScheduledRoot) {
412
- // Schedule ourselves to the end.
413
- lastScheduledRoot.nextScheduledRoot = root;
414
- lastScheduledRoot = root;
415
- } else {
416
- // We're the only work scheduled.
417
- nextScheduledRoot = root;
418
- lastScheduledRoot = root;
426
427
+ if (!isDeferredCallbackScheduled) {
428
+ isDeferredCallbackScheduled = true;
429
scheduleDeferredCallback(performDeferredWork);
430
}
431
}
@@ -434,11 +443,15 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
443
}
444
}
445
if (nextUnitOfWork && nextPriorityLevel > AnimationPriority) {
437
- scheduleDeferredCallback(performDeferredWork);
446
+ if (!isDeferredCallbackScheduled) {
447
+ isDeferredCallbackScheduled = true;
448
+ scheduleDeferredCallback(performDeferredWork);
449
+ }
450
}
451
}
452
453
function performAnimationWork() {
454
+ isAnimationCallbackScheduled = false;
455
performAndHandleErrors(AnimationPriority);
456
}
457
@@ -449,19 +462,21 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
462
root.current.pendingWorkPriority = priorityLevel;
463
}
464
452
- if (root.isScheduled) {
453
- // If we're already scheduled, we can bail out.
454
- return;
465
+ if (!root.isScheduled) {
466
+ root.isScheduled = true;
467
+ if (lastScheduledRoot) {
468
+ // Schedule ourselves to the end.
469
+ lastScheduledRoot.nextScheduledRoot = root;
470
+ lastScheduledRoot = root;
471
+ } else {
472
+ // We're the only work scheduled.
473
+ nextScheduledRoot = root;
474
+ lastScheduledRoot = root;
475
+ }
476
}
456
- root.isScheduled = true;
457
- if (lastScheduledRoot) {
458
- // Schedule ourselves to the end.
459
- lastScheduledRoot.nextScheduledRoot = root;
460
- lastScheduledRoot = root;
461
- } else {
462
- // We're the only work scheduled.
463
- nextScheduledRoot = root;
464
- lastScheduledRoot = root;
477
+
478
+ if (!isAnimationCallbackScheduled) {
479
+ isAnimationCallbackScheduled = true;
480
scheduleAnimationCallback(performAnimationWork);
481
}
482
}
@@ -505,10 +520,16 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
520
}
521
if (nextUnitOfWork) {
522
if (nextPriorityLevel > AnimationPriority) {
508
- scheduleDeferredCallback(performDeferredWork);
523
+ if (!isDeferredCallbackScheduled) {
524
+ isDeferredCallbackScheduled = true;
525
+ scheduleDeferredCallback(performDeferredWork);
526
+ }
527
return;
528
}
511
- scheduleAnimationCallback(performAnimationWork);
529
+ if (!isAnimationCallbackScheduled) {
530
+ isAnimationCallbackScheduled = true;
531
+ scheduleAnimationCallback(performAnimationWork);
532
+ }
533
}
534
}
535
src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
+9
-97
@@ -175,27 +175,22 @@ describe('ReactIncrementalScheduling', () => {
175
expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
176
});
177
178
- it('FIXME: flushes late animation work in a deferred callback if it wins', () => {
178
+ it('flushes late animation work in a deferred callback if it wins', () => {
179
// Schedule early deferred
180
ReactNoop.render(<span prop="1" />);
181
// Schedule late animation
182
ReactNoop.performAnimationWork(() => {
183
ReactNoop.render(<span prop="2" />);
184
});
185
-
186
- // FIXME: I would expect true here because we should have scheduled an animation callback:
187
- // expect(ReactNoop.hasScheduledAnimationCallback()).toBe(true);
185
expect(ReactNoop.getChildren()).toEqual([]);
186
187
// We only scheduled deferred callback so that's what we get.
188
// It will flush everything.
189
ReactNoop.flushDeferredPri();
193
- // TODO: I would expect true here because we should have tried to schedule animation sooner:
194
- // expect(ReactNoop.hasScheduledAnimationCallback()).toBe(true);
190
expect(ReactNoop.getChildren()).toEqual([span('2')]);
191
});
192
198
- it('FIXME: flushes late animation work in a deferred callback if it wins with many roots', () => {
193
+ it('flushes late animation work in a deferred callback if it wins with many roots', () => {
194
// Schedule early deferred
195
ReactNoop.renderToRootWithID(<span prop="a:1" />, 'a');
196
ReactNoop.renderToRootWithID(<span prop="b:1" />, 'b');
@@ -204,9 +199,6 @@ describe('ReactIncrementalScheduling', () => {
199
ReactNoop.renderToRootWithID(<span prop="b:2" />, 'b');
200
ReactNoop.renderToRootWithID(<span prop="c:2" />, 'c');
201
});
207
-
208
- // FIXME: I would expect true here because we should have scheduled an animation callback:
209
- // expect(ReactNoop.hasScheduledAnimationCallback()).toBe(true);
202
expect(ReactNoop.getChildren('a')).toEqual([]);
203
expect(ReactNoop.getChildren('b')).toEqual([]);
204
expect(ReactNoop.getChildren('c')).toEqual([]);
@@ -214,43 +206,26 @@ describe('ReactIncrementalScheduling', () => {
206
// We only scheduled deferred callback so that's what we get.
207
// It will flush everything.
208
ReactNoop.flushDeferredPri();
217
- // TODO: I would expect true here because we tried to schedule animation sooner, and so it would still be scheduled:
218
- // expect(ReactNoop.hasScheduledAnimationCallback()).toBe(true);
209
expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
210
expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
211
expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
212
});
213
224
- // This test documents the existing behavior. Desired behavior:
225
- // it('flushes late animation work in an animation callback if it wins', () => {
226
- it('FIXME: ignores late animation work in an animation callback if it wins', () => {
214
+ it('flushes late animation work in an animation callback if it wins', () => {
215
// Schedule early deferred
216
ReactNoop.render(<span prop="1" />);
217
// Schedule late animation
218
ReactNoop.performAnimationWork(() => {
219
ReactNoop.render(<span prop="2" />);
220
});
233
-
234
- // FIXME: I would expect true here because we should have scheduled an animation callback:
235
- // expect(ReactNoop.hasScheduledAnimationCallback()).toBe(true);
221
expect(ReactNoop.getChildren()).toEqual([]);
222
223
// Flushing animation should have flushed the animation.
239
- // FIXME: But it currently doesn't.
240
- // Technically we didn't schedule this callback. This test is valid because we should have scheduled it.
224
ReactNoop.flushAnimationPri();
242
- // TODO: I would expect [span('2')] here because it was scheduled as animation:
243
- // expect(ReactNoop.getChildren()).toEqual([span('2')]);
244
- expect(ReactNoop.getChildren()).toEqual([]);
245
-
246
- // TODO: I wouldn't expect this flush to be necessary for animation work to kick in.
247
- ReactNoop.flushDeferredPri();
225
expect(ReactNoop.getChildren()).toEqual([span('2')]);
226
});
227
251
- // This test documents the existing behavior. Desired behavior:
252
- // it('flushes late animation work in an animation callback if it wins with many roots', () => {
253
- it('FIXME: ignores late animation work in an animation callback if it wins with many roots', () => {
228
+ it('flushes late animation work in an animation callback if it wins with many roots', () => {
229
// Schedule early deferred
230
ReactNoop.renderToRootWithID(<span prop="a:1" />, 'a');
231
ReactNoop.renderToRootWithID(<span prop="b:1" />, 'b');
@@ -259,61 +234,38 @@ describe('ReactIncrementalScheduling', () => {
234
ReactNoop.renderToRootWithID(<span prop="b:2" />, 'b');
235
ReactNoop.renderToRootWithID(<span prop="c:2" />, 'c');
236
});
262
-
263
- // FIXME: I would expect true here because we should have scheduled an animation callback:
264
- // expect(ReactNoop.hasScheduledAnimationCallback()).toBe(true);
237
expect(ReactNoop.getChildren('a')).toEqual([]);
238
expect(ReactNoop.getChildren('b')).toEqual([]);
239
expect(ReactNoop.getChildren('c')).toEqual([]);
240
241
// Flushing animation should have flushed the animation.
270
- // FIXME: But it currently doesn't.
271
- // Technically we didn't schedule this callback. This test is valid because we should have scheduled it.
242
ReactNoop.flushAnimationPri();
273
- // TODO: I would expect this here because they were scheduled as animation:
274
- // expect(ReactNoop.getChildren('a')).toEqual([]);
275
- // expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
276
- // expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
243
expect(ReactNoop.getChildren('a')).toEqual([]);
278
- expect(ReactNoop.getChildren('b')).toEqual([]);
279
- expect(ReactNoop.getChildren('c')).toEqual([]);
244
+ expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
245
+ expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
246
281
- // TODO: I wouldn't expect this flush to be necessary for animation work to kick in.
247
ReactNoop.flushDeferredPri();
248
expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
249
expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
250
expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
251
});
252
288
- // This test documents the existing behavior. Desired behavior:
289
- // it('flushes all work in a deferred callback if it wins', () => {
290
- it('FIXME: ignores all work in a deferred callback if it wins', () => {
253
+ it('flushes all work in a deferred callback if it wins', () => {
254
// Schedule early animation
255
ReactNoop.performAnimationWork(() => {
256
ReactNoop.render(<span prop="1" />);
257
});
258
// Schedule late deferred
259
ReactNoop.render(<span prop="2" />);
297
- // We could also make this true because we know we have a deferred update coming.
260
expect(ReactNoop.getChildren()).toEqual([]);
261
262
// Flushing deferred should have flushed both early animation and late deferred work that invalidated it.
263
// This is not a common case, as animation should generally be flushed before deferred work.
302
- // FIXME: Instead, it currently doesn't do anything.
303
- // TODO: is this even a valid test? Technically we didn't schedule this callback.
264
ReactNoop.flushDeferredPri();
305
- // TODO: I would expect [span('2')] here because we should have flushed everything by now:
306
- // expect(ReactNoop.getChildren()).toEqual([span('2')]);
307
- expect(ReactNoop.getChildren()).toEqual([]);
308
-
309
- // Flushing animation should have been a no-op
310
- ReactNoop.flushAnimationPri();
265
expect(ReactNoop.getChildren()).toEqual([span('2')]);
266
});
267
314
- // This test documents the existing behavior. Desired behavior:
315
- // it('flushes all work in a deferred callback if it wins with many roots', () => {
316
- it('FIXME: ignores all work in a deferred callback if it wins with many roots', () => {
268
+ it('flushes all work in a deferred callback if it wins with many roots', () => {
269
// Schedule early animation
270
ReactNoop.performAnimationWork(() => {
271
ReactNoop.renderToRootWithID(<span prop="a:1" />, 'a');
@@ -322,36 +274,15 @@ describe('ReactIncrementalScheduling', () => {
274
// Schedule late deferred
275
ReactNoop.renderToRootWithID(<span prop="b:2" />, 'b');
276
ReactNoop.renderToRootWithID(<span prop="c:2" />, 'c');
325
- // We could also make this true because we know we have a deferred update coming.
277
expect(ReactNoop.getChildren('a')).toEqual([]);
278
expect(ReactNoop.getChildren('b')).toEqual([]);
279
expect(ReactNoop.getChildren('c')).toEqual([]);
280
281
// Flushing deferred should have flushed both early animation and late deferred work that invalidated it.
282
// This is not a common case, as animation should generally be flushed before deferred work.
332
- // FIXME: Instead, it currently doesn't do anything.
333
- // TODO: is this even a valid test? Technically we didn't schedule this callback.
283
ReactNoop.flushDeferredPri();
335
- // TODO: I would expect this here because we should have flushed everything:
336
- // expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
337
- // expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
338
- // expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
339
- expect(ReactNoop.getChildren('a')).toEqual([]);
340
- expect(ReactNoop.getChildren('b')).toEqual([]);
341
- expect(ReactNoop.getChildren('c')).toEqual([]);
342
-
343
- // Flushing animation should have been a no-op.
344
- ReactNoop.flushAnimationPri();
345
- // FIXME: this deferred work should have been handled earlier.
346
- // expect(ReactNoop.hasScheduledDeferredCallback()).toBe(false);
284
expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
285
expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
349
- // FIXME: this deferred work should have been handled earlier.
350
- // expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
351
- expect(ReactNoop.getChildren('c')).toEqual([]);
352
-
353
- // FIXME: it should be unnecessary to flush one more time:
354
- ReactNoop.flushDeferredPri();
286
expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]);
287
});
288
@@ -362,7 +293,6 @@ describe('ReactIncrementalScheduling', () => {
293
});
294
// Schedule late deferred
295
ReactNoop.render(<span prop="2" />);
365
- // We don't need a deferred callback because the root already has animation work
296
expect(ReactNoop.getChildren()).toEqual([]);
297
298
// Flushing animation work flushes everything on this root.
@@ -379,14 +309,12 @@ describe('ReactIncrementalScheduling', () => {
309
// Schedule late deferred
310
ReactNoop.renderToRootWithID(<span prop="b:2" />, 'b');
311
ReactNoop.renderToRootWithID(<span prop="c:2" />, 'c');
382
- // TODO: I would expect this to be true because we know we have a deferred update coming.
312
expect(ReactNoop.getChildren('a')).toEqual([]);
313
expect(ReactNoop.getChildren('b')).toEqual([]);
314
expect(ReactNoop.getChildren('c')).toEqual([]);
315
316
// Flushing animation work flushes all roots with animation work.
317
ReactNoop.flushAnimationPri();
389
- // Now we know we also need a deferred update. Should we have scheduled it earlier?
318
expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
319
expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]);
320
expect(ReactNoop.getChildren('c')).toEqual([]);
@@ -783,9 +711,7 @@ describe('ReactIncrementalScheduling', () => {
711
expect(ReactNoop.getChildren('d')).toEqual([span('d:1')]);
712
});
713
786
- // This test documents the existing behavior. Desired behavior:
787
- // it('schedules animated work in deferred callback if it runs out of time', () => {
788
- it('FIXME: schedules animated work as if it was deferred in deferred callback if it runs out of time', () => {
714
+ it('schedules animated work in deferred callback if it runs out of time', () => {
715
let hasScheduled = false;
716
class Foo extends React.Component {
717
componentDidMount() {
@@ -808,23 +734,9 @@ describe('ReactIncrementalScheduling', () => {
734
expect(hasScheduled).toBe(true);
735
expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
736
expect(ReactNoop.getChildren('b')).toEqual([]);
811
- // FIXME: I would expect these to be flipped because animation work was scheduled.
812
- // expect(ReactNoop.hasScheduledAnimationCallback()).toBe(true);
813
- // expect(ReactNoop.hasScheduledDeferredCallback()).toBe(false);
737
738
// Flush the rest of work in an animated callback.
816
- // Technically we didn't schedule this callback. This test is valid because we should have scheduled it.
739
ReactNoop.flushAnimationPri();
818
- // FIXME: we should have flushed everything here.
819
- // expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
820
- // expect(ReactNoop.getChildren('b')).toEqual([span('b:1')]);
821
- // expect(ReactNoop.hasScheduledAnimationCallback()).toBe(false);
822
- // expect(ReactNoop.hasScheduledDeferredCallback()).toBe(false);
823
- expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
824
- expect(ReactNoop.getChildren('b')).toEqual([]);
825
-
826
- // FIXME: it should be unnecessary to flush the deferred work:
827
- ReactNoop.flushDeferredPri();
740
expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]);
741
expect(ReactNoop.getChildren('b')).toEqual([span('b:1')]);
742
});