Resolve refs in the order of the children #7101

Merged
merged 2 commits into from Jun 22, 2016

Projects

None yet

4 participants

@gaearon
Member
gaearon commented Jun 22, 2016 edited

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 (or replaced) during an update.

In this case, the mountComponent() call used to be delayed until ReactMultiChild processes the queue. Therefore, for newly mounted components, attachRef() gets enqueued too late, and previously existing children resolve their refs first even when they appear after newly mounted components. This subtly breaks the assumption made by our dialog layer APIs.

Demo fiddle: https://jsfiddle.net/c1f9chm3/

screen shot 2016-06-22 at 17 10 32

This PR changes the mountComponent() to be performed inside ReactChildReconciler.updateChildren(), just like receiveComponent() and unmountComponent(). This 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.

screen shot 2016-06-22 at 17 10 52

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).


Reviewers: @spicyj @sebmarkbage

Is this a breaking change? Personally I don’t think so because ref resolution order is indeterministic (in fact that’s the issue I’m trying to fix here). We used to resolve refs in a specific order in almost all cases except mounting. So I think it’s hard to even accidentally rely on the existing behavior. However in theory I can imagine something breaking in a ref-heavy code. So I’m torn.

@gaearon gaearon 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).
c59878e
@gaearon gaearon commented on an outdated diff Jun 22, 2016
...erers/shared/stack/reconciler/ReactChildReconciler.js
@@ -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(
@gaearon
gaearon Jun 22, 2016 Member

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

@gaearon gaearon commented on the diff Jun 22, 2016
...reconciler/__tests__/ReactMultiChildReconcile-test.js
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) {
@gaearon
gaearon Jun 22, 2016 Member

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

@sebmarkbage
Member

This looks good to me but that extra allocation worries me. It is going to stay on the young generation so I guess it is fine. You should at least make it an array if that's possible and lookups are fast.

We might want to keep this for a minor release at least. Can have small artifacts show up.

@gaearon gaearon added this to the 15.2.0 milestone Jun 22, 2016
@gaearon
Member
gaearon commented Jun 22, 2016

@sebmarkbage Changed to use an array. Stamp?

@gaearon
Member
gaearon commented Jun 22, 2016

We might want to keep this for a minor release at least.

Do you mean queue it for 15.3.0 but use on the website?

@spicyj spicyj modified the milestone: 15.2.0, 15-next Jun 22, 2016
@gaearon gaearon merged commit 83cbc3e into facebook:master Jun 22, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@gaearon gaearon deleted the unknown repository branch Jun 23, 2016
@zpao zpao modified the milestone: 15-next, 15.3.0 Jul 13, 2016
@zpao zpao added a commit that referenced this pull request Jul 13, 2016
@gaearon @zpao gaearon + zpao Resolve refs in the order of the children (#7101)
* 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 83cbc3e)
343033b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment