From 12b532c253e6efc3d077b35f9c3aa88ed94fc435 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sat, 28 Jun 2014 16:46:31 -0700 Subject: [PATCH 1/2] Add ReactUpdates.setImmediate for async callbacks Callbacks passed to this setImmediate function are called at the end of the current update cycle, which is guaranteed to be asynchronous but in the same event loop (with the default batching strategy). This is useful for new-style refs (#1373, #1554) and for fixing #1698. Test Plan: jest --- src/core/ReactUpdates.js | 41 +++++++++++--- src/core/__tests__/ReactUpdates-test.js | 75 +++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 7 deletions(-) diff --git a/src/core/ReactUpdates.js b/src/core/ReactUpdates.js index 9f8f9bf89e7..8bad4f43094 100644 --- a/src/core/ReactUpdates.js +++ b/src/core/ReactUpdates.js @@ -29,6 +29,8 @@ var mixInto = require('mixInto'); var warning = require('warning'); var dirtyComponents = []; +var setImmediateCallbackQueue = CallbackQueue.getPooled(); +var setImmediateEnqueued = false; var batchingStrategy = null; @@ -73,7 +75,7 @@ var TRANSACTION_WRAPPERS = [NESTED_UPDATES, UPDATE_QUEUEING]; function ReactUpdatesFlushTransaction() { this.reinitializeTransaction(); this.dirtyComponentsLength = null; - this.callbackQueue = CallbackQueue.getPooled(null); + this.callbackQueue = CallbackQueue.getPooled(); this.reconcileTransaction = ReactUpdates.ReactReconcileTransaction.getPooled(); } @@ -170,11 +172,21 @@ var flushBatchedUpdates = ReactPerf.measure( // ReactUpdatesFlushTransaction's wrappers will clear the dirtyComponents // array and perform any updates enqueued by mount-ready handlers (i.e., // componentDidUpdate) but we need to check here too in order to catch - // updates enqueued by setState callbacks. - while (dirtyComponents.length) { - var transaction = ReactUpdatesFlushTransaction.getPooled(); - transaction.perform(runBatchedUpdates, null, transaction); - ReactUpdatesFlushTransaction.release(transaction); + // updates enqueued by setState callbacks and setImmediate calls. + while (dirtyComponents.length || setImmediateEnqueued) { + if (dirtyComponents.length) { + var transaction = ReactUpdatesFlushTransaction.getPooled(); + transaction.perform(runBatchedUpdates, null, transaction); + ReactUpdatesFlushTransaction.release(transaction); + } + + if (setImmediateEnqueued) { + setImmediateEnqueued = false; + var queue = setImmediateCallbackQueue; + setImmediateCallbackQueue = CallbackQueue.getPooled(); + queue.notifyAll(); + CallbackQueue.release(queue); + } } } ); @@ -221,6 +233,20 @@ function enqueueUpdate(component, callback) { } } +/** + * Enqueue a callback to be run at the end of the current batching cycle. Throws + * if no updates are currently being performed. + */ +function setImmediate(callback, context) { + invariant( + batchingStrategy.isBatchingUpdates, + 'ReactUpdates.setImmediate: Can\'t enqueue an immediate callback in a ' + + 'context where updates are not being batched.' + ); + setImmediateCallbackQueue.enqueue(callback, context); + setImmediateEnqueued = true; +} + var ReactUpdatesInjection = { injectReconcileTransaction: function(ReconcileTransaction) { invariant( @@ -259,7 +285,8 @@ var ReactUpdates = { batchedUpdates: batchedUpdates, enqueueUpdate: enqueueUpdate, flushBatchedUpdates: flushBatchedUpdates, - injection: ReactUpdatesInjection + injection: ReactUpdatesInjection, + setImmediate: setImmediate }; module.exports = ReactUpdates; diff --git a/src/core/__tests__/ReactUpdates-test.js b/src/core/__tests__/ReactUpdates-test.js index 3c22c434597..f4093daa586 100644 --- a/src/core/__tests__/ReactUpdates-test.js +++ b/src/core/__tests__/ReactUpdates-test.js @@ -748,4 +748,79 @@ describe('ReactUpdates', function() { React.renderComponent(, container); expect(callbackCount).toBe(1); }); + + it('calls setImmediate callbacks properly', function() { + var callbackCount = 0; + var A = React.createClass({ + render: function() { + return
; + }, + componentDidUpdate: function() { + var component = this; + ReactUpdates.setImmediate(function() { + expect(this).toBe(component); + callbackCount++; + ReactUpdates.setImmediate(function() { + callbackCount++; + }); + expect(callbackCount).toBe(1); + }, this); + expect(callbackCount).toBe(0); + } + }); + + var container = document.createElement('div'); + var component = React.renderComponent(, container); + component.forceUpdate(); + expect(callbackCount).toBe(2); + }); + + it('calls setImmediate callbacks with queued updates', function() { + var log = []; + var A = React.createClass({ + getInitialState: () => ({updates: 0}), + render: function() { + log.push('render-' + this.state.updates); + return
; + }, + componentDidUpdate: function() { + if (this.state.updates === 1) { + ReactUpdates.setImmediate(function() { + this.setState({updates: 2}, function() { + ReactUpdates.setImmediate(function() { + log.push('setImmediate-1.2'); + }); + log.push('setState-cb'); + }); + log.push('setImmediate-1.1'); + }, this); + } else if (this.state.updates === 2) { + ReactUpdates.setImmediate(function() { + log.push('setImmediate-2'); + }); + } + log.push('didUpdate-' + this.state.updates); + } + }); + + var container = document.createElement('div'); + var component = React.renderComponent(, container); + component.setState({updates: 1}); + expect(log).toEqual([ + 'render-0', + // We do the first update... + 'render-1', + 'didUpdate-1', + // ...which calls a setImmediate and enqueues a second update... + 'setImmediate-1.1', + // ...which runs and enqueues the setImmediate-2 log in its didUpdate... + 'render-2', + 'didUpdate-2', + // ...and runs the setState callback, which enqueues the log for + // setImmediate-1.2. + 'setState-cb', + 'setImmediate-2', + 'setImmediate-1.2' + ]); + }); }); From 354fb4429978a8d77df72781ce1754c2ffcce9b3 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sat, 28 Jun 2014 16:45:43 -0700 Subject: [PATCH 2/2] Use setImmediate to defer value restoration Depends on #1758. Fixes #1698. Previously, controlled components would update too soon when using something like ReactLayeredComponentMixin (i.e., before the layer's updates could propagate from the parent), causing the cursor to jump even when always updating the new model value to match the DOM state. With this change, we defer the update until after all nested updates have had a chance to finish, which prevents the cursor from misbehaving. Also cleaned up the logic around updating a bit -- the .value and .checked updates in ReactDOMInput weren't being relied on at all so I removed them and opted for a simple forceUpdate instead. I also got rid of _isChanging which hasn't been necessary since the introduction of update batching. Test Plan: Tested the example in http://jsfiddle.net/Bobris/ZZtXn/2/ and verified that the cursor didn't jump. Changed the code to filter out numbers and verified that the field prevents typing numbers (attempting to do so still causes the cursor to jump to the end). Also verified that controlled and uncontrolled radio buttons, textareas, and select boxes work. --- .../ui/dom/components/ReactDOMInput.js | 42 +++++++++---------- .../ui/dom/components/ReactDOMSelect.js | 23 ++++++---- .../ui/dom/components/ReactDOMTextarea.js | 17 ++++---- 3 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/browser/ui/dom/components/ReactDOMInput.js b/src/browser/ui/dom/components/ReactDOMInput.js index 0437f60513c..1fd43512516 100644 --- a/src/browser/ui/dom/components/ReactDOMInput.js +++ b/src/browser/ui/dom/components/ReactDOMInput.js @@ -25,6 +25,7 @@ var ReactBrowserComponentMixin = require('ReactBrowserComponentMixin'); var ReactCompositeComponent = require('ReactCompositeComponent'); var ReactDOM = require('ReactDOM'); var ReactMount = require('ReactMount'); +var ReactUpdates = require('ReactUpdates'); var invariant = require('invariant'); var merge = require('merge'); @@ -34,6 +35,13 @@ var input = ReactDOM.input; var instancesByReactID = {}; +function forceUpdateIfMounted() { + /*jshint validthis:true */ + if (this.isMounted()) { + this.forceUpdate(); + } +} + /** * Implements an native component that allows setting these optional * props: `checked`, `value`, `defaultChecked`, and `defaultValue`. @@ -58,16 +66,11 @@ var ReactDOMInput = ReactCompositeComponent.createClass({ getInitialState: function() { var defaultValue = this.props.defaultValue; return { - checked: this.props.defaultChecked || false, - value: defaultValue != null ? defaultValue : null + initialChecked: this.props.defaultChecked || false, + initialValue: defaultValue != null ? defaultValue : null }; }, - shouldComponentUpdate: function() { - // Defer any updates to this component during the `onChange` handler. - return !this._isChanging; - }, - render: function() { // Clone `this.props` so we don't mutate the input. var props = merge(this.props); @@ -76,10 +79,10 @@ var ReactDOMInput = ReactCompositeComponent.createClass({ props.defaultValue = null; var value = LinkedValueUtils.getValue(this); - props.value = value != null ? value : this.state.value; + props.value = value != null ? value : this.state.initialValue; var checked = LinkedValueUtils.getChecked(this); - props.checked = checked != null ? checked : this.state.checked; + props.checked = checked != null ? checked : this.state.initialChecked; props.onChange = this._handleChange; @@ -119,14 +122,12 @@ var ReactDOMInput = ReactCompositeComponent.createClass({ var returnValue; var onChange = LinkedValueUtils.getOnChange(this); if (onChange) { - this._isChanging = true; returnValue = onChange.call(this, event); - this._isChanging = false; } - this.setState({ - checked: event.target.checked, - value: event.target.value - }); + // Here we use setImmediate to wait until all updates have propagated, which + // is important when using controlled components within layers: + // https://github.com/facebook/react/issues/1698 + ReactUpdates.setImmediate(forceUpdateIfMounted, this); var name = this.props.name; if (this.props.type === 'radio' && name != null) { @@ -164,13 +165,10 @@ var ReactDOMInput = ReactCompositeComponent.createClass({ 'ReactDOMInput: Unknown radio button ID %s.', otherID ); - // In some cases, this will actually change the `checked` state value. - // In other cases, there's no change but this forces a reconcile upon - // which componentDidUpdate will reset the DOM property to whatever it - // should be. - otherInstance.setState({ - checked: false - }); + // If this is a controlled radio button group, forcing the input that + // was previously checked to update will cause it to be come re-checked + // as appropriate. + ReactUpdates.setImmediate(forceUpdateIfMounted, otherInstance); } } diff --git a/src/browser/ui/dom/components/ReactDOMSelect.js b/src/browser/ui/dom/components/ReactDOMSelect.js index 9a3a3e08182..3b965c60bad 100644 --- a/src/browser/ui/dom/components/ReactDOMSelect.js +++ b/src/browser/ui/dom/components/ReactDOMSelect.js @@ -23,12 +23,21 @@ var LinkedValueUtils = require('LinkedValueUtils'); var ReactBrowserComponentMixin = require('ReactBrowserComponentMixin'); var ReactCompositeComponent = require('ReactCompositeComponent'); var ReactDOM = require('ReactDOM'); +var ReactUpdates = require('ReactUpdates'); var merge = require('merge'); // Store a reference to the