From 343033bf0647a365a3a49f4e60a6ae1e8b2682bf Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 22 Jun 2016 22:28:06 +0100 Subject: [PATCH] Resolve refs in the order of the children (#7101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Resolve refs in the order of the children React makes no guarantees about ref resolution order. Unfortunately, some of the internal Facebook component APIs (specifically, layer dialogs) currently depend on the ref resolution order. Specifically, the assumption is that if the layer dialog is placed as a last child, by the time it mounts or updates, the refs to any previously declared elements have been resolved. With the current `ReactMultiChild`, this is *usually* the case but not always. Both initial mount and an update of all components satisfy this assumption: by the time a child mounts or updates, the previous children’s refs have been resolved. The one scenario where it isn’t true is when **a new child is mounted during an update**. In this case, the `mountComponent()` call used to be delayed until `ReactMultiChild` processes the queue. However, this is inconsistent with how updates normally work: unlike mounting, updating and unmounting happens inside `ReactChildReconciler.updateChildren()` loop. This PR changes the `mountComponent()` to be performed inside `ReactChildReconciler`, just like `receiveComponent()` and `unmountComponent()`, and thus ensures that `attachRef()` calls are enqueued in the order the children were processed, so by the time the next child flushes, the refs of the previous children have been resolved. This is not ideal and will probably be broken by incremental reconciler in the future. However, since we are trying to get rid of mixins in the internal codebase, and layered components are one of the biggest blockers to that, it’s lesser evil to temporarily make ref resolution order more strict until we have time to fix up the layer APIs to not rely on it, and are able to relax it again (which would be a breaking change). * Use array instead of object to avoid lookups (cherry picked from commit 83cbc3e5fb700f45c48214c3785d8b44ab5ebdce) --- .../stack/reconciler/ReactChildReconciler.js | 13 ++++ .../stack/reconciler/ReactMultiChild.js | 35 ++++++++--- .../ReactMultiChildReconcile-test.js | 63 +++++++++++++++---- 3 files changed, 89 insertions(+), 22 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/ReactChildReconciler.js b/src/renderers/shared/stack/reconciler/ReactChildReconciler.js index 060f5c0ac076b..40f589c435234 100644 --- a/src/renderers/shared/stack/reconciler/ReactChildReconciler.js +++ b/src/renderers/shared/stack/reconciler/ReactChildReconciler.js @@ -93,8 +93,11 @@ var ReactChildReconciler = { updateChildren: function( prevChildren, nextChildren, + mountImages, removedNodes, transaction, + hostParent, + hostContainerInfo, context) { // We currently don't have a way to track moves here but if we use iterators // instead of for..in we can zip the iterators and check if an item has @@ -127,6 +130,16 @@ var ReactChildReconciler = { // The child must be instantiated before it's mounted. var nextChildInstance = instantiateReactComponent(nextElement, true); nextChildren[name] = nextChildInstance; + // Creating mount image now ensures refs are resolved in right order + // (see https://github.com/facebook/react/pull/7101 for explanation). + var nextChildMountImage = ReactReconciler.mountComponent( + nextChildInstance, + transaction, + hostParent, + hostContainerInfo, + context + ); + mountImages.push(nextChildMountImage); } } // Unmount children that are no longer present. diff --git a/src/renderers/shared/stack/reconciler/ReactMultiChild.js b/src/renderers/shared/stack/reconciler/ReactMultiChild.js index 5f35b7a3cb48d..474ec774f055d 100644 --- a/src/renderers/shared/stack/reconciler/ReactMultiChild.js +++ b/src/renderers/shared/stack/reconciler/ReactMultiChild.js @@ -212,6 +212,7 @@ var ReactMultiChild = { _reconcilerUpdateChildren: function( prevChildren, nextNestedChildrenElements, + mountImages, removedNodes, transaction, context @@ -226,14 +227,28 @@ var ReactMultiChild = { ReactCurrentOwner.current = null; } ReactChildReconciler.updateChildren( - prevChildren, nextChildren, removedNodes, transaction, context + prevChildren, + nextChildren, + mountImages, + removedNodes, + transaction, + this, + this._hostContainerInfo, + context ); return nextChildren; } } nextChildren = flattenChildren(nextNestedChildrenElements); ReactChildReconciler.updateChildren( - prevChildren, nextChildren, removedNodes, transaction, context + prevChildren, + nextChildren, + mountImages, + removedNodes, + transaction, + this, + this._hostContainerInfo, + context ); return nextChildren; }, @@ -339,9 +354,11 @@ var ReactMultiChild = { _updateChildren: function(nextNestedChildrenElements, transaction, context) { var prevChildren = this._renderedChildren; var removedNodes = {}; + var mountImages = []; var nextChildren = this._reconcilerUpdateChildren( prevChildren, nextNestedChildrenElements, + mountImages, removedNodes, transaction, context @@ -353,8 +370,10 @@ var ReactMultiChild = { var name; // `nextIndex` will increment for each child in `nextChildren`, but // `lastIndex` will be the last index visited in `prevChildren`. - var lastIndex = 0; var nextIndex = 0; + var lastIndex = 0; + // `nextMountIndex` will increment for each newly mounted child. + var nextMountIndex = 0; var lastPlacedNode = null; for (name in nextChildren) { if (!nextChildren.hasOwnProperty(name)) { @@ -380,12 +399,14 @@ var ReactMultiChild = { updates, this._mountChildAtIndex( nextChild, + mountImages[nextMountIndex], lastPlacedNode, nextIndex, transaction, context ) ); + nextMountIndex++; } nextIndex++; lastPlacedNode = ReactReconciler.getHostNode(nextChild); @@ -473,17 +494,11 @@ var ReactMultiChild = { */ _mountChildAtIndex: function( child, + mountImage, afterNode, index, transaction, context) { - var mountImage = ReactReconciler.mountComponent( - child, - transaction, - this, - this._hostContainerInfo, - context - ); child._mountIndex = index; return this.createChild(child, afterNode, mountImage); }, diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactMultiChildReconcile-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactMultiChildReconcile-test.js index b0f190b6cf719..50a1a01e208ff 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactMultiChildReconcile-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactMultiChildReconcile-test.js @@ -60,6 +60,14 @@ var StatusDisplay = React.createClass({ return this.state.internalState; }, + componentDidMount: function() { + this.props.onFlush(); + }, + + componentDidUpdate: function() { + this.props.onFlush(); + }, + render: function() { return (
@@ -74,35 +82,61 @@ var StatusDisplay = React.createClass({ */ var FriendsStatusDisplay = React.createClass({ /** - * Retrieves the rendered children in a nice format for comparing to the input - * `this.props.usernameToStatus`. Gets the order directly from each rendered - * child's `index` field. Refs are not maintained in the rendered order, and - * neither is `this._renderedChildren` (surprisingly). - */ - getStatusDisplays: function() { - var name; - var orderOfUsernames = []; + * Gets the order directly from each rendered child's `index` field. + * Refs are not maintained in the rendered order, and neither is + * `this._renderedChildren` (surprisingly). + */ + getOriginalKeys: function() { + var originalKeys = []; // TODO: Update this to a better test that doesn't rely so much on internal // implementation details. var statusDisplays = ReactInstanceMap.get(this) ._renderedComponent ._renderedChildren; + var name; for (name in statusDisplays) { var child = statusDisplays[name]; var isPresent = !!child; if (isPresent) { - orderOfUsernames[child._mountIndex] = getOriginalKey(name); + originalKeys[child._mountIndex] = getOriginalKey(name); } } + return originalKeys; + }, + /** + * Retrieves the rendered children in a nice format for comparing to the input + * `this.props.usernameToStatus`. + */ + getStatusDisplays: function() { var res = {}; var i; - for (i = 0; i < orderOfUsernames.length; i++) { - var key = orderOfUsernames[i]; + var originalKeys = this.getOriginalKeys(); + for (i = 0; i < originalKeys.length; i++) { + var key = originalKeys[i]; res[key] = this.refs[key]; } return res; }, + /** + * Verifies that by the time a child is flushed, the refs that appeared + * earlier have already been resolved. + * TODO: This assumption will likely break with incremental reconciler + * but our internal layer API depends on this assumption. We need to change + * it to be more declarative before making ref resolution indeterministic. + */ + verifyPreviousRefsResolved: function(flushedKey) { + var i; + var originalKeys = this.getOriginalKeys(); + for (i = 0; i < originalKeys.length; i++) { + var key = originalKeys[i]; + if (key === flushedKey) { + // We are only interested in children up to the current key. + return; + } + expect(this.refs[key]).toBeTruthy(); + } + }, render: function() { var children = []; var key; @@ -110,7 +144,12 @@ var FriendsStatusDisplay = React.createClass({ var status = this.props.usernameToStatus[key]; children.push( !status ? null : - + ); } return (