@samitouri / QOS-React-2 / commits / 6a41ede2d4
Fork 0

The library for web and native user interfaces.

Fixing known keying problems

This fixes the last known parts of the flattening experiment. This has grown to be somewhat complex and potentially fragile because of it. We may end up reverting flattening in the future or address it slightly differently. The purpose of this diff is to test if we've finally understood the real world edge cases that flattening can lead to and how we have to key components to cover those cases. With this commit we never rekey the internal _key property. The semantics is that once a component passes through a composite component, it's identity is frozen. props.key should accept numeric values and booleans which includes 0 and false. This fixes the truthiness check. We should never warn about missing key properties if a component is passed as a static child. The _key acts as a flag to determine whether this component was checked already.

CommitSyncScript committed Jun 17, 2013 at 12:47 UTC 6a41ede2d426cea1cb29103305362383f4d10f4d
6 files changed +203 -57
src/core/ReactComponent.js
+58 -10
@@ -66,37 +66,84 @@ var HAS_WARNED = false;
66 * one.
67 */
68
69 +/**
70 + * Generate a unique key that identifies this child within a set.
71 + *
72 + * @param {*} Manually provided key.
73 + * @param {number} Index that is used if a manual key is not provided.
74 + * @param {?number} Grouping index if this is used in a nested array.
75 + * @return {string}
76 + */
77 +function createKey(explicitKey, childIndex, groupingIndex) {
78 + return ReactCurrentOwner.getDepth() + ':' +
79 + (groupingIndex == null ? '' : groupingIndex + ':') +
80 + (explicitKey == null ? '' + childIndex : explicitKey);
81 +}
82 +
83 +/**
84 + * Returns true if this parameter type is considered an empty child slot.
85 + * Used to filter out empty slots and generate a compact array.
86 + *
87 + * @param {*} Child component or any value.
88 + * @return {boolean}
89 + */
90 function isEmptyChild(child) {
91 return child == null || typeof child === 'boolean';
92 }
93
73 -function assignKey(setKey, child, index) {
74 - if (ReactComponent.isValidComponent(child)) {
94 +/**
95 + * Assign an internal identity to a child component.
96 + *
97 + * @param {number} Index of the current array grouping.
98 + * @param {*} Child component or any value.
99 + * @param {number} Index of the current child within it's grouping.
100 + */
101 +function assignKey(groupingIndex, child, index) {
102 + // Only truthy internal keys are valid. If it's not, we assign one.
103 + if (ReactComponent.isValidComponent(child) && !child._key) {
104 var key = child.props.key;
105 if (__DEV__) {
77 - if (!HAS_WARNED && !key) {
106 + // This is in an array. This could grow and shrink or be reordered.
107 + // All children that doesn't already have autogenerated identity needs
108 + // to have an explicit key provided.
109 + if (!HAS_WARNED && key == null) {
110 HAS_WARNED = true;
111 console && console.warn && console.warn(CHILD_HAS_NO_IDENTITY);
112 }
113 }
82 - child._key = (setKey ? setKey + ':' : '') + (key || ('' + index));
114 + child._key = createKey(key, index, groupingIndex);
115 }
116 }
117
118 +/**
119 + * Make sure all children have an internal identity. Returns true if this is
120 + * already a compact array.
121 + *
122 + * @param {array} Children of any type.
123 + * @return {boolean}
124 + */
125 function tryToReuseArray(children) {
126 for (var i = 0; i < children.length; i++) {
127 var child = children[i];
128 if (isEmptyChild(child)) return false;
90 - assignKey('0', child, i);
129 + assignKey(0, child, i);
130 }
131 return true;
132 }
133
95 -function appendNestedChildren(parentKey, sourceArray, targetArray) {
134 +/**
135 + * Append children from the source array to the target array. Make sure all
136 + * children have an internal identity assigned to it based on insertion point.
137 + *
138 + * @param {number} Index of the current array grouping.
139 + * @param {array} Source array.
140 + * @param {array} Target array that will be appended to.
141 + */
142 +function appendNestedChildren(groupingIndex, sourceArray, targetArray) {
143 for (var i = 0; i < sourceArray.length; i++) {
144 var child = sourceArray[i];
145 if (isEmptyChild(child)) continue;
99 - assignKey(parentKey, child, i);
146 + assignKey(groupingIndex, child, i);
147 // TODO: Invalid components like strings could possibly need
148 // keys assigned to them here. Usually they're not stateful but
149 // CSS transitions and special events could make them stateful.
@@ -294,14 +341,15 @@ var ReactComponent = {
341 if (child.length === 0) continue;
342
343 if (targetArray === null) targetArray = [];
297 - appendNestedChildren('' + (i - 1), child, targetArray);
344 + appendNestedChildren(i - 1, child, targetArray);
345
346 } else if (!isEmptyChild(child)) {
347
301 - if (ReactComponent.isValidComponent(child)) {
348 + // Only truthy internal keys are valid. If it's not, we assign one.
349 + if (ReactComponent.isValidComponent(child) && !child._key) {
350 // This is a static node and therefore safe to key by index.
351 // No warning necessary.
304 - child._key = child.props.key || ('' + (i - 1));
352 + child._key = createKey(child.props.key, i - 1);
353 }
354
355 if (targetArray === null) targetArray = [];
src/core/ReactCompositeComponent.js
+4
@@ -414,6 +414,8 @@ var ReactCompositeComponentMixin = {
414 this.state = null;
415 this._pendingState = null;
416 this._compositeLifeCycleState = null;
417 + this._compositionLevel = ReactCurrentOwner.current ?
418 + ReactCurrentOwner.getDepth() + 1 : 0;
419 },
420
421 /**
@@ -453,6 +455,8 @@ var ReactCompositeComponentMixin = {
455
456 this._renderedComponent = this._renderValidatedComponent();
457
458 + this._renderedComponent._compositionLevel = this._compositionLevel + 1;
459 +
460 // Done with mounting, `setState` will now trigger UI changes.
461 this._compositeLifeCycleState = null;
462 this._lifeCycleState = ReactComponent.LifeCycle.MOUNTED;
src/core/ReactCurrentOwner.js
+13 -1
@@ -23,6 +23,8 @@
23 *
24 * The current owner is the component who should own any components that are
25 * currently being constructed.
26 + *
27 + * The depth indicate how many composite components are above this render level.
28 */
29 var ReactCurrentOwner = {
30
@@ -30,7 +32,17 @@ var ReactCurrentOwner = {
32 * @internal
33 * @type {ReactComponent}
34 */
33 - current: null
35 + current: null,
36 +
37 + /**
38 + * @internal
39 + * @return {number}
40 + */
41 + getDepth: function() {
42 + var owner = ReactCurrentOwner.current;
43 + if (!owner) return 0;
44 + return owner._compositionLevel;
45 + }
46
47 };
48
src/core/__tests__/ReactComponentFlattenChildren-test.js
+23 -22
@@ -56,8 +56,8 @@ describe('ReactComponentFlatten', function() {
56 var flat = singleChild(children);
57
58 expect(flat).toBe(children);
59 - expect(flat[0]._key).toEqual('0:foo');
60 - expect(flat[1]._key).toEqual('0:bar');
59 + expect(flat[0]._key).toEqual('0:0:foo');
60 + expect(flat[1]._key).toEqual('0:0:bar');
61 });
62
63 it("should collapse a sparse array", function() {
@@ -71,8 +71,8 @@ describe('ReactComponentFlatten', function() {
71
72 expect(flat).not.toBe(children);
73 expect(flat.length).toEqual(2);
74 - expect(flat[0]._key).toEqual('0:foo');
75 - expect(flat[1]._key).toEqual('0:bar');
74 + expect(flat[0]._key).toEqual('0:0:foo');
75 + expect(flat[1]._key).toEqual('0:0:bar');
76 });
77
78 it("should collapse a nested array into a flat array", function() {
@@ -83,13 +83,11 @@ describe('ReactComponentFlatten', function() {
83 );
84
85 expect(flat.length).toEqual(2);
86 - expect(flat[0]._key).toEqual('foo');
87 - expect(flat[1]._key).toEqual('2:bar');
86 + expect(flat[0]._key).toEqual('0:foo');
87 + expect(flat[1]._key).toEqual('0:2:bar');
88 });
89
90 - it("should use retain key index despite static empty values", function() {
91 - // TODO: Wrap in another single child (currently breaks)
92 -
90 + it("should retain key index despite static empty values", function() {
91 var before = multiChild(
92 <div />,
93 <div />,
@@ -110,22 +108,25 @@ describe('ReactComponentFlatten', function() {
108 expect(before[3]._key).toEqual(after[1]._key);
109 });
110
113 - it("should assign idempotent keys for extra flattening layers", function() {
114 - var flat = multiChild(
115 - null,
116 - [null, <div key="FOO" />],
117 - false
118 - );
111 + it("should retain key index when wrapped", function() {
112 + var before = singleChild(multiChild(
113 + <div />,
114 + <div />,
115 + [<div key="foo" />],
116 + [<div key="foo" />]
117 + ));
118
120 - var preFlat = multiChild(
119 + var after = singleChild(multiChild(
120 null,
122 - singleChild([null, <div key="FOO" />]),
123 - false
124 - );
121 + <div />,
122 + null,
123 + [<div key="foo" />]
124 + ));
125
126 - expect(flat.length).toBe(1);
127 - expect(preFlat.length).toBe(1);
128 - expect(preFlat[0]._key).toBe(flat[0]._key);
126 + expect(before.length).toEqual(4);
127 + expect(after.length).toEqual(2);
128 + expect(before[1]._key).toEqual(after[0]._key);
129 + expect(before[3]._key).toEqual(after[1]._key);
130 });
131
132 it("should assign idempotent strings through flattening", function() {
src/core/__tests__/ReactIdentity-test.js
+97 -17
@@ -62,13 +62,17 @@ describe('ReactIdentity', function() {
62 <div>
63 <div key="apple" />
64 <div key="banana" />
65 + <div key={0} />
66 + <div key={123} />
67 </div>;
68
69 React.renderComponent(instance, document.createElement('div'));
70 var node = instance.getDOMNode();
69 - reactComponentExpect(instance).toBeDOMComponentWithChildCount(2);
70 - checkId(node.childNodes[0], '.reactRoot[0].[apple]');
71 - checkId(node.childNodes[1], '.reactRoot[0].[banana]');
71 + reactComponentExpect(instance).toBeDOMComponentWithChildCount(4);
72 + checkId(node.childNodes[0], '.reactRoot[0].[0:apple]');
73 + checkId(node.childNodes[1], '.reactRoot[0].[0:banana]');
74 + checkId(node.childNodes[2], '.reactRoot[0].[0:0]');
75 + checkId(node.childNodes[3], '.reactRoot[0].[0:123]');
76 });
77
78 it('should use instance identity', function() {
@@ -89,32 +93,108 @@ describe('ReactIdentity', function() {
93 React.renderComponent(instance, document.createElement('div'));
94 var node = instance.getDOMNode();
95 reactComponentExpect(instance).toBeDOMComponentWithChildCount(3);
92 - checkId(node.childNodes[0], '.reactRoot[0].[wrap1]');
96 + checkId(node.childNodes[0], '.reactRoot[0].[0:wrap1]');
97 checkId(
98 node.childNodes[0].firstChild,
95 - '.reactRoot[0].[wrap1].[0:squirrel]'
99 + '.reactRoot[0].[0:wrap1].[0:squirrel]'
100 );
97 - checkId(node.childNodes[1], '.reactRoot[0].[wrap2]');
98 - checkId(node.childNodes[1].firstChild, '.reactRoot[0].[wrap2].[0:bunny]');
99 - checkId(node.childNodes[2], '.reactRoot[0].[2]');
100 - checkId(node.childNodes[2].firstChild, '.reactRoot[0].[2].[0:chipmunk]');
101 + checkId(node.childNodes[1], '.reactRoot[0].[0:wrap2]');
102 + checkId(node.childNodes[1].firstChild, '.reactRoot[0].[0:wrap2].[0:bunny]');
103 + checkId(node.childNodes[2], '.reactRoot[0].[0:2]');
104 + checkId(node.childNodes[2].firstChild, '.reactRoot[0].[0:2].[0:chipmunk]');
105 });
106
107 it('should let restructured components retain their uniqueness', function() {
108 var instance0 = <span />;
109 var instance1 = <span />;
110 var instance2 = <span />;
107 - var wrapped = <div>{instance0} {instance1}</div>;
108 - var unwrappedAndAdded =
109 - <div>
110 - {instance2}
111 - {wrapped.props.children[0]}
112 - {wrapped.props.children[1]}
113 - </div>;
111 +
112 + var TestComponent = React.createClass({
113 + render: function() {
114 + return (
115 + <div>
116 + {instance2}
117 + {this.props.children[0]}
118 + {this.props.children[1]}
119 + </div>
120 + );
121 + }
122 + });
123 +
124 + var TestContainer = React.createClass({
125 +
126 + render: function() {
127 + return <TestComponent>{instance0} {instance1}</TestComponent>;
128 + }
129 +
130 + });
131 +
132 + expect(function() {
133 +
134 + React.renderComponent(<TestContainer />, document.createElement('div'));
135 +
136 + }).not.toThrow();
137 + });
138 +
139 + it('should let nested restructures retain their uniqueness', function() {
140 + var instance0 = <span />;
141 + var instance1 = <span />;
142 + var instance2 = <span />;
143 +
144 + var TestComponent = React.createClass({
145 + render: function() {
146 + return (
147 + <div>
148 + {instance2}
149 + {this.props.children[0]}
150 + {this.props.children[1]}
151 + </div>
152 + );
153 + }
154 + });
155 +
156 + var TestContainer = React.createClass({
157 +
158 + render: function() {
159 + return (
160 + <div>
161 + <TestComponent>{instance0} {instance1}</TestComponent>
162 + </div>
163 + );
164 + }
165 +
166 + });
167 +
168 + expect(function() {
169 +
170 + React.renderComponent(<TestContainer />, document.createElement('div'));
171 +
172 + }).not.toThrow();
173 + });
174 +
175 + it('should let text nodes retain their uniqueness', function() {
176 + var TestComponent = React.createClass({
177 + render: function() {
178 + return <div>{this.props.children}<span /></div>;
179 + }
180 + });
181 +
182 + var TestContainer = React.createClass({
183 +
184 + render: function() {
185 + return (
186 + <TestComponent>
187 + <div />
188 + {'second'}
189 + </TestComponent>
190 + );
191 + }
192 +
193 + });
194
195 expect(function() {
196
117 - React.renderComponent(unwrappedAndAdded, document.createElement('div'));
197 + React.renderComponent(<TestContainer />, document.createElement('div'));
198
199 }).not.toThrow();
200 });
src/utils/__tests__/mapChildren-test.js
+8 -7
@@ -73,7 +73,7 @@ describe('mapChildren', function() {
73 .expectRenderedChild()
74 .instance();
75
76 - expect(mapFn).toHaveBeenCalledWith(simpleKid, 'simple', 0);
76 + expect(mapFn).toHaveBeenCalledWith(simpleKid, '0:simple', 0);
77 expect(rendered.props.children[0]).toBe(simpleKid);
78 expect(rendered.props.children).toHaveKeys(['0:simple']);
79 });
@@ -94,8 +94,9 @@ describe('mapChildren', function() {
94
95 expect(rendered.props.children[0]).not.toBe(simpleKid);
96 expect(rendered.props.children[0].props.children[0]).toBe(simpleKid);
97 - expect(rendered.props.children).toHaveKeys(['0:simple']);
98 - expect(rendered.props.children[0].props.children).toHaveKeys(['simple']);
97 + expect(rendered.props.children).toHaveKeys(['0:0:0:simple']);
98 + expect(rendered.props.children[0].props.children)
99 + .toHaveKeys(['0:simple']);
100 });
101
102 it('should be called for each child', function() {
@@ -122,12 +123,12 @@ describe('mapChildren', function() {
123 .instance();
124
125 expect(mapFn.calls.length).toBe(3);
125 - expect(mapFn).toHaveBeenCalledWith(kidOne, 'one', 0);
126 - expect(mapFn).toHaveBeenCalledWith(kidTwo, 'two', 1);
127 - expect(mapFn).toHaveBeenCalledWith(kidThree, 'three', 2);
126 + expect(mapFn).toHaveBeenCalledWith(kidOne, '0:one', 0);
127 + expect(mapFn).toHaveBeenCalledWith(kidTwo, '0:two', 1);
128 + expect(mapFn).toHaveBeenCalledWith(kidThree, '0:three', 2);
129 expect(rendered.props.children).not.toEqual(instance.props.children);
130 expect(rendered.props.children).toHaveKeys([
130 - '0:one', '0:two', '0:three'
131 + '0:0:0:one', '0:0:0:two', '0:0:0:three'
132 ]);
133 });
134 });