From ba708fa79b3db6481b7886f9fdb6bb776d0c2fb9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 22 Feb 2019 17:27:30 -0800 Subject: [PATCH] Remove ReactNoop.flushDeferredPri and flushUnitsOfWork (#14934) * Remove ReactNoop.flushDeferredPri and flushUnitsOfWork Some of our older tests worked by counting how many times React checked whether it should yield to the main thread, instead of something publicly observable like how many times a component is rendered. Our newer tests have converged on a style where we push into a log and make assertions on the log. This pattern is less coupled to the implementation while still being sufficient to test performance optimizations, like resuming (whenever we add that back). This commit removes flushDeferredPri and flushUnitsOfWork and upgrades the affected tests. * Remove shouldYieldToRenderer indirection This wrapper is no longer necessary. --- .../src/createReactNoop.js | 39 ++-- .../src/ReactFiberScheduler.js | 57 ++---- .../ReactIncremental-test.internal.js | 191 +++++++----------- ...tIncrementalErrorHandling-test.internal.js | 28 +-- .../ReactIncrementalErrorLogging-test.js | 6 +- .../ReactIncrementalPerf-test.internal.js | 26 ++- ...eactIncrementalReflection-test.internal.js | 123 ++++------- ...eactIncrementalScheduling-test.internal.js | 104 ++++++---- ...actIncrementalSideEffects-test.internal.js | 85 ++++---- .../ReactIncrementalTriangle-test.internal.js | 2 +- .../ReactIncrementalUpdates-test.internal.js | 3 +- .../ReactNewContext-test.internal.js | 3 +- ...ReactIncrementalPerf-test.internal.js.snap | 28 ++- 13 files changed, 309 insertions(+), 386 deletions(-) diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 6e1272e42d66..985493ea4713 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -747,36 +747,27 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return NoopRenderer.findHostInstance(component); }, - flushDeferredPri(timeout: number = Infinity): Array { - // The legacy version of this function decremented the timeout before - // returning the new time. - // TODO: Convert tests to use flushUnitsOfWork or flushAndYield instead. - const n = timeout / 5 - 1; - - let values = []; + flush(): Array { + let values = yieldedValues || []; + yieldedValues = null; // eslint-disable-next-line no-for-of-loops/no-for-of-loops - for (const value of flushUnitsOfWork(n)) { + for (const value of flushUnitsOfWork(Infinity)) { values.push(...value); } return values; }, - flush(): Array { - return ReactNoop.flushUnitsOfWork(Infinity); - }, - - flushAndYield( - unitsOfWork: number = Infinity, - ): Generator, void, void> { - return flushUnitsOfWork(unitsOfWork); - }, - - flushUnitsOfWork(n: number): Array { + // TODO: Should only be used via a Jest plugin (like we do with the + // test renderer). + unstable_flushNumberOfYields(n: number): Array { let values = yieldedValues || []; yieldedValues = null; // eslint-disable-next-line no-for-of-loops/no-for-of-loops - for (const value of flushUnitsOfWork(n)) { + for (const value of flushUnitsOfWork(Infinity)) { values.push(...value); + if (values.length >= n) { + break; + } } return values; }, @@ -847,7 +838,13 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { }, flushExpired(): Array { - return ReactNoop.flushUnitsOfWork(0); + let values = yieldedValues || []; + yieldedValues = null; + // eslint-disable-next-line no-for-of-loops/no-for-of-loops + for (const value of flushUnitsOfWork(0)) { + values.push(...value); + } + return values; }, yield(value: mixed) { diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 4524d87b57b1..415d5bbc98ae 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1214,7 +1214,7 @@ function workLoop(isYieldy) { } } else { // Flush asynchronous work until there's a higher priority event - while (nextUnitOfWork !== null && !shouldYieldToRenderer()) { + while (nextUnitOfWork !== null && !shouldYield()) { nextUnitOfWork = performUnitOfWork(nextUnitOfWork); } } @@ -2007,7 +2007,7 @@ function onSuspend( msUntilTimeout: number, ): void { root.expirationTime = rootExpirationTime; - if (msUntilTimeout === 0 && !shouldYieldToRenderer()) { + if (msUntilTimeout === 0 && !shouldYield()) { // Don't wait an additional tick. Commit the tree immediately. root.pendingCommitExpirationTime = suspendedExpirationTime; root.finishedWork = finishedWork; @@ -2204,43 +2204,24 @@ function findHighestPriorityRoot() { nextFlushedExpirationTime = highestPriorityWork; } -// TODO: This wrapper exists because many of the older tests (the ones that use -// flushDeferredPri) rely on the number of times `shouldYield` is called. We -// should get rid of it. -let didYield: boolean = false; -function shouldYieldToRenderer() { - if (didYield) { - return true; - } - if (shouldYield()) { - didYield = true; - return true; - } - return false; -} - function performAsyncWork(didTimeout) { - try { - if (didTimeout) { - // The callback timed out. That means at least one update has expired. - // Iterate through the root schedule. If they contain expired work, set - // the next render expiration time to the current time. This has the effect - // of flushing all expired work in a single batch, instead of flushing each - // level one at a time. - if (firstScheduledRoot !== null) { - recomputeCurrentRendererTime(); - let root: FiberRoot = firstScheduledRoot; - do { - didExpireAtExpirationTime(root, currentRendererTime); - // The root schedule is circular, so this is never null. - root = (root.nextScheduledRoot: any); - } while (root !== firstScheduledRoot); - } + if (didTimeout) { + // The callback timed out. That means at least one update has expired. + // Iterate through the root schedule. If they contain expired work, set + // the next render expiration time to the current time. This has the effect + // of flushing all expired work in a single batch, instead of flushing each + // level one at a time. + if (firstScheduledRoot !== null) { + recomputeCurrentRendererTime(); + let root: FiberRoot = firstScheduledRoot; + do { + didExpireAtExpirationTime(root, currentRendererTime); + // The root schedule is circular, so this is never null. + root = (root.nextScheduledRoot: any); + } while (root !== firstScheduledRoot); } - performWork(NoWork, true); - } finally { - didYield = false; } + performWork(NoWork, true); } function performSyncWork() { @@ -2266,7 +2247,7 @@ function performWork(minExpirationTime: ExpirationTime, isYieldy: boolean) { nextFlushedRoot !== null && nextFlushedExpirationTime !== NoWork && minExpirationTime <= nextFlushedExpirationTime && - !(didYield && currentRendererTime > nextFlushedExpirationTime) + !(shouldYield() && currentRendererTime > nextFlushedExpirationTime) ) { performWorkOnRoot( nextFlushedRoot, @@ -2414,7 +2395,7 @@ function performWorkOnRoot( if (finishedWork !== null) { // We've completed the root. Check the if we should yield one more time // before committing. - if (!shouldYieldToRenderer()) { + if (!shouldYield()) { // Still time left. Commit the root. completeRoot(root, finishedWork, expirationTime); } else { diff --git a/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js index 69bd1dd0aa90..1f65fc6ec3d9 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js @@ -39,10 +39,8 @@ describe('ReactIncremental', () => { }); it('should render a simple component, in steps if needed', () => { - let renderCallbackCalled = false; - let barCalled = false; function Bar() { - barCalled = true; + ReactNoop.yield('Bar'); return (
Hello World
@@ -50,26 +48,17 @@ describe('ReactIncremental', () => { ); } - let fooCalled = false; function Foo() { - fooCalled = true; + ReactNoop.yield('Foo'); return [, ]; } - ReactNoop.render(, () => (renderCallbackCalled = true)); - expect(fooCalled).toBe(false); - expect(barCalled).toBe(false); - expect(renderCallbackCalled).toBe(false); + ReactNoop.render(, () => ReactNoop.yield('callback')); // Do one step of work. - ReactNoop.flushDeferredPri(7 + 5); - expect(fooCalled).toBe(true); - expect(barCalled).toBe(false); - expect(renderCallbackCalled).toBe(false); + expect(ReactNoop.flushNextYield()).toEqual(['Foo']); + // Do the rest of the work. - ReactNoop.flushDeferredPri(50); - expect(fooCalled).toBe(true); - expect(barCalled).toBe(true); - expect(renderCallbackCalled).toBe(true); + expect(ReactNoop.flush()).toEqual(['Bar', 'Bar', 'callback']); }); it('updates a previous render', () => { @@ -140,15 +129,13 @@ describe('ReactIncremental', () => { }); it('can cancel partially rendered work and restart', () => { - let ops = []; - function Bar(props) { - ops.push('Bar'); + ReactNoop.yield('Bar'); return
{props.children}
; } function Foo(props) { - ops.push('Foo'); + ReactNoop.yield('Foo'); return (
{props.text} @@ -161,34 +148,22 @@ describe('ReactIncremental', () => { ReactNoop.render(); ReactNoop.flush(); - ops = []; - ReactNoop.render(); // Flush part of the work - ReactNoop.flushDeferredPri(20 + 5); - - expect(ops).toEqual(['Foo', 'Bar']); - - ops = []; + ReactNoop.flushThrough(['Foo', 'Bar']); // This will abort the previous work and restart ReactNoop.flushSync(() => ReactNoop.render(null)); ReactNoop.render(); - ReactNoop.clearYields(); // Flush part of the new work - ReactNoop.flushDeferredPri(20 + 5); - - expect(ops).toEqual(['Foo', 'Bar']); + ReactNoop.flushThrough(['Foo', 'Bar']); // Flush the rest of the work which now includes the low priority - ReactNoop.flush(20); - - expect(ops).toEqual(['Foo', 'Bar', 'Bar']); + expect(ReactNoop.flush()).toEqual(['Bar']); }); it('should call callbacks even if updates are aborted', () => { - const ops = []; let inst; class Foo extends React.Component { @@ -215,32 +190,27 @@ describe('ReactIncremental', () => { inst.setState( () => { - ops.push('setState1'); + ReactNoop.yield('setState1'); return {text: 'bar'}; }, - () => ops.push('callback1'), + () => ReactNoop.yield('callback1'), ); // Flush part of the work - ReactNoop.flushDeferredPri(20 + 5); - - expect(ops).toEqual(['setState1']); + ReactNoop.flushThrough(['setState1']); // This will abort the previous work and restart ReactNoop.flushSync(() => ReactNoop.render()); inst.setState( () => { - ops.push('setState2'); + ReactNoop.yield('setState2'); return {text2: 'baz'}; }, - () => ops.push('callback2'), + () => ReactNoop.yield('callback2'), ); // Flush the rest of the work which now includes the low priority - ReactNoop.flush(); - - expect(ops).toEqual([ - 'setState1', + expect(ReactNoop.flush()).toEqual([ 'setState1', 'setState2', 'callback1', @@ -250,20 +220,18 @@ describe('ReactIncremental', () => { }); it('can deprioritize unfinished work and resume it later', () => { - let ops = []; - function Bar(props) { - ops.push('Bar'); + ReactNoop.yield('Bar'); return
{props.children}
; } function Middle(props) { - ops.push('Middle'); + ReactNoop.yield('Middle'); return {props.children}; } function Foo(props) { - ops.push('Foo'); + ReactNoop.yield('Foo'); return (
{props.text} @@ -280,25 +248,20 @@ describe('ReactIncremental', () => { // Init ReactNoop.render(); - ReactNoop.flush(); - - expect(ops).toEqual(['Foo', 'Bar', 'Bar', 'Middle', 'Middle']); - - ops = []; + expect(ReactNoop.flush()).toEqual([ + 'Foo', + 'Bar', + 'Bar', + 'Middle', + 'Middle', + ]); // Render part of the work. This should be enough to flush everything except // the middle which has lower priority. ReactNoop.render(); - ReactNoop.flushDeferredPri(40); - - expect(ops).toEqual(['Foo', 'Bar', 'Bar']); - - ops = []; - + ReactNoop.flushThrough(['Foo', 'Bar', 'Bar']); // Flush only the remaining work - ReactNoop.flush(); - - expect(ops).toEqual(['Middle', 'Middle']); + expect(ReactNoop.flush()).toEqual(['Middle', 'Middle']); }); it('can deprioritize a tree from without dropping work', () => { @@ -1844,8 +1807,6 @@ describe('ReactIncremental', () => { }); it('merges and masks context', () => { - const ops = []; - class Intl extends React.Component { static childContextTypes = { locale: PropTypes.string, @@ -1856,7 +1817,7 @@ describe('ReactIncremental', () => { }; } render() { - ops.push('Intl ' + JSON.stringify(this.context)); + ReactNoop.yield('Intl ' + JSON.stringify(this.context)); return this.props.children; } } @@ -1871,7 +1832,7 @@ describe('ReactIncremental', () => { }; } render() { - ops.push('Router ' + JSON.stringify(this.context)); + ReactNoop.yield('Router ' + JSON.stringify(this.context)); return this.props.children; } } @@ -1881,7 +1842,7 @@ describe('ReactIncremental', () => { locale: PropTypes.string, }; render() { - ops.push('ShowLocale ' + JSON.stringify(this.context)); + ReactNoop.yield('ShowLocale ' + JSON.stringify(this.context)); return this.context.locale; } } @@ -1891,13 +1852,13 @@ describe('ReactIncremental', () => { route: PropTypes.string, }; render() { - ops.push('ShowRoute ' + JSON.stringify(this.context)); + ReactNoop.yield('ShowRoute ' + JSON.stringify(this.context)); return this.context.route; } } function ShowBoth(props, context) { - ops.push('ShowBoth ' + JSON.stringify(context)); + ReactNoop.yield('ShowBoth ' + JSON.stringify(context)); return `${context.route} in ${context.locale}`; } ShowBoth.contextTypes = { @@ -1907,14 +1868,14 @@ describe('ReactIncremental', () => { class ShowNeither extends React.Component { render() { - ops.push('ShowNeither ' + JSON.stringify(this.context)); + ReactNoop.yield('ShowNeither ' + JSON.stringify(this.context)); return null; } } class Indirection extends React.Component { render() { - ops.push('Indirection ' + JSON.stringify(this.context)); + ReactNoop.yield('Indirection ' + JSON.stringify(this.context)); return [ , , @@ -1927,7 +1888,6 @@ describe('ReactIncremental', () => { } } - ops.length = 0; ReactNoop.render( @@ -1936,18 +1896,18 @@ describe('ReactIncremental', () => {
, ); - expect(ReactNoop.flush).toWarnDev( + expect(() => + expect(ReactNoop.flush()).toEqual([ + 'Intl {}', + 'ShowLocale {"locale":"fr"}', + 'ShowBoth {"locale":"fr"}', + ]), + ).toWarnDev( 'Legacy context API has been detected within a strict-mode tree: \n\n' + 'Please update the following components: Intl, ShowBoth, ShowLocale', {withoutStack: true}, ); - expect(ops).toEqual([ - 'Intl {}', - 'ShowLocale {"locale":"fr"}', - 'ShowBoth {"locale":"fr"}', - ]); - ops.length = 0; ReactNoop.render( @@ -1956,14 +1916,12 @@ describe('ReactIncremental', () => {
, ); - ReactNoop.flush(); - expect(ops).toEqual([ + expect(ReactNoop.flush()).toEqual([ 'Intl {}', 'ShowLocale {"locale":"de"}', 'ShowBoth {"locale":"de"}', ]); - ops.length = 0; ReactNoop.render( @@ -1972,10 +1930,8 @@ describe('ReactIncremental', () => { , ); - ReactNoop.flushDeferredPri(15); - expect(ops).toEqual(['Intl {}']); + ReactNoop.flushThrough(['Intl {}']); - ops.length = 0; ReactNoop.render( @@ -1985,26 +1941,27 @@ describe('ReactIncremental', () => { , ); - expect(ReactNoop.flush).toWarnDev( + expect(() => + expect(ReactNoop.flush()).toEqual([ + 'ShowLocale {"locale":"sv"}', + 'ShowBoth {"locale":"sv"}', + 'Intl {}', + 'ShowLocale {"locale":"en"}', + 'Router {}', + 'Indirection {}', + 'ShowLocale {"locale":"en"}', + 'ShowRoute {"route":"/about"}', + 'ShowNeither {}', + 'Intl {}', + 'ShowBoth {"locale":"ru","route":"/about"}', + 'ShowBoth {"locale":"en","route":"/about"}', + 'ShowBoth {"locale":"en"}', + ]), + ).toWarnDev( 'Legacy context API has been detected within a strict-mode tree: \n\n' + 'Please update the following components: Router, ShowRoute', {withoutStack: true}, ); - expect(ops).toEqual([ - 'ShowLocale {"locale":"sv"}', - 'ShowBoth {"locale":"sv"}', - 'Intl {}', - 'ShowLocale {"locale":"en"}', - 'Router {}', - 'Indirection {}', - 'ShowLocale {"locale":"en"}', - 'ShowRoute {"route":"/about"}', - 'ShowNeither {}', - 'Intl {}', - 'ShowBoth {"locale":"ru","route":"/about"}', - 'ShowBoth {"locale":"en","route":"/about"}', - 'ShowBoth {"locale":"en"}', - ]); }); it('does not leak own context into context provider', () => { @@ -2080,8 +2037,6 @@ describe('ReactIncremental', () => { }); it('provides context when reusing work', () => { - const ops = []; - class Intl extends React.Component { static childContextTypes = { locale: PropTypes.string, @@ -2092,7 +2047,7 @@ describe('ReactIncremental', () => { }; } render() { - ops.push('Intl ' + JSON.stringify(this.context)); + ReactNoop.yield('Intl ' + JSON.stringify(this.context)); return this.props.children; } } @@ -2102,12 +2057,11 @@ describe('ReactIncremental', () => { locale: PropTypes.string, }; render() { - ops.push('ShowLocale ' + JSON.stringify(this.context)); + ReactNoop.yield('ShowLocale ' + JSON.stringify(this.context)); return this.context.locale; } } - ops.length = 0; ReactNoop.render( @@ -2120,24 +2074,23 @@ describe('ReactIncremental', () => { , ); - ReactNoop.flushDeferredPri(40); - expect(ops).toEqual([ + ReactNoop.flushThrough([ 'Intl {}', 'ShowLocale {"locale":"fr"}', 'ShowLocale {"locale":"fr"}', ]); - ops.length = 0; - expect(ReactNoop.flush).toWarnDev( + expect(() => + expect(ReactNoop.flush()).toEqual([ + 'ShowLocale {"locale":"fr"}', + 'Intl {}', + 'ShowLocale {"locale":"ru"}', + ]), + ).toWarnDev( 'Legacy context API has been detected within a strict-mode tree: \n\n' + 'Please update the following components: Intl, ShowLocale', {withoutStack: true}, ); - expect(ops).toEqual([ - 'ShowLocale {"locale":"fr"}', - 'Intl {}', - 'ShowLocale {"locale":"ru"}', - ]); }); it('reads context when setState is below the provider', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 43d2570c99f8..f4a24cfc0ae4 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -318,7 +318,7 @@ describe('ReactIncrementalErrorHandling', () => { ); } - ReactNoop.render(); + ReactNoop.render(, () => ReactNoop.yield('commit')); // Render the bad component asynchronously ReactNoop.flushThrough(['Parent', 'BadRender']); @@ -326,10 +326,9 @@ describe('ReactIncrementalErrorHandling', () => { // Finish the rest of the async work ReactNoop.flushThrough(['Sibling']); - // Rendering two more units of work should be enough to trigger the retry - // and synchronously throw an error. + // React retries once, synchronously, before throwing. ops = []; - expect(() => ReactNoop.flushUnitsOfWork(2)).toThrow('oops'); + expect(() => ReactNoop.flushNextYield()).toThrow('oops'); expect(ops).toEqual(['Parent', 'BadRender', 'Sibling']); }); @@ -409,7 +408,7 @@ describe('ReactIncrementalErrorHandling', () => { , ); - ReactNoop.flushDeferredPri(); + ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Caught an error: Hello.')]); }); @@ -588,20 +587,19 @@ describe('ReactIncrementalErrorHandling', () => { }); it('propagates an error from a noop error boundary during partial deferred mounting', () => { - const ops = []; class RethrowErrorBoundary extends React.Component { componentDidCatch(error) { - ops.push('RethrowErrorBoundary componentDidCatch'); + ReactNoop.yield('RethrowErrorBoundary componentDidCatch'); throw error; } render() { - ops.push('RethrowErrorBoundary render'); + ReactNoop.yield('RethrowErrorBoundary render'); return this.props.children; } } function BrokenRender() { - ops.push('BrokenRender'); + ReactNoop.yield('BrokenRender'); throw new Error('Hello'); } @@ -611,16 +609,12 @@ describe('ReactIncrementalErrorHandling', () => { , ); - ReactNoop.flushDeferredPri(15); - expect(ops).toEqual(['RethrowErrorBoundary render']); + ReactNoop.flushThrough(['RethrowErrorBoundary render']); - ops.length = 0; expect(() => { ReactNoop.flush(); }).toThrow('Hello'); - expect(ops).toEqual([ - 'BrokenRender', - + expect(ReactNoop.clearYields()).toEqual([ // React retries one more time 'RethrowErrorBoundary render', 'BrokenRender', @@ -1526,7 +1520,7 @@ describe('ReactIncrementalErrorHandling', () => { , ); - ReactNoop.flushDeferredPri(); + ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([ span( 'Caught an error:\n' + @@ -1562,7 +1556,7 @@ describe('ReactIncrementalErrorHandling', () => { , ); - ReactNoop.flushDeferredPri(); + ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Caught an error: Hello')]); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js index 06f38de82910..4394b2822e79 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js @@ -50,7 +50,7 @@ describe('ReactIncrementalErrorLogging', () => {
, ); - expect(ReactNoop.flushDeferredPri).toThrowError('constructor error'); + expect(() => ReactNoop.flush()).toThrowError('constructor error'); expect(console.error).toHaveBeenCalledTimes(1); expect(console.error).toHaveBeenCalledWith( __DEV__ @@ -86,7 +86,7 @@ describe('ReactIncrementalErrorLogging', () => { , ); - expect(ReactNoop.flushDeferredPri).toThrowError('componentDidMount error'); + expect(() => ReactNoop.flush()).toThrowError('componentDidMount error'); expect(console.error).toHaveBeenCalledTimes(1); expect(console.error).toHaveBeenCalledWith( __DEV__ @@ -125,7 +125,7 @@ describe('ReactIncrementalErrorLogging', () => { , ); - expect(ReactNoop.flushDeferredPri).toThrow('render error'); + expect(() => ReactNoop.flush()).toThrow('render error'); expect(logCapturedErrorCalls.length).toBe(1); expect(logCapturedErrorCalls[0]).toEqual( __DEV__ diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js index 01a462debb6f..c3b18696380b 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js @@ -117,6 +117,7 @@ describe('ReactDebugFiberPerf', () => { require('shared/ReactFeatureFlags').enableUserTimingAPI = true; require('shared/ReactFeatureFlags').enableProfilerTimer = false; require('shared/ReactFeatureFlags').replayFailedUnitOfWorkWithInvokeGuardedCallback = false; + require('shared/ReactFeatureFlags').debugRenderPhaseSideEffectsForStrictMode = false; // Import after the polyfill is set up: React = require('react'); @@ -397,12 +398,21 @@ describe('ReactDebugFiberPerf', () => { it('measures deferred work in chunks', () => { class A extends React.Component { render() { + ReactNoop.yield('A'); return
{this.props.children}
; } } class B extends React.Component { render() { + ReactNoop.yield('B'); + return
{this.props.children}
; + } + } + + class C extends React.Component { + render() { + ReactNoop.yield('C'); return
{this.props.children}
; } } @@ -415,14 +425,15 @@ describe('ReactDebugFiberPerf', () => { + + + , ); - addComment('Start mounting Parent and A'); - ReactNoop.flushDeferredPri(40); - addComment('Mount B just a little (but not enough to memoize)'); - ReactNoop.flushDeferredPri(10); - addComment('Complete B and Parent'); - ReactNoop.flushDeferredPri(); + addComment('Start rendering through B'); + ReactNoop.flushThrough(['A', 'B']); + addComment('Complete the rest'); + ReactNoop.flush(); expect(getFlameChart()).toMatchSnapshot(); }); @@ -632,11 +643,12 @@ describe('ReactDebugFiberPerf', () => { it('warns if an in-progress update is interrupted', () => { function Foo() { + ReactNoop.yield('Foo'); return ; } ReactNoop.render(); - ReactNoop.flushUnitsOfWork(2); + ReactNoop.flushNextYield(); ReactNoop.flushSync(() => { ReactNoop.render(); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalReflection-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalReflection-test.internal.js index fa3dc6e7e013..c82b2f990ef2 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalReflection-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalReflection-test.internal.js @@ -24,8 +24,6 @@ describe('ReactIncrementalReflection', () => { }); it('handles isMounted even when the initial render is deferred', () => { - let ops = []; - const instances = []; class Component extends React.Component { @@ -36,10 +34,10 @@ describe('ReactIncrementalReflection', () => { } UNSAFE_componentWillMount() { instances.push(this); - ops.push('componentWillMount', this._isMounted()); + ReactNoop.yield('componentWillMount: ' + this._isMounted()); } componentDidMount() { - ops.push('componentDidMount', this._isMounted()); + ReactNoop.yield('componentDidMount: ' + this._isMounted()); } render() { return ; @@ -53,29 +51,23 @@ describe('ReactIncrementalReflection', () => { ReactNoop.render(); // Render part way through but don't yet commit the updates. - ReactNoop.flushDeferredPri(20); - - expect(ops).toEqual(['componentWillMount', false]); + ReactNoop.flushThrough(['componentWillMount: false']); expect(instances[0]._isMounted()).toBe(false); - ops = []; - // Render the rest and commit the updates. - expect(ReactNoop.flush).toWarnDev( + expect(() => + expect(ReactNoop.flush()).toEqual(['componentDidMount: true']), + ).toWarnDev( 'componentWillMount: Please update the following components ' + 'to use componentDidMount instead: Component', {withoutStack: true}, ); - expect(ops).toEqual(['componentDidMount', true]); - expect(instances[0]._isMounted()).toBe(true); }); it('handles isMounted when an unmount is deferred', () => { - let ops = []; - const instances = []; class Component extends React.Component { @@ -86,16 +78,16 @@ describe('ReactIncrementalReflection', () => { instances.push(this); } componentWillUnmount() { - ops.push('componentWillUnmount', this._isMounted()); + ReactNoop.yield('componentWillUnmount: ' + this._isMounted()); } render() { - ops.push('Component'); + ReactNoop.yield('Component'); return ; } } function Other() { - ops.push('Other'); + ReactNoop.yield('Other'); return ; } @@ -104,38 +96,28 @@ describe('ReactIncrementalReflection', () => { } ReactNoop.render(); - expect(ReactNoop.flush).toWarnDev( + expect(() => expect(ReactNoop.flush()).toEqual(['Component'])).toWarnDev( 'componentWillMount: Please update the following components ' + 'to use componentDidMount instead: Component', {withoutStack: true}, ); - expect(ops).toEqual(['Component']); - ops = []; - expect(instances[0]._isMounted()).toBe(true); ReactNoop.render(); // Render part way through but don't yet commit the updates so it is not // fully unmounted yet. - ReactNoop.flushDeferredPri(20); - - expect(ops).toEqual(['Other']); - ops = []; + ReactNoop.flushThrough(['Other']); expect(instances[0]._isMounted()).toBe(true); // Finish flushing the unmount. - ReactNoop.flush(); - - expect(ops).toEqual(['componentWillUnmount', true]); + expect(ReactNoop.flush()).toEqual(['componentWillUnmount: true']); expect(instances[0]._isMounted()).toBe(false); }); it('finds no node before insertion and correct node before deletion', () => { - let ops = []; - let classInstance = null; function findInstance(inst) { @@ -154,22 +136,22 @@ describe('ReactIncrementalReflection', () => { class Component extends React.Component { UNSAFE_componentWillMount() { classInstance = this; - ops.push('componentWillMount', findInstance(this)); + ReactNoop.yield(['componentWillMount', findInstance(this)]); } componentDidMount() { - ops.push('componentDidMount', findInstance(this)); + ReactNoop.yield(['componentDidMount', findInstance(this)]); } UNSAFE_componentWillUpdate() { - ops.push('componentWillUpdate', findInstance(this)); + ReactNoop.yield(['componentWillUpdate', findInstance(this)]); } componentDidUpdate() { - ops.push('componentDidUpdate', findInstance(this)); + ReactNoop.yield(['componentDidUpdate', findInstance(this)]); } componentWillUnmount() { - ops.push('componentWillUnmount', findInstance(this)); + ReactNoop.yield(['componentWillUnmount', findInstance(this)]); } render() { - ops.push('render'); + ReactNoop.yield('render'); return this.props.step < 2 ? ( (this.span = ref)} /> ) : this.props.step === 2 ? ( @@ -182,7 +164,7 @@ describe('ReactIncrementalReflection', () => { function Sibling() { // Sibling is used to assert that we've rendered past the first component. - ops.push('render sibling'); + ReactNoop.yield('render sibling'); return ; } @@ -192,23 +174,22 @@ describe('ReactIncrementalReflection', () => { ReactNoop.render(); // Flush past Component but don't complete rendering everything yet. - ReactNoop.flushDeferredPri(30); - - expect(ops).toEqual([ - 'componentWillMount', - null, + ReactNoop.flushThrough([ + ['componentWillMount', null], 'render', 'render sibling', ]); - ops = []; - expect(classInstance).toBeDefined(); // The instance has been complete but is still not committed so it should // not find any host nodes in it. expect(findInstance(classInstance)).toBe(null); - expect(ReactNoop.flush).toWarnDev( + expect(() => + expect(ReactNoop.flush()).toEqual([ + ['componentDidMount', classInstance.span], + ]), + ).toWarnDev( 'componentWillMount: Please update the following components ' + 'to use componentDidMount instead: Component' + '\n\ncomponentWillUpdate: Please update the following components ' + @@ -221,79 +202,54 @@ describe('ReactIncrementalReflection', () => { expect(findInstance(classInstance)).toBe(hostSpan); - expect(ops).toEqual(['componentDidMount', hostSpan]); - - ops = []; - // Flush next step which will cause an update but not yet render a new host // node. ReactNoop.render(); - ReactNoop.flush(); - - expect(ops).toEqual([ - 'componentWillUpdate', - hostSpan, + expect(ReactNoop.flush()).toEqual([ + ['componentWillUpdate', hostSpan], 'render', 'render sibling', - 'componentDidUpdate', - hostSpan, + ['componentDidUpdate', hostSpan], ]); expect(ReactNoop.findInstance(classInstance)).toBe(hostSpan); - ops = []; - // The next step will render a new host node but won't get committed yet. // We expect this to mutate the original Fiber. ReactNoop.render(); - ReactNoop.flushDeferredPri(30); - - expect(ops).toEqual([ - 'componentWillUpdate', - hostSpan, + ReactNoop.flushThrough([ + ['componentWillUpdate', hostSpan], 'render', 'render sibling', ]); - ops = []; - // This should still be the host span. expect(ReactNoop.findInstance(classInstance)).toBe(hostSpan); // When we finally flush the tree it will get committed. - ReactNoop.flush(); + expect(ReactNoop.flush()).toEqual([ + ['componentDidUpdate', classInstance.div], + ]); const hostDiv = classInstance.div; - expect(hostDiv).toBeDefined(); expect(hostSpan).not.toBe(hostDiv); - expect(ops).toEqual(['componentDidUpdate', hostDiv]); - - ops = []; - // We should now find the new host node. expect(ReactNoop.findInstance(classInstance)).toBe(hostDiv); // Render to null but don't commit it yet. ReactNoop.render(); - ReactNoop.flushDeferredPri(25); - - expect(ops).toEqual([ - 'componentWillUpdate', - hostDiv, + ReactNoop.flushThrough([ + ['componentWillUpdate', hostDiv], 'render', 'render sibling', ]); - ops = []; - // This should still be the host div since the deletion is not committed. expect(ReactNoop.findInstance(classInstance)).toBe(hostDiv); - ReactNoop.flush(); - - expect(ops).toEqual(['componentDidUpdate', null]); + expect(ReactNoop.flush()).toEqual([['componentDidUpdate', null]]); // This should still be the host div since the deletion is not committed. expect(ReactNoop.findInstance(classInstance)).toBe(null); @@ -302,11 +258,8 @@ describe('ReactIncrementalReflection', () => { ReactNoop.render(); ReactNoop.flush(); - ops = []; - // Unmount the component. ReactNoop.render([]); - ReactNoop.flush(); - expect(ops).toEqual(['componentWillUnmount', hostDiv]); + expect(ReactNoop.flush()).toEqual([['componentWillUnmount', hostDiv]]); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js index b362585a5a5c..353a20ce65b2 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js @@ -31,7 +31,7 @@ describe('ReactIncrementalScheduling', () => { ReactNoop.render(); expect(ReactNoop.getChildren()).toEqual([]); - ReactNoop.flushDeferredPri(); + ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('1')]); }); @@ -86,71 +86,99 @@ describe('ReactIncrementalScheduling', () => { }); it('works on deferred roots in the order they were scheduled', () => { - ReactNoop.renderToRootWithID(, 'a'); - ReactNoop.renderToRootWithID(, 'b'); - ReactNoop.renderToRootWithID(, 'c'); + const {useEffect} = React; + function Text({text}) { + useEffect( + () => { + ReactNoop.yield(text); + }, + [text], + ); + return text; + } + + ReactNoop.act(() => { + ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.renderToRootWithID(, 'b'); + ReactNoop.renderToRootWithID(, 'c'); + }); ReactNoop.flush(); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:1')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:1')]); + + expect(ReactNoop.getChildrenAsJSX('a')).toEqual('a:1'); + expect(ReactNoop.getChildrenAsJSX('b')).toEqual('b:1'); + expect(ReactNoop.getChildrenAsJSX('c')).toEqual('c:1'); // Schedule deferred work in the reverse order - ReactNoop.renderToRootWithID(, 'c'); - ReactNoop.renderToRootWithID(, 'b'); + ReactNoop.act(() => { + ReactNoop.renderToRootWithID(, 'c'); + ReactNoop.renderToRootWithID(, 'b'); + }); // Ensure it starts in the order it was scheduled - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:1')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); + ReactNoop.flushThrough(['c:2']); + + expect(ReactNoop.getChildrenAsJSX('a')).toEqual('a:1'); + expect(ReactNoop.getChildrenAsJSX('b')).toEqual('b:1'); + expect(ReactNoop.getChildrenAsJSX('c')).toEqual('c:2'); // Schedule last bit of work, it will get processed the last - ReactNoop.renderToRootWithID(, 'a'); + ReactNoop.act(() => { + ReactNoop.renderToRootWithID(, 'a'); + }); // Keep performing work in the order it was scheduled - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:1')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); - ReactNoop.flushDeferredPri(15 + 5); - expect(ReactNoop.getChildren('a')).toEqual([span('a:2')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:2')]); + ReactNoop.flushThrough(['b:2']); + expect(ReactNoop.getChildrenAsJSX('a')).toEqual('a:1'); + expect(ReactNoop.getChildrenAsJSX('b')).toEqual('b:2'); + expect(ReactNoop.getChildrenAsJSX('c')).toEqual('c:2'); + + ReactNoop.flushThrough(['a:2']); + expect(ReactNoop.getChildrenAsJSX('a')).toEqual('a:2'); + expect(ReactNoop.getChildrenAsJSX('b')).toEqual('b:2'); + expect(ReactNoop.getChildrenAsJSX('c')).toEqual('c:2'); }); it('schedules sync updates when inside componentDidMount/Update', () => { let instance; - let ops = []; class Foo extends React.Component { state = {tick: 0}; componentDidMount() { - ops.push('componentDidMount (before setState): ' + this.state.tick); + ReactNoop.yield( + 'componentDidMount (before setState): ' + this.state.tick, + ); this.setState({tick: 1}); // We're in a batch. Update hasn't flushed yet. - ops.push('componentDidMount (after setState): ' + this.state.tick); + ReactNoop.yield( + 'componentDidMount (after setState): ' + this.state.tick, + ); } componentDidUpdate() { - ops.push('componentDidUpdate: ' + this.state.tick); + ReactNoop.yield('componentDidUpdate: ' + this.state.tick); if (this.state.tick === 2) { - ops.push('componentDidUpdate (before setState): ' + this.state.tick); + ReactNoop.yield( + 'componentDidUpdate (before setState): ' + this.state.tick, + ); this.setState({tick: 3}); - ops.push('componentDidUpdate (after setState): ' + this.state.tick); + ReactNoop.yield( + 'componentDidUpdate (after setState): ' + this.state.tick, + ); // We're in a batch. Update hasn't flushed yet. } } render() { - ops.push('render: ' + this.state.tick); + ReactNoop.yield('render: ' + this.state.tick); instance = this; return ; } } ReactNoop.render(); + // Render without committing + ReactNoop.flushThrough(['render: 0']); - ReactNoop.flushDeferredPri(20 + 5); - expect(ops).toEqual([ - 'render: 0', + // Do one more unit of work to commit + expect(ReactNoop.flushNextYield()).toEqual([ 'componentDidMount (before setState): 0', 'componentDidMount (after setState): 0', // If the setState inside componentDidMount were deferred, there would be @@ -159,12 +187,9 @@ describe('ReactIncrementalScheduling', () => { 'componentDidUpdate: 1', ]); - ops = []; instance.setState({tick: 2}); - ReactNoop.flushDeferredPri(20 + 5); - - expect(ops).toEqual([ - 'render: 2', + ReactNoop.flushThrough(['render: 2']); + expect(ReactNoop.flushNextYield()).toEqual([ 'componentDidUpdate: 2', 'componentDidUpdate (before setState): 2', 'componentDidUpdate (after setState): 2', @@ -254,17 +279,18 @@ describe('ReactIncrementalScheduling', () => { }); } render() { + ReactNoop.yield('Foo'); return ; } } ReactNoop.render(); // This should be just enough to complete all the work, but not enough to // commit it. - ReactNoop.flushDeferredPri(20); + ReactNoop.flushThrough(['Foo']); expect(ReactNoop.getChildren()).toEqual([]); // Do one more unit of work. - ReactNoop.flushDeferredPri(10); + ReactNoop.flushNextYield(); // The updates should all be flushed with Task priority expect(ReactNoop.getChildren()).toEqual([span(5)]); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js index f5195298614e..9e14dd202fb5 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js @@ -373,10 +373,12 @@ describe('ReactIncrementalSideEffects', () => { it('does not update child nodes if a flush is aborted', () => { function Bar(props) { + ReactNoop.yield('Bar'); return ; } function Foo(props) { + ReactNoop.yield('Foo'); return (
@@ -395,7 +397,9 @@ describe('ReactIncrementalSideEffects', () => { ]); ReactNoop.render(); - ReactNoop.flushDeferredPri(35); + + // Flush some of the work without committing + ReactNoop.flushThrough(['Foo', 'Bar']); expect(ReactNoop.getChildren()).toEqual([ div(div(span('Hello'), span('Hello')), span('Yo')), ]); @@ -403,10 +407,12 @@ describe('ReactIncrementalSideEffects', () => { it('preserves a previously rendered node when deprioritized', () => { function Middle(props) { + ReactNoop.yield('Middle'); return ; } function Foo(props) { + ReactNoop.yield('Foo'); return (
, ); - ReactNoop.render(); - ReactNoop.flushDeferredPri(20); - + ReactNoop.render(, () => ReactNoop.yield('commit')); + ReactNoop.flushThrough(['Foo', 'commit']); expect(ReactNoop.getChildrenAsJSX()).toEqual(
, ); - ReactNoop.flush(); + ReactNoop.flush(); expect(ReactNoop.getChildrenAsJSX()).toEqual(