Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve refs in the order of the children #7101

Merged
merged 2 commits into from
Jun 22, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 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,13 @@ var ReactChildReconciler = {
// The child must be instantiated before it's mounted.
var nextChildInstance = instantiateReactComponent(nextElement);
nextChildren[name] = nextChildInstance;
mountImages[name] = ReactReconciler.mountComponent(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use an array for this in mountChildren so I can change it here too if necessary.

nextChildInstance,
transaction,
hostParent,
hostContainerInfo,
context
);
}
}
// Unmount children that are no longer present.
Expand Down
30 changes: 21 additions & 9 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 Down Expand Up @@ -375,6 +392,7 @@ var ReactMultiChild = {
updates,
this._mountChildAtIndex(
nextChild,
mountImages[name],
lastPlacedNode,
nextIndex,
transaction,
Expand Down Expand Up @@ -468,17 +486,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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I verified this fails on master (5 tests failing).

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