From a9fad187ddd859d96c9d70f0d980a7c864a2b5a7 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 27 Feb 2019 14:26:33 -0800 Subject: [PATCH 1/2] Replace noop's fake Scheduler implementation with mock Scheduler build The noop renderer has its own mock implementation of the Scheduler interface, with the ability to partially render work in tests. Now that this functionality has been lifted into a proper mock Scheduler build, we can use that instead. Most of the existing noop tests were unaffected, but I did have to make some changes. The biggest one involved passive effects: previously, they were scheduled on a separate queue from the queue that handles rendering. After this change, both rendering and effects are scheduled in the Scheduler queue. I think this is a better approach because tests no longer have to worry about the difference; if you call `flushAll`, all the work is flushed, both rendering and effects. But for those few tests that do care to flush the rendering without the effects, that's still possible using the `yieldValue` API. Follow-up: Do the same for test renderer. --- .../src/createReactNoop.js | 165 ++-------- .../ReactExpiration-test.internal.js | 67 +++-- ...eactHooksWithNoopRenderer-test.internal.js | 282 ++++++++++++------ .../ReactIncremental-test.internal.js | 4 +- ...tIncrementalErrorHandling-test.internal.js | 14 +- .../ReactIncrementalPerf-test.internal.js | 1 - ...eactIncrementalScheduling-test.internal.js | 33 -- .../ReactIncrementalTriangle-test.internal.js | 10 +- .../ReactIncrementalUpdates-test.internal.js | 240 +++++++++------ .../ReactNewContext-test.internal.js | 8 +- ...tSuspenseWithNoopRenderer-test.internal.js | 15 +- .../__tests__/ReactProfiler-test.internal.js | 15 +- scripts/jest/matchers/reactTestMatchers.js | 89 ++---- 13 files changed, 474 insertions(+), 469 deletions(-) diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 714141ccd18..a3871263fda 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -18,6 +18,7 @@ import type {Fiber} from 'react-reconciler/src/ReactFiber'; import type {UpdateQueue} from 'react-reconciler/src/ReactUpdateQueue'; import type {ReactNodeList} from 'shared/ReactTypes'; +import * as Scheduler from 'scheduler'; import {createPortal} from 'shared/ReactPortal'; import expect from 'expect'; import {REACT_FRAGMENT_TYPE, REACT_ELEMENT_TYPE} from 'shared/ReactSymbols'; @@ -51,9 +52,6 @@ if (__DEV__) { } function createReactNoop(reconciler: Function, useMutation: boolean) { - let scheduledCallback = null; - let scheduledCallbackTimeout = -1; - let scheduledPassiveCallback = null; let instanceCounter = 0; let hostDiffCounter = 0; let hostUpdateCounter = 0; @@ -218,8 +216,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { ); } - let elapsedTimeInMs = 0; - const sharedHostConfig = { getRootHostContext() { return NO_CONTEXT; @@ -308,66 +304,23 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return inst; }, - scheduleDeferredCallback(callback, options) { - if (scheduledCallback) { - throw new Error( - 'Scheduling a callback twice is excessive. Instead, keep track of ' + - 'whether the callback has already been scheduled.', - ); - } - scheduledCallback = callback; - if ( - typeof options === 'object' && - options !== null && - typeof options.timeout === 'number' - ) { - const newTimeout = options.timeout; - if ( - scheduledCallbackTimeout === -1 || - scheduledCallbackTimeout > newTimeout - ) { - scheduledCallbackTimeout = elapsedTimeInMs + newTimeout; - } - } - return 0; - }, - - cancelDeferredCallback() { - if (scheduledCallback === null) { - throw new Error('No callback is scheduled.'); - } - scheduledCallback = null; - scheduledCallbackTimeout = -1; - }, + scheduleDeferredCallback: Scheduler.unstable_scheduleCallback, + cancelDeferredCallback: Scheduler.unstable_cancelCallback, - shouldYield, + shouldYield: Scheduler.unstable_shouldYield, scheduleTimeout: setTimeout, cancelTimeout: clearTimeout, noTimeout: -1, - schedulePassiveEffects(callback) { - if (scheduledCallback) { - throw new Error( - 'Scheduling a callback twice is excessive. Instead, keep track of ' + - 'whether the callback has already been scheduled.', - ); - } - scheduledPassiveCallback = callback; - }, - cancelPassiveEffects() { - if (scheduledPassiveCallback === null) { - throw new Error('No passive effects callback is scheduled.'); - } - scheduledPassiveCallback = null; - }, + + schedulePassiveEffects: Scheduler.unstable_scheduleCallback, + cancelPassiveEffects: Scheduler.unstable_cancelCallback, prepareForCommit(): void {}, resetAfterCommit(): void {}, - now(): number { - return elapsedTimeInMs; - }, + now: Scheduler.unstable_now, isPrimaryRenderer: true, supportsHydration: false, @@ -534,71 +487,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { const roots = new Map(); const DEFAULT_ROOT_ID = ''; - let yieldedValues: Array = []; - let didStop: boolean = false; - let expectedNumberOfYields: number = -1; - - function shouldYield() { - if ( - expectedNumberOfYields !== -1 && - yieldedValues.length >= expectedNumberOfYields && - (scheduledCallbackTimeout === -1 || - elapsedTimeInMs < scheduledCallbackTimeout) - ) { - // We yielded at least as many values as expected. Stop rendering. - didStop = true; - return true; - } - // Keep rendering. - return false; - } - - function flushAll(): Array { - yieldedValues = []; - while (scheduledCallback !== null) { - const cb = scheduledCallback; - scheduledCallback = null; - const didTimeout = - scheduledCallbackTimeout !== -1 && - scheduledCallbackTimeout < elapsedTimeInMs; - cb(didTimeout); - } - const values = yieldedValues; - yieldedValues = []; - return values; - } - - function flushNumberOfYields(count: number): Array { - expectedNumberOfYields = count; - didStop = false; - yieldedValues = []; - try { - while (scheduledCallback !== null && !didStop) { - const cb = scheduledCallback; - scheduledCallback = null; - const didTimeout = - scheduledCallbackTimeout !== -1 && - scheduledCallbackTimeout < elapsedTimeInMs; - cb(didTimeout); - } - return yieldedValues; - } finally { - expectedNumberOfYields = -1; - didStop = false; - yieldedValues = []; - } - } - - function yieldValue(value: mixed): void { - yieldedValues.push(value); - } - - function clearYields(): Array { - const values = yieldedValues; - yieldedValues = []; - return values; - } - function childToJSX(child, text) { if (text !== null) { return text; @@ -653,6 +541,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { } const ReactNoop = { + _Scheduler: Scheduler, + getChildren(rootID: string = DEFAULT_ROOT_ID) { const container = rootContainers.get(rootID); if (container) { @@ -763,14 +653,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return NoopRenderer.findHostInstance(component); }, - // TODO: Should only be used via a Jest plugin (like we do with the - // test renderer). - unstable_flushWithoutYielding: flushAll, - unstable_flushNumberOfYields: flushNumberOfYields, - unstable_clearYields: clearYields, - flushNextYield(): Array { - return flushNumberOfYields(1); + Scheduler.unstable_flushNumberOfYields(1); + return Scheduler.unstable_clearYields(); }, flushWithHostCounters( @@ -788,7 +673,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { hostUpdateCounter = 0; hostCloneCounter = 0; try { - flushAll(); + Scheduler.flushAll(); return useMutation ? { hostDiffCounter, @@ -805,24 +690,13 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { } }, - expire(ms: number): Array { - ReactNoop.advanceTime(ms); - return ReactNoop.flushExpired(); - }, - - advanceTime(ms: number): void { - elapsedTimeInMs += ms; - }, + expire: Scheduler.advanceTime, flushExpired(): Array { - return flushNumberOfYields(0); + return Scheduler.unstable_flushExpired(); }, - yield: yieldValue, - - hasScheduledCallback() { - return !!scheduledCallback; - }, + yield: Scheduler.yieldValue, batchedUpdates: NoopRenderer.batchedUpdates, @@ -870,9 +744,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { }, flushSync(fn: () => mixed) { - yieldedValues = []; NoopRenderer.flushSync(fn); - return yieldedValues; }, flushPassiveEffects() { @@ -997,12 +869,13 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { _next: null, }; root.firstBatch = batch; - const actual = flushAll(); + Scheduler.unstable_flushWithoutYielding(); + const actual = Scheduler.unstable_clearYields(); expect(actual).toEqual(expectedFlush); return (expectedCommit: Array) => { batch._defer = false; NoopRenderer.flushRoot(root, expiration); - expect(yieldedValues).toEqual(expectedCommit); + expect(Scheduler.unstable_clearYields()).toEqual(expectedCommit); }; }, diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js index c397d01b345..b69592a0c7c 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js @@ -12,6 +12,7 @@ let React; let ReactFeatureFlags; let ReactNoop; +let Scheduler; describe('ReactExpiration', () => { beforeEach(() => { @@ -20,6 +21,7 @@ describe('ReactExpiration', () => { ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; React = require('react'); ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); }); function span(prop) { @@ -60,19 +62,33 @@ describe('ReactExpiration', () => { } } + function interrupt() { + ReactNoop.flushSync(() => { + ReactNoop.renderToRootWithID(null, 'other-root'); + }); + } + // First, show what happens for updates in two separate events. // Schedule an update. ReactNoop.render(); - // Advance the timer and flush any work that expired. Flushing the expired - // work signals to the renderer that the event has ended. - ReactNoop.advanceTime(2000); + // Advance the timer. + Scheduler.advanceTime(2000); + // Partially flush the the first update, then interrupt it. + expect(ReactNoop).toFlushAndYieldThrough(['A [render]']); + interrupt(); + // Don't advance time by enough to expire the first update. - expect(ReactNoop.flushExpired()).toEqual([]); + expect(Scheduler).toHaveYielded([]); expect(ReactNoop.getChildren()).toEqual([]); + // Schedule another update. ReactNoop.render(); // The updates should flush in separate batches, since sufficient time // passed in between them *and* they occurred in separate events. + // Note: This isn't necessarily the ideal behavior. It might be better to + // batch these two updates together. The fact that they aren't batched + // is an implementation detail. The important part of this unit test is that + // they are batched if it's possible that they happened in the same event. expect(ReactNoop).toFlushAndYield([ 'A [render]', 'A [commit]', @@ -84,10 +100,7 @@ describe('ReactExpiration', () => { // Now do the same thing again, except this time don't flush any work in // between the two updates. ReactNoop.render(); - // Advance the timer, but don't flush the expired work. Because we still - // haven't entered an idle callback, the scheduler must assume that we're - // inside the same event. - ReactNoop.advanceTime(2000); + Scheduler.advanceTime(2000); expect(ReactNoop).toHaveYielded([]); expect(ReactNoop.getChildren()).toEqual([span('B')]); // Schedule another update. @@ -114,19 +127,33 @@ describe('ReactExpiration', () => { } } + function interrupt() { + ReactNoop.flushSync(() => { + ReactNoop.renderToRootWithID(null, 'other-root'); + }); + } + // First, show what happens for updates in two separate events. // Schedule an update. ReactNoop.render(); - // Advance the timer and flush any work that expired. Flushing the expired - // work signals to the renderer that the event has ended. - ReactNoop.advanceTime(2000); + // Advance the timer. + Scheduler.advanceTime(2000); + // Partially flush the the first update, then interrupt it. + expect(ReactNoop).toFlushAndYieldThrough(['A [render]']); + interrupt(); + // Don't advance time by enough to expire the first update. - expect(ReactNoop.flushExpired()).toEqual([]); + expect(Scheduler).toHaveYielded([]); expect(ReactNoop.getChildren()).toEqual([]); + // Schedule another update. ReactNoop.render(); // The updates should flush in separate batches, since sufficient time // passed in between them *and* they occurred in separate events. + // Note: This isn't necessarily the ideal behavior. It might be better to + // batch these two updates together. The fact that they aren't batched + // is an implementation detail. The important part of this unit test is that + // they are batched if it's possible that they happened in the same event. expect(ReactNoop).toFlushAndYield([ 'A [render]', 'A [commit]', @@ -138,21 +165,15 @@ describe('ReactExpiration', () => { // Now do the same thing again, except this time don't flush any work in // between the two updates. ReactNoop.render(); - // Advance the timer, but don't flush the expired work. Because we still - // haven't entered an idle callback, the scheduler must assume that we're - // inside the same event. - ReactNoop.advanceTime(2000); + Scheduler.advanceTime(2000); expect(ReactNoop).toHaveYielded([]); expect(ReactNoop.getChildren()).toEqual([span('B')]); - // Perform some synchronous work. Again, the scheduler must assume we're - // inside the same event. - ReactNoop.flushSync(() => { - ReactNoop.renderToRootWithID('1', 'second-root'); - }); + // Perform some synchronous work. The scheduler must assume we're inside + // the same event. + interrupt(); - // Even though React flushed a sync update, it should not have updated the - // current time. Schedule another update. + // Schedule another update. ReactNoop.render(); // The updates should flush in the same batch, since as far as the scheduler // knows, they may have occurred inside the same event. diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 9a1c73a62f2..568d9e0872b 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -15,6 +15,7 @@ let React; let ReactFeatureFlags; let ReactNoop; +let Scheduler; let SchedulerTracing; let useState; let useReducer; @@ -40,6 +41,7 @@ describe('ReactHooksWithNoopRenderer', () => { ReactFeatureFlags.enableSchedulerTracing = true; React = require('react'); ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); SchedulerTracing = require('scheduler/tracing'); useState = React.useState; useReducer = React.useReducer; @@ -595,22 +597,25 @@ describe('ReactHooksWithNoopRenderer', () => { it('simple mount and update', () => { function Counter(props) { useEffect(() => { - ReactNoop.yield(`Did commit [${props.count}]`); + ReactNoop.yield(`Passive effect [${props.count}]`); }); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - ReactNoop.flushPassiveEffects(); - expect(ReactNoop).toHaveYielded(['Did commit [0]']); + // Effects are deferred until after the commit + expect(ReactNoop).toFlushAndYield(['Passive effect [0]']); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); // Effects are deferred until after the commit - ReactNoop.flushPassiveEffects(); - expect(ReactNoop).toHaveYielded(['Did commit [1]']); + expect(ReactNoop).toFlushAndYield(['Passive effect [1]']); }); it('flushes passive effects even with sibling deletions', () => { @@ -628,7 +633,11 @@ describe('ReactHooksWithNoopRenderer', () => { } let passive = ; ReactNoop.render([, passive]); - expect(ReactNoop).toFlushAndYield(['Layout', 'Passive', 'Layout effect']); + expect(ReactNoop).toFlushAndYieldThrough([ + 'Layout', + 'Passive', + 'Layout effect', + ]); expect(ReactNoop.getChildren()).toEqual([ span('Layout'), span('Passive'), @@ -733,17 +742,21 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield([0]); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough([0, 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span(0)]); // Before the effects have a chance to flush, schedule another update - ReactNoop.render(); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); expect(ReactNoop).toHaveYielded([ // The previous effect flushes before the reconciliation 'Committed state when effect was fired: 0', ]); - expect(ReactNoop).toFlushAndYield([1]); + expect(ReactNoop).toFlushAndYieldThrough([1, 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span(1)]); ReactNoop.flushPassiveEffects(); @@ -765,15 +778,22 @@ describe('ReactHooksWithNoopRenderer', () => { ); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: (empty)']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough([ + 'Count: (empty)', + 'Sync effect', + ]); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Schedule update [0]']); expect(ReactNoop).toFlushAndYield(['Count: 0']); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Schedule update [1]']); @@ -792,20 +812,24 @@ describe('ReactHooksWithNoopRenderer', () => { ); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: (empty)']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough([ + 'Count: (empty)', + 'Sync effect', + ]); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); // Rendering again should flush the previous commit's effects - ReactNoop.render(); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); expect(ReactNoop).toHaveYielded(['Schedule update [0]']); expect(ReactNoop).toFlushAndYieldThrough(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); - ReactNoop.batchedUpdates(() => { - expect(ReactNoop).toFlushAndYield([]); - }); - + expect(ReactNoop).toFlushAndYieldThrough(['Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); @@ -826,8 +850,10 @@ describe('ReactHooksWithNoopRenderer', () => { return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); // Enqueuing this update forces the passive effect to be flushed -- @@ -869,10 +895,12 @@ describe('ReactHooksWithNoopRenderer', () => { tracingEvent.name, tracingEvent.timestamp, () => { - ReactNoop.render(); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); }, ); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(0); @@ -936,8 +964,13 @@ describe('ReactHooksWithNoopRenderer', () => { ); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: (empty)']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough([ + 'Count: (empty)', + 'Sync effect', + ]); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); expect(() => { @@ -955,14 +988,18 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Did create [0]']); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Did destroy [0]', 'Did create [1]']); @@ -978,8 +1015,10 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Did create [0]']); @@ -999,14 +1038,18 @@ describe('ReactHooksWithNoopRenderer', () => { }, []); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Did create [0]']); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded([]); @@ -1027,14 +1070,18 @@ describe('ReactHooksWithNoopRenderer', () => { useEffect(effect); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Did create']); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Did destroy', 'Did create']); @@ -1058,15 +1105,19 @@ describe('ReactHooksWithNoopRenderer', () => { ); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Did create [Count: 0]']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - ReactNoop.render(); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); // Count changed - expect(ReactNoop).toFlushAndYield(['Count: 1']); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded([ @@ -1074,16 +1125,20 @@ describe('ReactHooksWithNoopRenderer', () => { 'Did create [Count: 1]', ]); - ReactNoop.render(); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); // Nothing changed, so no effect should have fired - expect(ReactNoop).toFlushAndYield(['Count: 1']); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded([]); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); - ReactNoop.render(); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); // Label changed - expect(ReactNoop).toFlushAndYield(['Total: 1']); + expect(ReactNoop).toFlushAndYieldThrough(['Total: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Total: 1')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded([ @@ -1102,14 +1157,18 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Did commit 1 [0]', 'Did commit 2 [0]']); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Did commit 1 [1]', 'Did commit 2 [1]']); @@ -1131,14 +1190,18 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Mount A [0]', 'Mount B [0]']); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded([ @@ -1168,8 +1231,10 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops'); expect(ReactNoop).toHaveYielded([ @@ -1202,15 +1267,19 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Mount A [0]', 'Mount B [0]']); // This update will trigger an errror - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops'); expect(ReactNoop).toHaveYielded([ @@ -1244,15 +1313,19 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 0', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Mount A [0]', 'Mount B [0]']); // This update will trigger an errror - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops'); expect(ReactNoop).toHaveYielded([ @@ -1273,16 +1346,29 @@ describe('ReactHooksWithNoopRenderer', () => { } Counter = memo(Counter); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 0', 'Mount: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough([ + 'Count: 0', + 'Mount: 0', + 'Sync effect', + ]); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Count: 1', 'Unmount: 0', 'Mount: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough([ + 'Count: 1', + 'Unmount: 0', + 'Mount: 1', + 'Sync effect', + ]); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); ReactNoop.render(null); - expect(ReactNoop).toFlushAndYield(['Unmount: 1']); + expect(ReactNoop).toFlushAndYieldThrough(['Unmount: 1']); expect(ReactNoop.getChildren()).toEqual([]); }); }); @@ -1290,7 +1376,7 @@ describe('ReactHooksWithNoopRenderer', () => { describe('useLayoutEffect', () => { it('fires layout effects after the host has been mutated', () => { function getCommittedText() { - const yields = ReactNoop.unstable_clearYields(); + const yields = Scheduler.unstable_clearYields(); const children = ReactNoop.getChildren(); ReactNoop.yield(yields); if (children === null) { @@ -1306,12 +1392,24 @@ describe('ReactHooksWithNoopRenderer', () => { return ; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield([[0], 'Current: 0']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough([ + [0], + 'Current: 0', + 'Sync effect', + ]); expect(ReactNoop.getChildren()).toEqual([span(0)]); - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield([[1], 'Current: 1']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough([ + [1], + 'Current: 1', + 'Sync effect', + ]); expect(ReactNoop.getChildren()).toEqual([span(1)]); }); @@ -1338,15 +1436,23 @@ describe('ReactHooksWithNoopRenderer', () => { return null; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield(['Mount layout [current: 0]']); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough([ + 'Mount layout [current: 0]', + 'Sync effect', + ]); expect(committedText).toEqual('0'); - ReactNoop.render(); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); expect(ReactNoop).toHaveYielded(['Mount normal [current: 0]']); - expect(ReactNoop).toFlushAndYield([ + expect(ReactNoop).toFlushAndYieldThrough([ 'Unmount layout [current: 0]', 'Mount layout [current: 1]', + 'Sync effect', ]); expect(committedText).toEqual('1'); @@ -1823,8 +1929,10 @@ describe('ReactHooksWithNoopRenderer', () => { return null; } - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield([]); + ReactNoop.render(, () => + ReactNoop.yield('Sync effect'), + ); + expect(ReactNoop).toFlushAndYieldThrough(['Sync effect']); ReactNoop.flushPassiveEffects(); expect(ReactNoop).toHaveYielded(['Mount A']); diff --git a/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js index 1ae63e690e7..586400acd3c 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js @@ -2862,9 +2862,7 @@ describe('ReactIncremental', () => { expect(ReactNoop).toFlushAndYieldThrough(['Parent: 1']); // Interrupt at higher priority - expect( - ReactNoop.flushSync(() => ReactNoop.render()), - ).toEqual(['Parent: 2', 'Child: 2']); + ReactNoop.flushSync(() => ReactNoop.render()); expect(ReactNoop).toHaveYielded(['Parent: 2', 'Child: 2']); expect(ReactNoop).toFlushAndYield([]); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index fe09a682cb4..4bf486ca439 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -14,6 +14,7 @@ let PropTypes; let ReactFeatureFlags; let React; let ReactNoop; +let Scheduler; describe('ReactIncrementalErrorHandling', () => { beforeEach(() => { @@ -24,6 +25,7 @@ describe('ReactIncrementalErrorHandling', () => { PropTypes = require('prop-types'); React = require('react'); ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); }); function div(...children) { @@ -226,8 +228,18 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.yield('commit'); } + function interrupt() { + ReactNoop.flushSync(() => { + ReactNoop.renderToRootWithID(null, 'other-root'); + }); + } + ReactNoop.render(, onCommit); - ReactNoop.expire(2000); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['error']); + interrupt(); + + // This update is in a separate batch ReactNoop.render(, onCommit); expect(ReactNoop).toFlushAndYield([ diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js index cab78697424..c1f555207c6 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js @@ -283,7 +283,6 @@ describe('ReactDebugFiberPerf', () => { componentDidMount() { ReactNoop.renderToRootWithID(, 'b'); addComment('Scheduling another root from componentDidMount'); - expect(ReactNoop).toFlushWithoutYielding(); } render() { return
{this.props.children}
; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js index b1896b97b37..75185d263c4 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js @@ -361,37 +361,4 @@ describe('ReactIncrementalScheduling', () => { 'componentDidUpdate: 2', ]); }); - - it('updates do not schedule a new callback if already inside a callback', () => { - class Foo extends React.Component { - state = {foo: 'foo'}; - UNSAFE_componentWillReceiveProps() { - ReactNoop.yield( - 'has callback before setState: ' + ReactNoop.hasScheduledCallback(), - ); - this.setState({foo: 'baz'}); - ReactNoop.yield( - 'has callback after setState: ' + ReactNoop.hasScheduledCallback(), - ); - } - render() { - return null; - } - } - - ReactNoop.render(); - expect(() => { - expect(ReactNoop).toFlushWithoutYielding(); - }).toWarnDev( - 'componentWillReceiveProps: Please update the following components ' + - 'to use static getDerivedStateFromProps instead: Foo', - {withoutStack: true}, - ); - - ReactNoop.render(); - expect(ReactNoop).toFlushAndYield([ - 'has callback before setState: false', - 'has callback after setState: false', - ]); - }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js index 70b1d57204a..23c74dbc63c 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js @@ -13,6 +13,7 @@ let React; let ReactFeatureFlags; let ReactNoop; +let Scheduler; describe('ReactIncrementalTriangle', () => { beforeEach(() => { @@ -21,6 +22,7 @@ describe('ReactIncrementalTriangle', () => { ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; React = require('react'); ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); }); function span(prop) { @@ -306,7 +308,7 @@ describe('ReactIncrementalTriangle', () => { } else { ReactNoop.render(); } - ReactNoop.unstable_flushWithoutYielding(); + Scheduler.unstable_flushWithoutYielding(); assertConsistentTree(); return appInstance; } @@ -385,10 +387,10 @@ describe('ReactIncrementalTriangle', () => { ReactNoop.flushSync(() => { switch (action.type) { case FLUSH: - ReactNoop.unstable_flushNumberOfYields(action.unitsOfWork); + Scheduler.unstable_flushNumberOfYields(action.unitsOfWork); break; case FLUSH_ALL: - ReactNoop.unstable_flushWithoutYielding(); + Scheduler.unstable_flushWithoutYielding(); break; case STEP: ReactNoop.deferredUpdates(() => { @@ -430,7 +432,7 @@ describe('ReactIncrementalTriangle', () => { assertConsistentTree(activeLeafIndices); } // Flush remaining work - ReactNoop.unstable_flushWithoutYielding(); + Scheduler.unstable_flushWithoutYielding(); assertConsistentTree(activeLeafIndices, expectedCounterAtEnd); } diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js index 9042a7e4dd9..fb67e6d937b 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js @@ -13,6 +13,7 @@ let React; let ReactFeatureFlags; let ReactNoop; +let Scheduler; describe('ReactIncrementalUpdates', () => { beforeEach(() => { @@ -21,6 +22,7 @@ describe('ReactIncrementalUpdates', () => { ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; React = require('react'); ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); }); function span(prop) { @@ -456,33 +458,50 @@ describe('ReactIncrementalUpdates', () => { }); it('flushes all expired updates in a single batch', () => { - class Foo extends React.Component { - componentDidUpdate() { - ReactNoop.yield('Commit: ' + this.props.prop); - } - componentDidMount() { - ReactNoop.yield('Commit: ' + this.props.prop); - } - render() { - ReactNoop.yield('Render: ' + this.props.prop); - return ; - } + const {useEffect} = React; + + function App({label}) { + ReactNoop.yield('Render: ' + label); + useEffect(() => { + ReactNoop.yield('Commit: ' + label); + }); + return label; + } + + function interrupt() { + ReactNoop.flushSync(() => { + ReactNoop.renderToRootWithID(null, 'other-root'); + }); } // First, as a sanity check, assert what happens when four low pri // updates in separate batches are all flushed in the same callback - ReactNoop.render(); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.render(); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.render(); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.render(); - - // There should be a separate render and commit for each update + ReactNoop.render(); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.render(); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.render(); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.render(); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + // Each update flushes in a separate commit. + // Note: This isn't necessarily the ideal behavior. It might be better to + // batch all of these updates together. The fact that they don't is an + // implementation detail. The important part of this unit test is what + // happens when they expire, in which case they really should be batched to + // avoid blocking the main thread for a long time. expect(ReactNoop).toFlushAndYield([ 'Render: ', 'Commit: ', @@ -493,65 +512,86 @@ describe('ReactIncrementalUpdates', () => { 'Render: hello', 'Commit: hello', ]); - expect(ReactNoop.getChildren()).toEqual([span('hello')]); - - // Now do the same thing, except this time expire all the updates - // before flushing them. - ReactNoop.render(); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.render(); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.render(); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.render(); - - ReactNoop.advanceTime(10000); - jest.advanceTimersByTime(10000); + + // Now do the same thing over again, but this time, expire all the updates + // instead of flushing them normally. + ReactNoop.render(); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.render(); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.render(); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.render(); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); // All the updates should render and commit in a single batch. - expect(ReactNoop).toFlushAndYield(['Render: goodbye', 'Commit: goodbye']); - expect(ReactNoop.getChildren()).toEqual([span('goodbye')]); + Scheduler.advanceTime(10000); + expect(ReactNoop).toHaveYielded(['Render: goodbye']); + // Passive effect + expect(ReactNoop).toFlushAndYield(['Commit: goodbye']); }); it('flushes all expired updates in a single batch across multiple roots', () => { // Same as previous test, but with two roots. - class Foo extends React.Component { - componentDidUpdate() { - ReactNoop.yield('Commit: ' + this.props.prop); - } - componentDidMount() { - ReactNoop.yield('Commit: ' + this.props.prop); - } - render() { - ReactNoop.yield('Render: ' + this.props.prop); - return ; - } + const {useEffect} = React; + + function App({label}) { + ReactNoop.yield('Render: ' + label); + useEffect(() => { + ReactNoop.yield('Commit: ' + label); + }); + return label; + } + + function interrupt() { + ReactNoop.flushSync(() => { + ReactNoop.renderToRootWithID(null, 'other-root'); + }); } // First, as a sanity check, assert what happens when four low pri // updates in separate batches are all flushed in the same callback - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - - // There should be a separate render and commit for each update + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + // Each update flushes in a separate commit. + // Note: This isn't necessarily the ideal behavior. It might be better to + // batch all of these updates together. The fact that they don't is an + // implementation detail. The important part of this unit test is what + // happens when they expire, in which case they really should be batched to + // avoid blocking the main thread for a long time. expect(ReactNoop).toFlushAndYield([ 'Render: ', 'Commit: ', @@ -570,37 +610,41 @@ describe('ReactIncrementalUpdates', () => { 'Render: hello', 'Commit: hello', ]); - expect(ReactNoop.getChildren('a')).toEqual([span('hello')]); - expect(ReactNoop.getChildren('b')).toEqual([span('hello')]); - - // Now do the same thing, except this time expire all the updates - // before flushing them. - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - - ReactNoop.advanceTime(10000); - jest.advanceTimersByTime(10000); + + // Now do the same thing over again, but this time, expire all the updates + // instead of flushing them normally. + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); + + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + Scheduler.advanceTime(1000); + expect(ReactNoop).toFlushAndYieldThrough(['Render: ']); + interrupt(); // All the updates should render and commit in a single batch. - expect(ReactNoop).toFlushAndYield([ + Scheduler.advanceTime(10000); + expect(ReactNoop).toHaveYielded([ 'Render: goodbye', 'Commit: goodbye', 'Render: goodbye', - 'Commit: goodbye', ]); - expect(ReactNoop.getChildren('a')).toEqual([span('goodbye')]); - expect(ReactNoop.getChildren('b')).toEqual([span('goodbye')]); + // Passive effect + expect(ReactNoop).toFlushAndYield(['Commit: goodbye']); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 3008ffac6ad..ec81f523847 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -14,6 +14,7 @@ let ReactFeatureFlags = require('shared/ReactFeatureFlags'); let React = require('react'); let useContext; let ReactNoop; +let Scheduler; let gen; describe('ReactNewContext', () => { @@ -24,6 +25,7 @@ describe('ReactNewContext', () => { React = require('react'); useContext = React.useContext; ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); gen = require('random-seed'); }); @@ -1769,10 +1771,10 @@ describe('ReactNewContext', () => { actions.forEach(action => { switch (action.type) { case FLUSH_ALL: - ReactNoop.unstable_flushWithoutYielding(); + Scheduler.unstable_flushWithoutYielding(); break; case FLUSH: - ReactNoop.unstable_flushNumberOfYields(action.unitsOfWork); + Scheduler.unstable_flushNumberOfYields(action.unitsOfWork); break; case UPDATE: finalExpectedValues = { @@ -1787,7 +1789,7 @@ describe('ReactNewContext', () => { assertConsistentTree(); }); - ReactNoop.unstable_flushWithoutYielding(); + Scheduler.unstable_flushWithoutYielding(); assertConsistentTree(finalExpectedValues); } diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 6559a20aa41..135217466cb 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -2,6 +2,7 @@ let React; let ReactFeatureFlags; let Fragment; let ReactNoop; +let Scheduler; let ReactCache; let Suspense; let StrictMode; @@ -22,6 +23,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { React = require('react'); Fragment = React.Fragment; ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); ReactCache = require('react-cache'); Suspense = React.Suspense; StrictMode = React.StrictMode; @@ -449,7 +451,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { await advanceTimers(10000); // No additional rendering work is required, since we already prepared // the placeholder. - expect(ReactNoop.flushExpired()).toEqual([]); + expect(Scheduler).toHaveYielded([]); // Should have committed the placeholder. expect(ReactNoop.getChildren()).toEqual([span('Loading...'), span('Sync')]); @@ -636,7 +638,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); expect(ReactNoop.flushNextYield()).toEqual(['Suspend! [Async]']); await advanceTimers(1500); - expect(ReactNoop.expire(1500)).toEqual([]); + expect(Scheduler).toHaveYielded([]); // Before we have a chance to flush, the promise resolves. await advanceTimers(2000); expect(ReactNoop).toHaveYielded(['Promise resolved [Async]']); @@ -659,7 +661,8 @@ describe('ReactSuspenseWithNoopRenderer', () => { , ); - expect(ReactNoop.expire(10000)).toEqual([ + Scheduler.advanceTime(10000); + expect(Scheduler).toHaveYielded([ 'Suspend! [A]', 'Suspend! [B]', 'Loading...', @@ -780,17 +783,17 @@ describe('ReactSuspenseWithNoopRenderer', () => { jest.advanceTimersByTime(1000); ReactNoop.render(); - ReactNoop.advanceTime(10000); + Scheduler.advanceTime(10000); jest.advanceTimersByTime(10000); - expect(ReactNoop).toFlushAndYield([ + expect(ReactNoop).toHaveYielded([ 'Suspend! [goodbye]', 'Loading...', 'Commit: goodbye', ]); expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); - ReactNoop.advanceTime(20000); + Scheduler.advanceTime(20000); await advanceTimers(20000); expect(ReactNoop).toHaveYielded(['Promise resolved [goodbye]']); expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 20924123851..4be4de71c34 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -13,6 +13,7 @@ let React; let ReactFeatureFlags; let ReactNoop; +let Scheduler; let ReactCache; let ReactTestRenderer; let advanceTimeBy; @@ -44,6 +45,7 @@ function loadModules({ ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = replayFailedUnitOfWorkWithInvokeGuardedCallback; React = require('react'); + Scheduler = require('scheduler'); SchedulerTracing = require('scheduler/tracing'); ReactCache = require('react-cache'); @@ -1205,7 +1207,7 @@ describe('Profiler', () => { loadModules({useNoopRenderer: true}); const Child = ({duration, id}) => { - ReactNoop.advanceTime(duration); + Scheduler.advanceTime(duration); ReactNoop.yield(`Child:render:${id}`); return null; }; @@ -1225,7 +1227,7 @@ describe('Profiler', () => { } } - ReactNoop.advanceTime(50); + Scheduler.advanceTime(50); ReactNoop.renderToRootWithID(, 'one'); @@ -1238,13 +1240,13 @@ describe('Profiler', () => { expect(ReactNoop.getRoot('one').current.actualDuration).toBe(0); - ReactNoop.advanceTime(100); + Scheduler.advanceTime(100); // Process some async work, but yield before committing it. ReactNoop.renderToRootWithID(, 'two'); expect(ReactNoop).toFlushAndYieldThrough(['Child:render:two']); - ReactNoop.advanceTime(150); + Scheduler.advanceTime(150); // Commit the previously paused, batched work. commitFirstRender(['Parent:componentDidMount:one']); @@ -1252,9 +1254,8 @@ describe('Profiler', () => { expect(ReactNoop.getRoot('one').current.actualDuration).toBe(6); expect(ReactNoop.getRoot('two').current.actualDuration).toBe(0); - ReactNoop.advanceTime(200); + Scheduler.advanceTime(200); - expect(ReactNoop).toHaveYielded(['Parent:componentDidMount:one']); expect(ReactNoop).toFlushAndYield([ 'Child:render:two', 'Parent:componentDidMount:two', @@ -2256,7 +2257,7 @@ describe('Profiler', () => { await awaitableAdvanceTimers(10000); // No additional rendering work is required, since we already prepared // the placeholder. - expect(ReactNoop.flushExpired()).toEqual([]); + expect(Scheduler).toHaveYielded([]); // Should have committed the placeholder. expect(ReactNoop.getChildrenAsJSX()).toEqual('Loading...Sync'); expect(onRender).toHaveBeenCalledTimes(1); diff --git a/scripts/jest/matchers/reactTestMatchers.js b/scripts/jest/matchers/reactTestMatchers.js index fb3d01f549f..4007065e163 100644 --- a/scripts/jest/matchers/reactTestMatchers.js +++ b/scripts/jest/matchers/reactTestMatchers.js @@ -19,16 +19,18 @@ function captureAssertion(fn) { return {pass: true}; } -function isScheduler(obj) { - return typeof obj.unstable_scheduleCallback === 'function'; -} - -function isReactNoop(obj) { - return typeof obj.hasScheduledCallback === 'function'; +function resolveScheduler(obj) { + if (obj._Scheduler !== undefined) { + return obj._Scheduler; + } + if (typeof obj.unstable_scheduleCallback === 'function') { + return obj; + } + return null; } -function assertYieldsWereCleared(ReactNoop) { - const actualYields = ReactNoop.unstable_clearYields(); +function assertYieldsWereCleared(Scheduler) { + const actualYields = Scheduler.unstable_clearYields(); if (actualYields.length !== 0) { throw new Error( 'Log of yielded values is not empty. ' + @@ -38,81 +40,54 @@ function assertYieldsWereCleared(ReactNoop) { } function toFlushAndYield(ReactNoop, expectedYields) { - if (isScheduler(ReactNoop)) { - return SchedulerMatchers.toFlushAndYield(ReactNoop, expectedYields); - } - if (!isReactNoop(ReactNoop)) { + const Scheduler = resolveScheduler(ReactNoop); + if (Scheduler === null) { return JestReact.unstable_toFlushAndYield(ReactNoop, expectedYields); } - assertYieldsWereCleared(ReactNoop); - const actualYields = ReactNoop.unstable_flushWithoutYielding(); - return captureAssertion(() => { - expect(actualYields).toEqual(expectedYields); - }); + return SchedulerMatchers.toFlushAndYield(Scheduler, expectedYields); } function toFlushAndYieldThrough(ReactNoop, expectedYields) { - if (isScheduler(ReactNoop)) { - return SchedulerMatchers.toFlushAndYieldThrough(ReactNoop, expectedYields); - } - if (!isReactNoop(ReactNoop)) { + const Scheduler = resolveScheduler(ReactNoop); + if (Scheduler === null) { return JestReact.unstable_toFlushAndYieldThrough(ReactNoop, expectedYields); } - assertYieldsWereCleared(ReactNoop); - const actualYields = ReactNoop.unstable_flushNumberOfYields( - expectedYields.length - ); - return captureAssertion(() => { - expect(actualYields).toEqual(expectedYields); - }); + return SchedulerMatchers.toFlushAndYieldThrough(Scheduler, expectedYields); } function toFlushWithoutYielding(ReactNoop) { - if (isScheduler(ReactNoop)) { - return SchedulerMatchers.toFlushWithoutYielding(ReactNoop); - } - if (!isReactNoop(ReactNoop)) { + const Scheduler = resolveScheduler(ReactNoop); + if (Scheduler === null) { return JestReact.unstable_toFlushWithoutYielding(ReactNoop); } - return toFlushAndYield(ReactNoop, []); + return SchedulerMatchers.toFlushWithoutYielding(Scheduler); } function toHaveYielded(ReactNoop, expectedYields) { - if (isScheduler(ReactNoop)) { - return SchedulerMatchers.toHaveYielded(ReactNoop, expectedYields); - } - if (!isReactNoop(ReactNoop)) { + const Scheduler = resolveScheduler(ReactNoop); + if (Scheduler === null) { return JestReact.unstable_toHaveYielded(ReactNoop, expectedYields); } - return captureAssertion(() => { - const actualYields = ReactNoop.unstable_clearYields(); - expect(actualYields).toEqual(expectedYields); - }); + return SchedulerMatchers.toHaveYielded(Scheduler, expectedYields); } function toFlushAndThrow(ReactNoop, ...rest) { - if (isScheduler(ReactNoop)) { - return SchedulerMatchers.toFlushAndThrow(ReactNoop, ...rest); - } - if (!isReactNoop(ReactNoop)) { + const Scheduler = resolveScheduler(ReactNoop); + if (Scheduler === null) { return JestReact.unstable_toFlushAndThrow(ReactNoop, ...rest); } - assertYieldsWereCleared(ReactNoop); - return captureAssertion(() => { - expect(() => { - ReactNoop.unstable_flushWithoutYielding(); - }).toThrow(...rest); - }); + return SchedulerMatchers.toFlushAndThrow(Scheduler, ...rest); } function toMatchRenderedOutput(ReactNoop, expectedJSX) { - if (!isReactNoop(ReactNoop)) { - return JestReact.unstable_toMatchRenderedOutput(ReactNoop, expectedJSX); + if (typeof ReactNoop.getChildrenAsJSX === 'function') { + const Scheduler = ReactNoop._Scheduler; + assertYieldsWereCleared(Scheduler); + return captureAssertion(() => { + expect(ReactNoop.getChildrenAsJSX()).toEqual(expectedJSX); + }); } - assertYieldsWereCleared(ReactNoop); - return captureAssertion(() => { - expect(ReactNoop.getChildrenAsJSX()).toEqual(expectedJSX); - }); + return JestReact.unstable_toMatchRenderedOutput(ReactNoop, expectedJSX); } module.exports = { From 932034c2cfe63fb4db90719de4dda42e58f9a3a3 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 27 Feb 2019 14:49:22 -0800 Subject: [PATCH 2/2] Fix import to scheduler/unstable_mock --- packages/react-noop-renderer/src/createReactNoop.js | 2 +- scripts/rollup/forks.js | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index a3871263fda..c8b17c961d5 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -18,7 +18,7 @@ import type {Fiber} from 'react-reconciler/src/ReactFiber'; import type {UpdateQueue} from 'react-reconciler/src/ReactUpdateQueue'; import type {ReactNodeList} from 'shared/ReactTypes'; -import * as Scheduler from 'scheduler'; +import * as Scheduler from 'scheduler/unstable_mock'; import {createPortal} from 'shared/ReactPortal'; import expect from 'expect'; import {REACT_FRAGMENT_TYPE, REACT_ELEMENT_TYPE} from 'shared/ReactSymbols'; diff --git a/scripts/rollup/forks.js b/scripts/rollup/forks.js index 98c1b4a00f6..f86cceb09ba 100644 --- a/scripts/rollup/forks.js +++ b/scripts/rollup/forks.js @@ -169,7 +169,11 @@ const forks = Object.freeze({ }, 'scheduler/src/SchedulerHostConfig': (bundleType, entry, dependencies) => { - if (entry === 'scheduler/unstable_mock') { + if ( + entry === 'scheduler/unstable_mock' || + entry === 'react-noop-renderer' || + entry === 'react-noop-renderer/persistent' + ) { return 'scheduler/src/forks/SchedulerHostConfig.mock'; } return 'scheduler/src/forks/SchedulerHostConfig.default';