@samitouri / QOS-React / commits / 83cbc3e5fb

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

Dan Abramov committed Jun 22, 2016 at 22:28 UTC 83cbc3e5fb700f45c48214c3785d8b44ab5ebdce

3 files changed +89 -22

src/renderers/shared/stack/reconciler/ReactChildReconciler.js

+13

@@ -93,8 +93,11 @@ var ReactChildReconciler = {

93 updateChildren: function(

94 prevChildren,

95 nextChildren,

96 + mountImages,

97 removedNodes,

98 transaction,

99 + hostParent,

100 + hostContainerInfo,

101 context) {

102 // We currently don't have a way to track moves here but if we use iterators

103 // instead of for..in we can zip the iterators and check if an item has

@@ -127,6 +130,16 @@ var ReactChildReconciler = {

130 // The child must be instantiated before it's mounted.

131 var nextChildInstance = instantiateReactComponent(nextElement);

132 nextChildren[name] = nextChildInstance;

133 + // Creating mount image now ensures refs are resolved in right order

134 + // (see https://github.com/facebook/react/pull/7101 for explanation).

135 + var nextChildMountImage = ReactReconciler.mountComponent(

136 + nextChildInstance,

137 + transaction,

138 + hostParent,

139 + hostContainerInfo,

140 + context

141 + );

142 + mountImages.push(nextChildMountImage);

143 }

144 }

145 // Unmount children that are no longer present.

src/renderers/shared/stack/reconciler/ReactMultiChild.js

+25 -10

@@ -207,6 +207,7 @@ var ReactMultiChild = {

207 _reconcilerUpdateChildren: function(

208 prevChildren,

209 nextNestedChildrenElements,

210 + mountImages,

211 removedNodes,

212 transaction,

213 context

@@ -221,14 +222,28 @@ var ReactMultiChild = {

222 ReactCurrentOwner.current = null;

223 }

224 ReactChildReconciler.updateChildren(

224 - prevChildren, nextChildren, removedNodes, transaction, context

225 + prevChildren,

226 + nextChildren,

227 + mountImages,

228 + removedNodes,

229 + transaction,

230 + this,

231 + this._hostContainerInfo,

232 + context

233 );

234 return nextChildren;

235 }

236 }

237 nextChildren = flattenChildren(nextNestedChildrenElements);

238 ReactChildReconciler.updateChildren(

231 - prevChildren, nextChildren, removedNodes, transaction, context

239 + prevChildren,

240 + nextChildren,

241 + mountImages,

242 + removedNodes,

243 + transaction,

244 + this,

245 + this._hostContainerInfo,

246 + context

247 );

248 return nextChildren;

249 },

@@ -334,9 +349,11 @@ var ReactMultiChild = {

349 _updateChildren: function(nextNestedChildrenElements, transaction, context) {

350 var prevChildren = this._renderedChildren;

351 var removedNodes = {};

352 + var mountImages = [];

353 var nextChildren = this._reconcilerUpdateChildren(

354 prevChildren,

355 nextNestedChildrenElements,

356 + mountImages,

357 removedNodes,

358 transaction,

359 context

@@ -348,8 +365,10 @@ var ReactMultiChild = {

365 var name;

366 // `nextIndex` will increment for each child in `nextChildren`, but

367 // `lastIndex` will be the last index visited in `prevChildren`.

351 - var lastIndex = 0;

368 var nextIndex = 0;

369 + var lastIndex = 0;

370 + // `nextMountIndex` will increment for each newly mounted child.

371 + var nextMountIndex = 0;

372 var lastPlacedNode = null;

373 for (name in nextChildren) {

374 if (!nextChildren.hasOwnProperty(name)) {

@@ -375,12 +394,14 @@ var ReactMultiChild = {

394 updates,

395 this._mountChildAtIndex(

396 nextChild,

397 + mountImages[nextMountIndex],

398 lastPlacedNode,

399 nextIndex,

400 transaction,

401 context

402 )

403 );

404 + nextMountIndex++;

405 }

406 nextIndex++;

407 lastPlacedNode = ReactReconciler.getHostNode(nextChild);

@@ -468,17 +489,11 @@ var ReactMultiChild = {

489 */

490 _mountChildAtIndex: function(

491 child,

492 + mountImage,

493 afterNode,

494 index,

495 transaction,

496 context) {

475 - var mountImage = ReactReconciler.mountComponent(

476 - child,

477 - transaction,

478 - this,

479 - this._hostContainerInfo,

480 - context

481 - );

497 child._mountIndex = index;

498 return this.createChild(child, afterNode, mountImage);

499 },

src/renderers/shared/stack/reconciler/__tests__/ReactMultiChildReconcile-test.js

+51 -12

@@ -60,6 +60,14 @@ var StatusDisplay = React.createClass({

60 return this.state.internalState;

61 },

63 + componentDidMount: function() {

64 + this.props.onFlush();

65 + },

66 +

67 + componentDidUpdate: function() {

68 + this.props.onFlush();

69 + },

70 +

71 render: function() {

72 return (

73 <div>

@@ -74,35 +82,61 @@ var StatusDisplay = React.createClass({

82 */

83 var FriendsStatusDisplay = React.createClass({

84 /**

77 - * Retrieves the rendered children in a nice format for comparing to the input

78 - * `this.props.usernameToStatus`. Gets the order directly from each rendered

79 - * child's `index` field. Refs are not maintained in the rendered order, and

80 - * neither is `this._renderedChildren` (surprisingly).

81 - */

82 - getStatusDisplays: function() {

83 - var name;

84 - var orderOfUsernames = [];

85 + * Gets the order directly from each rendered child's `index` field.

86 + * Refs are not maintained in the rendered order, and neither is

87 + * `this._renderedChildren` (surprisingly).

88 + */

89 + getOriginalKeys: function() {

90 + var originalKeys = [];

91 // TODO: Update this to a better test that doesn't rely so much on internal

92 // implementation details.

93 var statusDisplays =

94 ReactInstanceMap.get(this)

95 ._renderedComponent

96 ._renderedChildren;

97 + var name;

98 for (name in statusDisplays) {

99 var child = statusDisplays[name];

100 var isPresent = !!child;

101 if (isPresent) {

95 - orderOfUsernames[child._mountIndex] = getOriginalKey(name);

102 + originalKeys[child._mountIndex] = getOriginalKey(name);

103 }

104 }

105 + return originalKeys;

106 + },

107 + /**

108 + * Retrieves the rendered children in a nice format for comparing to the input

109 + * `this.props.usernameToStatus`.

110 + */

111 + getStatusDisplays: function() {

112 var res = {};

113 var i;

100 - for (i = 0; i < orderOfUsernames.length; i++) {

101 - var key = orderOfUsernames[i];

114 + var originalKeys = this.getOriginalKeys();

115 + for (i = 0; i < originalKeys.length; i++) {

116 + var key = originalKeys[i];

117 res[key] = this.refs[key];

118 }

119 return res;

120 },

121 + /**

122 + * Verifies that by the time a child is flushed, the refs that appeared

123 + * earlier have already been resolved.

124 + * TODO: This assumption will likely break with incremental reconciler

125 + * but our internal layer API depends on this assumption. We need to change

126 + * it to be more declarative before making ref resolution indeterministic.

127 + */

128 + verifyPreviousRefsResolved: function(flushedKey) {

129 + var i;

130 + var originalKeys = this.getOriginalKeys();

131 + for (i = 0; i < originalKeys.length; i++) {

132 + var key = originalKeys[i];

133 + if (key === flushedKey) {

134 + // We are only interested in children up to the current key.

135 + return;

136 + }

137 + expect(this.refs[key]).toBeTruthy();

138 + }

139 + },

140 render: function() {

141 var children = [];

142 var key;

@@ -110,7 +144,12 @@ var FriendsStatusDisplay = React.createClass({

144 var status = this.props.usernameToStatus[key];

145 children.push(

146 !status ? null :

113 - <StatusDisplay key={key} ref={key} status={status} />

147 + <StatusDisplay

148 + key={key}

149 + ref={key}

150 + onFlush={this.verifyPreviousRefsResolved.bind(this, key)}

151 + status={status}

152 + />

153 );

154 }

155 return (