Skip to content

Commit

Permalink
Resolve refs in the order of the children (#7101)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
gaearon authored Jun 22, 2016
1 parent c7ba16f commit 83cbc3e
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 22 deletions.
13 changes: 13 additions & 0 deletions src/renderers/shared/stack/reconciler/ReactChildReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -127,6 +130,16 @@ var ReactChildReconciler = {
// The child must be instantiated before it's mounted.
var nextChildInstance = instantiateReactComponent(nextElement);
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.
Expand Down
35 changes: 25 additions & 10 deletions src/renderers/shared/stack/reconciler/ReactMultiChild.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ var ReactMultiChild = {
_reconcilerUpdateChildren: function(
prevChildren,
nextNestedChildrenElements,
mountImages,
removedNodes,
transaction,
context
Expand All @@ -221,14 +222,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;
},
Expand Down Expand Up @@ -334,9 +349,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
Expand All @@ -348,8 +365,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)) {
Expand All @@ -375,12 +394,14 @@ var ReactMultiChild = {
updates,
this._mountChildAtIndex(
nextChild,
mountImages[nextMountIndex],
lastPlacedNode,
nextIndex,
transaction,
context
)
);
nextMountIndex++;
}
nextIndex++;
lastPlacedNode = ReactReconciler.getHostNode(nextChild);
Expand Down Expand Up @@ -468,17 +489,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);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<div>
Expand All @@ -74,43 +82,74 @@ 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;
for (key in this.props.usernameToStatus) {
var status = this.props.usernameToStatus[key];
children.push(
!status ? null :
<StatusDisplay key={key} ref={key} status={status} />
<StatusDisplay
key={key}
ref={key}
onFlush={this.verifyPreviousRefsResolved.bind(this, key)}
status={status}
/>
);
}
return (
Expand Down

0 comments on commit 83cbc3e

Please sign in to comment.