From f829e2f1f1d950db7d588a5dd288d3e39e074d4d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 8 Nov 2016 13:47:31 -0800 Subject: [PATCH] Meant to commit these changes with #8206... oops. --- scripts/fiber/tests-failing.txt | 3 - scripts/fiber/tests-passing.txt | 4 +- .../shared/fiber/ReactFiberScheduler.js | 7 +- .../ReactIncrementalScheduling-test.js | 138 ++++++++++++++++++ .../ReactIncrementalSideEffects-test.js | 60 -------- 5 files changed, 146 insertions(+), 66 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index c40f30eae8..ab6ebb9e4c 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -72,9 +72,6 @@ src/renderers/art/__tests__/ReactART-test.js * resolves refs before componentDidMount * resolves refs before componentDidUpdate -src/renderers/dom/__tests__/ReactDOMProduction-test.js -* should throw with an error code in production - src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js * should set style attribute when styles exist * should warn when using hyphenated style names diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 58e49f772f..bf71b6329a 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -453,6 +453,7 @@ src/renderers/dom/__tests__/ReactDOMProduction-test.js * should use prod React * should handle a simple flow * should call lifecycle methods +* should throw with an error code in production src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render strings as children @@ -842,6 +843,8 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js * splits deferred work on multiple roots * works on deferred roots in the order they were scheduled * handles interleaved deferred and animation work +* schedules sync updates when inside componentDidMount/Update +* can opt-in to deferred/animation scheduling inside componentDidMount/Update src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js * can update child nodes of a host instance @@ -862,7 +865,6 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js * calls setState callback even if component bails out * calls componentWillUnmount after a deletion, even if nested * calls componentDidMount/Update after insertion/update -* schedules sync updates when inside componentDidMount/Update * invokes ref callbacks after insertion/update/unmount * supports string refs diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index c69395a418..71aff51a26 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -211,8 +211,11 @@ module.exports = function(config : HostConfig) { const current = effectfulFiber.alternate; const previousPriorityContext = priorityContext; priorityContext = TaskPriority; - commitLifeCycles(current, effectfulFiber); - priorityContext = previousPriorityContext; + try { + commitLifeCycles(current, effectfulFiber); + } finally { + priorityContext = previousPriorityContext; + } } const next = effectfulFiber.nextEffect; // Ensure that we clean these up so that we don't accidentally keep them. diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js index 5808ab4c66..e272302e2b 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js @@ -449,4 +449,142 @@ describe('ReactIncrementalScheduling', () => { expect(ReactNoop.getChildren('b')).toEqual([span('b:3')]); expect(ReactNoop.getChildren('c')).toEqual([span('c:3')]); }); + + it('schedules sync updates when inside componentDidMount/Update', () => { + var instance; + var ops = []; + + class Foo extends React.Component { + state = { tick: 0 }; + + componentDidMount() { + ops.push('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); + } + + componentDidUpdate() { + ops.push('componentDidUpdate: ' + this.state.tick); + if (this.state.tick === 2) { + ops.push('componentDidUpdate (before setState): ' + this.state.tick); + this.setState({ tick: 3 }); + ops.push('componentDidUpdate (after setState): ' + this.state.tick); + // We're in a batch. Update hasn't flushed yet. + } + } + + render() { + ops.push('render: ' + this.state.tick); + instance = this; + return ; + } + } + + ReactNoop.render(); + + ReactNoop.flushDeferredPri(20); + expect(ops).toEqual([ + 'render: 0', + 'componentDidMount (before setState): 0', + 'componentDidMount (after setState): 0', + // If the setState inside componentDidMount were deferred, there would be + // no more ops. Because it has Task priority, we get these ops, too: + 'render: 1', + 'componentDidUpdate: 1', + ]); + + ops = []; + instance.setState({ tick: 2 }); + ReactNoop.flushDeferredPri(20); + + expect(ops).toEqual([ + 'render: 2', + 'componentDidUpdate: 2', + 'componentDidUpdate (before setState): 2', + 'componentDidUpdate (after setState): 2', + // If the setState inside componentDidUpdate were deferred, there would be + // no more ops. Because it has Task priority, we get these ops, too: + 'render: 3', + 'componentDidUpdate: 3', + ]); + }); + + it('can opt-in to deferred/animation scheduling inside componentDidMount/Update', () => { + var instance; + var ops = []; + + class Foo extends React.Component { + state = { tick: 0 }; + + componentDidMount() { + ReactNoop.performAnimationWork(() => { + ops.push('componentDidMount (before setState): ' + this.state.tick); + this.setState({ tick: 1 }); + ops.push('componentDidMount (after setState): ' + this.state.tick); + }); + } + + componentDidUpdate() { + ReactNoop.performAnimationWork(() => { + ops.push('componentDidUpdate: ' + this.state.tick); + if (this.state.tick === 2) { + ops.push('componentDidUpdate (before setState): ' + this.state.tick); + this.setState({ tick: 3 }); + ops.push('componentDidUpdate (after setState): ' + this.state.tick); + } + }); + } + + render() { + ops.push('render: ' + this.state.tick); + instance = this; + return ; + } + } + + ReactNoop.render(); + + ReactNoop.flushDeferredPri(20); + expect(ops).toEqual([ + 'render: 0', + 'componentDidMount (before setState): 0', + 'componentDidMount (after setState): 0', + // Following items shouldn't appear because they are the result of an + // update scheduled with animation priority + // 'render: 1', + // 'componentDidUpdate: 1', + ]); + + ops = []; + + ReactNoop.flushAnimationPri(); + expect(ops).toEqual([ + 'render: 1', + 'componentDidUpdate: 1', + ]); + + ops = []; + instance.setState({ tick: 2 }); + ReactNoop.flushDeferredPri(20); + + expect(ops).toEqual([ + 'render: 2', + 'componentDidUpdate: 2', + 'componentDidUpdate (before setState): 2', + 'componentDidUpdate (after setState): 2', + // Following items shouldn't appear because they are the result of an + // update scheduled with animation priority + // 'render: 3', + // 'componentDidUpdate: 3', + ]); + + ops = []; + + ReactNoop.flushAnimationPri(); + expect(ops).toEqual([ + 'render: 3', + 'componentDidUpdate: 3', + ]); + }); }); diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index a9294e2501..9e64dc43c1 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -1036,66 +1036,6 @@ describe('ReactIncrementalSideEffects', () => { }); - it('schedules sync updates when inside componentDidMount/Update', () => { - var instance; - var ops = []; - - class Foo extends React.Component { - state = { tick: 0 }; - - componentDidMount() { - ops.push('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); - } - - componentDidUpdate() { - ops.push('componentDidUpdate: ' + this.state.tick); - if (this.state.tick === 2) { - ops.push('componentDidUpdate (before setState): ' + this.state.tick); - this.setState({ tick: 3 }); - ops.push('componentDidUpdate (after setState): ' + this.state.tick); - // We're in a batch. Update hasn't flushed yet. - } - } - - render() { - ops.push('render: ' + this.state.tick); - instance = this; - return ; - } - } - - ReactNoop.render(); - - ReactNoop.flushDeferredPri(20); - expect(ops).toEqual([ - 'render: 0', - 'componentDidMount (before setState): 0', - 'componentDidMount (after setState): 0', - // If the setState inside componentDidMount were deferred, there would be - // no more ops. Because it has Task priority, we get these ops, too: - 'render: 1', - 'componentDidUpdate: 1', - ]); - - ops = []; - instance.setState({ tick: 2 }); - ReactNoop.flushDeferredPri(20); - - expect(ops).toEqual([ - 'render: 2', - 'componentDidUpdate: 2', - 'componentDidUpdate (before setState): 2', - 'componentDidUpdate (after setState): 2', - // If the setState inside componentDidUpdate were deferred, there would be - // no more ops. Because it has Task priority, we get these ops, too: - 'render: 3', - 'componentDidUpdate: 3', - ]); - }); - it('invokes ref callbacks after insertion/update/unmount', () => { var classInstance = null;