From a930f09dfe6f96347b035e6500b3b13e36e3c7b2 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 7 Dec 2016 18:51:21 -0800 Subject: [PATCH 1/2] Give setState callbacks componentWillUpdate semantics This matches the behavior in Fiber. Normally we would change Fiber to match Stack to minimize breaking changes for the initial release. However, in this case it would require too large a compromise to change Fiber to act like Stack. --- scripts/fiber/tests-failing.txt | 3 --- scripts/fiber/tests-passing.txt | 3 +++ ...ReactCompositeComponentNestedState-test.js | 2 +- .../reconciler/ReactCompositeComponent.js | 24 +++++++++++++++++++ .../shared/stack/reconciler/ReactUpdates.js | 15 ------------ 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 8d705c30e8..254abf87b2 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -82,9 +82,6 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js * should warn about `setState` in getChildContext * should update refs if shouldComponentUpdate gives false -src/renderers/shared/shared/__tests__/ReactCompositeComponentNestedState-test.js -* should provide up to date values for props - src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js * should update state when called from child cWRP diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 002f3c042f..e48c182712 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1323,6 +1323,9 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponentDOMMinimalism-test. * should not render extra nodes for non-interpolated text * should not render extra nodes for non-interpolated text +src/renderers/shared/shared/__tests__/ReactCompositeComponentNestedState-test.js +* should provide up to date values for props + src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js * should support setting state * should call componentDidUpdate of children first diff --git a/src/renderers/shared/shared/__tests__/ReactCompositeComponentNestedState-test.js b/src/renderers/shared/shared/__tests__/ReactCompositeComponentNestedState-test.js index b44bac3139..1c95f72293 100644 --- a/src/renderers/shared/shared/__tests__/ReactCompositeComponentNestedState-test.js +++ b/src/renderers/shared/shared/__tests__/ReactCompositeComponentNestedState-test.js @@ -111,8 +111,8 @@ describe('ReactCompositeComponentNestedState-state', () => { ['setState-this', 'dark blue', 'blue'], ['setState-args', 'dark blue', 'green'], ['render', 'light green', 'green'], - ['parent-after-setState', 'green'], ['after-setState', 'light green', 'green'], + ['parent-after-setState', 'green'], ]); }); }); diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 2a15d71383..0064b18d3d 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -780,6 +780,16 @@ var ReactCompositeComponent = { this._context ); } else { + var callbacks = this._pendingCallbacks; + this._pendingCallbacks = null; + if (callbacks) { + for (var j = 0; j < callbacks.length; j++) { + transaction.getReactMountReady().enqueue( + callbacks[j], + this.getPublicInstance() + ); + } + } this._updateBatchNumber = null; } }, @@ -848,6 +858,11 @@ var ReactCompositeComponent = { } } + // If updating happens to enqueue any new updates, we shouldn't execute new + // callbacks until the next render happens, so stash the callbacks first. + var callbacks = this._pendingCallbacks; + this._pendingCallbacks = null; + var nextState = this._processPendingState(nextProps, nextContext); var shouldUpdate = true; @@ -901,6 +916,15 @@ var ReactCompositeComponent = { inst.state = nextState; inst.context = nextContext; } + + if (callbacks) { + for (var j = 0; j < callbacks.length; j++) { + transaction.getReactMountReady().enqueue( + callbacks[j], + this.getPublicInstance() + ); + } + } }, _processPendingState: function(props, context) { diff --git a/src/renderers/shared/stack/reconciler/ReactUpdates.js b/src/renderers/shared/stack/reconciler/ReactUpdates.js index 2e44516430..8971a7c357 100644 --- a/src/renderers/shared/stack/reconciler/ReactUpdates.js +++ b/src/renderers/shared/stack/reconciler/ReactUpdates.js @@ -135,12 +135,6 @@ function runBatchedUpdates(transaction) { // that performUpdateIfNecessary is a noop. var component = dirtyComponents[i]; - // If performUpdateIfNecessary happens to enqueue any new updates, we - // shouldn't execute the callbacks until the next render happens, so - // stash the callbacks first - var callbacks = component._pendingCallbacks; - component._pendingCallbacks = null; - var markerName; if (ReactFeatureFlags.logTopLevelRenders) { var namedComponent = component; @@ -161,15 +155,6 @@ function runBatchedUpdates(transaction) { if (markerName) { console.timeEnd(markerName); } - - if (callbacks) { - for (var j = 0; j < callbacks.length; j++) { - transaction.reconcileTransaction.getReactMountReady().enqueue( - callbacks[j], - component.getPublicInstance() - ); - } - } } } From 97b7d0eff5e25ab9efd6f17e9a51dd378a725f54 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 8 Dec 2016 09:36:04 -0800 Subject: [PATCH 2/2] Test top-level callback when re-rendering with same element --- scripts/fiber/tests-passing.txt | 1 + .../dom/fiber/__tests__/ReactDOMFiber-test.js | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index e48c182712..2c127f082d 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -505,6 +505,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should render strings as children * should render numbers as children * should be called a callback argument +* should call a callback argument when the same element is re-rendered * should render a component returning strings directly from render * should render a component returning numbers directly from render * finds the DOM Text node of a string child diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 6dd3146541..990ad069d1 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -64,6 +64,35 @@ describe('ReactDOMFiber', () => { expect(called).toEqual(true); }); + it('should call a callback argument when the same element is re-rendered', () => { + class Foo extends React.Component { + render() { + return
Foo
; + } + } + const element = ; + + // mounting phase + let called = false; + ReactDOM.render( + element, + container, + () => called = true + ); + expect(called).toEqual(true); + + // updating phase + called = false; + ReactDOM.unstable_batchedUpdates(() => { + ReactDOM.render( + element, + container, + () => called = true + ); + }); + expect(called).toEqual(true); + }); + if (ReactDOMFeatureFlags.useFiber) { it('should render a component returning strings directly from render', () => { const Text = ({value}) => value;