From adae02b68f53d0a0652f80a93a278f012d94071d 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 | 29 +++++++++++++++++++++---- src/core/__tests__/ReactUpdates-test.js | 22 +++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/core/ReactUpdates.js b/src/core/ReactUpdates.js index 759e814ae08..9eb4fe1dc55 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,8 +172,8 @@ 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) { + // updates enqueued by setState callbacks and setImmediate calls. + while (dirtyComponents.length || setImmediateEnqueued) { var allUnmounted = true; for (var i = 0, l = dirtyComponents.length; i < l; i++) { if (dirtyComponents[i].isMounted()) { @@ -194,6 +196,10 @@ var flushBatchedUpdates = ReactPerf.measure( var transaction = ReactUpdatesFlushTransaction.getPooled(); transaction.perform(runBatchedUpdates, null, transaction); ReactUpdatesFlushTransaction.release(transaction); + + setImmediateCallbackQueue.notifyAll(); + setImmediateCallbackQueue.reset(); + setImmediateEnqueued = false; } } ); @@ -240,6 +246,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( @@ -278,7 +298,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..abe29a4d20a 100644 --- a/src/core/__tests__/ReactUpdates-test.js +++ b/src/core/__tests__/ReactUpdates-test.js @@ -748,4 +748,26 @@ 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++; + }, this); + expect(callbackCount).toBe(0); + } + }); + + var container = document.createElement('div'); + var component = React.renderComponent(, container); + component.forceUpdate(); + expect(callbackCount).toBe(1); + }); }); From f0d978025651b772a7915b0fd796e68a25e8c022 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