Respect 'key' prop for object identity

Now when a `key` prop appears, its value is always honored. This means that in the root component or as an only child, changing key will cause remounting; in a `children` object, the `key` prop will be joined with the object key to form a two-part key.

Fixes #590.

src/core/ReactComponent.js

@@ -155,23 +155,6 @@ var ReactComponent = {
     );
   },
 
-  /**
-   * Generate a key string that identifies a component within a set.
-   *
-   * @param {*} component A component that could contain a manual key.
-   * @param {number} index Index that is used if a manual key is not provided.
-   * @return {string}
-   * @internal
-   */
-  getKey: function(component, index) {
-    if (component && component.props && component.props.key != null) {
-      // Explicit key
-      return '{' + component.props.key + '}';
-    }
-    // Implicit key determined by the index in the set
-    return '[' + index + ']';
-  },
-
   /**
    * @internal
    */

src/core/__tests__/ReactIdentity-test.js

@@ -55,8 +55,8 @@ describe('ReactIdentity', function() {
     React.renderComponent(instance, document.createElement('div'));
     var node = instance.getDOMNode();
     reactComponentExpect(instance).toBeDOMComponentWithChildCount(2);
-    checkId(node.childNodes[0], '.r[0].{first}');
-    checkId(node.childNodes[1], '.r[0].{second}');
+    checkId(node.childNodes[0], '.r[0].{first}[0]');
+    checkId(node.childNodes[1], '.r[0].{second}[0]');
   });
 
   it('should allow key property to express identity', function() {
@@ -117,7 +117,7 @@ describe('ReactIdentity', function() {
     expect(span2.getDOMNode()).not.toBe(null);
 
     checkId(span1.getDOMNode(), '.r[0].{' + key + '}');
-    checkId(span2.getDOMNode(), '.r[0].[1]{' + key + '}');
+    checkId(span2.getDOMNode(), '.r[0].[1]{' + key + '}[0]');
   }
 
   it('should allow any character as a key, in a detached parent', function() {

src/core/__tests__/ReactMount-test.js

@@ -35,4 +35,39 @@ describe('ReactMount', function() {
     ReactMount.renderComponent(<span></span>, container);
     expect(container.firstChild.nodeName).toBe('SPAN');
   });
+
+  it('should unmount and remount if the key changes', function() {
+    var container = document.createElement('container');
+
+    var mockMount = mocks.getMockFunction();
+    var mockUnmount = mocks.getMockFunction();
+
+    var Component = React.createClass({
+      componentDidMount: mockMount,
+      componentWillUnmount: mockUnmount,
+      render: function() {
+        return <span>{this.props.text}</span>;
+      }
+    });
+
+    expect(mockMount.mock.calls.length).toBe(0);
+    expect(mockUnmount.mock.calls.length).toBe(0);
+
+    ReactMount.renderComponent(<Component text="orange" key="A" />, container);
+    expect(container.firstChild.innerText).toBe('orange');
+    expect(mockMount.mock.calls.length).toBe(1);
+    expect(mockUnmount.mock.calls.length).toBe(0);
+
+    // If we change the key, the component is unmounted and remounted
+    ReactMount.renderComponent(<Component text="green" key="B" />, container);
+    expect(container.firstChild.innerText).toBe('green');
+    expect(mockMount.mock.calls.length).toBe(2);
+    expect(mockUnmount.mock.calls.length).toBe(1);
+
+    // But if we don't change the key, the component instance is reused
+    ReactMount.renderComponent(<Component text="blue" key="B" />, container);
+    expect(container.firstChild.innerText).toBe('blue');
+    expect(mockMount.mock.calls.length).toBe(2);
+    expect(mockUnmount.mock.calls.length).toBe(1);
+  });
 });

src/core/__tests__/ReactMultiChild-test.js

@@ -125,6 +125,34 @@ describe('ReactMultiChild', function() {
       expect(mockMount.mock.calls.length).toBe(2);
       expect(mockUnmount.mock.calls.length).toBe(1);
     });
+
+    it('should replace children with different keys', function() {
+      var container = document.createElement('div');
+
+      var mockMount = mocks.getMockFunction();
+      var mockUnmount = mocks.getMockFunction();
+
+      var MockComponent = React.createClass({
+        componentDidMount: mockMount,
+        componentWillUnmount: mockUnmount,
+        render: function() {
+          return <span />;
+        }
+      });
+
+      expect(mockMount.mock.calls.length).toBe(0);
+      expect(mockUnmount.mock.calls.length).toBe(0);
+
+      React.renderComponent(<div><MockComponent key="A" /></div>, container);
+
+      expect(mockMount.mock.calls.length).toBe(1);
+      expect(mockUnmount.mock.calls.length).toBe(0);
+
+      React.renderComponent(<div><MockComponent key="B" /></div>, container);
+
+      expect(mockMount.mock.calls.length).toBe(2);
+      expect(mockUnmount.mock.calls.length).toBe(1);
+    });
   });
 
   describe('innerHTML', function() {

src/core/__tests__/ReactMultiChildReconcile-test.js

@@ -42,11 +42,11 @@ var stripEmptyValues = function(obj) {
 };
 
 /**
- * Child names is are wrapped in an prefix and suffix character. We strip those
- * out. This relies on a tiny implementation detail of the rendering system.
+ * Child key names are wrapped like '{key}[0]'. We strip the extra chars out
+ * here. This relies on an implementation detail of the rendering system.
  */
 var getOriginalKey = function(childName) {
-  return childName.substr(1, childName.length - 2);
+  return childName.slice('{'.length, childName.length - '}[0]'.length);
 };
 
 /**

src/core/getComponentKey.js

@@ -0,0 +1,38 @@
+/**
+ * Copyright 2013 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * @providesModule getComponentKey
+ */
+
+"use strict";
+
+/**
+ * Generate a key string that identifies a component within a set.
+ *
+ * @param {*} component A component that could contain a manual key.
+ * @param {number} index Index that is used if a manual key is not provided.
+ * @return {string}
+ * @internal
+ */
+function getComponentKey(component, index) {
+  if (component && component.props && component.props.key != null) {
+    // Explicit key
+    return '{' + component.props.key + '}';
+  }
+  // Implicit key determined by the index in the set
+  return '[' + index + ']';
+}
+
+module.exports = getComponentKey;

src/core/shouldUpdateReactComponent.js

@@ -19,6 +19,8 @@
 
 "use strict";
 
+var getComponentKey = require('getComponentKey');
+
 /**
  * Given a `prevComponent` and `nextComponent`, determines if `prevComponent`
  * should be updated as opposed to being destroyed or replaced.
@@ -32,7 +34,10 @@ function shouldUpdateReactComponent(prevComponent, nextComponent) {
   // TODO: Remove warning after a release.
   if (prevComponent && nextComponent &&
       prevComponent.constructor === nextComponent.constructor) {
-    if (prevComponent._owner === nextComponent._owner) {
+    if (prevComponent._owner === nextComponent._owner && (
+          getComponentKey(prevComponent, /* index: */ 0) ===
+          getComponentKey(nextComponent, /* index: */ 0)
+        )) {
       return true;
     } else {
       if (__DEV__) {

src/utils/__tests__/ReactChildren-test.js

@@ -124,7 +124,7 @@ describe('ReactChildren', function() {
     var three = null;
     var four = <div key="keyFour" />;
 
-    var zeroMapped = <div key="giraffe" />;  // Key should be overridden
+    var zeroMapped = <div key="giraffe" />;  // Key should be joined to obj key
     var oneMapped = null;  // Key should be added even if we don't supply it!
     var twoMapped = <div />;  // Key should be added even if not supplied!
     var threeMapped = <span />; // Map from null to something.
@@ -188,13 +188,12 @@ describe('ReactChildren', function() {
     var two = <div key="keyTwo" />;
     var three = null;
     var four = <div key="keyFour" />;
-    var five = <div key="keyFiveCompletelyIgnored" />;
-    // five is placed into a JS object with a key that takes precedence over the
+    var five = <div key="keyFiveInner" />;
+    // five is placed into a JS object with a key that is joined to the
     // component key attribute.
     // Precedence is as follows:
-    // 1. JavaScript Object key if in a JavaScript object:
-    // 2. If grouped in an Array, the `key` attribute.
-    // 3. The array index if in a JavaScript Array.
+    // 1. If grouped in an Object, the object key combined with `key` prop
+    // 2. If grouped in an Array, the `key` prop, falling back to array index
 
     var zeroMapped = <div key="giraffe" />;  // Key should be overridden
     var oneMapped = null;  // Key should be added even if we don't supply it!
@@ -236,12 +235,12 @@ describe('ReactChildren', function() {
     expect(mappedKeys.length).toBe(6);
     // Keys default to indices.
     expect(mappedKeys).toEqual([
-      '[0]{firstHalfKey}{keyZero}',
-      '[0]{firstHalfKey}[1]',
-      '[0]{firstHalfKey}{keyTwo}',
-      '[0]{secondHalfKey}[0]',
-      '[0]{secondHalfKey}{keyFour}',
-      '[0]{keyFive}'
+      '[0]{firstHalfKey}[0]{keyZero}',
+      '[0]{firstHalfKey}[0][1]',
+      '[0]{firstHalfKey}[0]{keyTwo}',
+      '[0]{secondHalfKey}[0][0]',
+      '[0]{secondHalfKey}[0]{keyFour}',
+      '[0]{keyFive}{keyFiveInner}'
     ]);
 
     expect(callback).toHaveBeenCalledWith(zero, 0);
@@ -267,7 +266,7 @@ describe('ReactChildren', function() {
     var zeroForceKey = <div key="keyZero" />;
     var oneForceKey = <div key="keyOne" />;
 
-    // Key should be overridden
+    // Key should be joined to object key
     var zeroForceKeyMapped = <div key="giraffe" />;
     // Key should be added even if we don't supply it!
     var oneForceKeyMapped = <div />;
@@ -289,7 +288,7 @@ describe('ReactChildren', function() {
     var mappedForcedKeys = Object.keys(mappedChildrenForcedKeys);
     expect(mappedForcedKeys).toEqual(expectedForcedKeys);
 
-    var expectedRemappedForcedKeys = ['{{keyZero}}', '{{keyOne}}'];
+    var expectedRemappedForcedKeys = ['{{keyZero}}{giraffe}', '{{keyOne}}[0]'];
     var remappedChildrenForcedKeys =
       ReactChildren.map(mappedChildrenForcedKeys, mapFn);
     expect(

src/utils/__tests__/traverseAllChildren-test.js

@@ -141,11 +141,12 @@ describe('traverseAllChildren', function() {
     var two = <div key="keyTwo" />;
     var three = null;
     var four = <div key="keyFour" />;
-    var five = <div key="keyFiveCompletelyIgnored" />;
-    // Name precedence is as follows:
-    // 1. JavaScript Object key if in a JavaScript object:
-    // 2. If grouped in an Array, the `key` attribute.
-    // 3. The array index if in a JavaScript Array.
+    var five = <div key="keyFiveInner" />;
+    // five is placed into a JS object with a key that is joined to the
+    // component key attribute.
+    // Precedence is as follows:
+    // 1. If grouped in an Object, the object key combined with `key` prop
+    // 2. If grouped in an Array, the `key` prop, falling back to array index
 
 
     var traverseContext = [];
@@ -170,32 +171,40 @@ describe('traverseAllChildren', function() {
     expect(traverseFn).toHaveBeenCalledWith(
       traverseContext,
       zero,
-      '[0]{firstHalfKey}{keyZero}',
+      '[0]{firstHalfKey}[0]{keyZero}',
       0
     );
 
     expect(traverseFn)
-      .toHaveBeenCalledWith(traverseContext, one, '[0]{firstHalfKey}[1]', 1);
+      .toHaveBeenCalledWith(traverseContext, one, '[0]{firstHalfKey}[0][1]', 1);
 
     expect(traverseFn).toHaveBeenCalledWith(
       traverseContext,
       two,
-      '[0]{firstHalfKey}{keyTwo}',
+      '[0]{firstHalfKey}[0]{keyTwo}',
       2
     );
 
-    expect(traverseFn)
-      .toHaveBeenCalledWith(traverseContext, three, '[0]{secondHalfKey}[0]', 3);
+    expect(traverseFn).toHaveBeenCalledWith(
+      traverseContext,
+      three,
+      '[0]{secondHalfKey}[0][0]',
+      3
+    );
 
     expect(traverseFn).toHaveBeenCalledWith(
       traverseContext,
       four,
-      '[0]{secondHalfKey}{keyFour}',
+      '[0]{secondHalfKey}[0]{keyFour}',
       4
     );
 
-    expect(traverseFn)
-      .toHaveBeenCalledWith(traverseContext, five, '[0]{keyFive}', 5);
+    expect(traverseFn).toHaveBeenCalledWith(
+      traverseContext,
+      five,
+      '[0]{keyFive}{keyFiveInner}',
+      5
+    );
   });
 
   it('should retain key across two mappings', function() {

src/utils/traverseAllChildren.js

@@ -18,9 +18,9 @@
 
 "use strict";
 
-var ReactComponent = require('ReactComponent');
 var ReactTextComponent = require('ReactTextComponent');
 
+var getComponentKey = require('getComponentKey');
 var invariant = require('invariant');
 
 /**
@@ -46,7 +46,7 @@ var traverseAllChildrenImpl =
     if (Array.isArray(children)) {
       for (var i = 0; i < children.length; i++) {
         var child = children[i];
-        var nextName = nameSoFar + ReactComponent.getKey(child, i);
+        var nextName = nameSoFar + getComponentKey(child, i);
         var nextIndex = indexSoFar + subtreeCount;
         subtreeCount += traverseAllChildrenImpl(
           child,
@@ -61,9 +61,7 @@ var traverseAllChildrenImpl =
       var isOnlyChild = nameSoFar === '';
       // If it's the only child, treat the name as if it was wrapped in an array
       // so that it's consistent if the number of children grows
-      var storageName = isOnlyChild ?
-        ReactComponent.getKey(children, 0):
-        nameSoFar;
+      var storageName = isOnlyChild ? getComponentKey(children, 0) : nameSoFar;
       if (children === null || children === undefined || type === 'boolean') {
         // All of the above are perceived as null.
         callback(traverseContext, null, storageName, indexSoFar);
@@ -82,7 +80,7 @@ var traverseAllChildrenImpl =
             if (children.hasOwnProperty(key)) {
               subtreeCount += traverseAllChildrenImpl(
                 children[key],
-                nameSoFar + '{' + key + '}',
+                nameSoFar + '{' + key + '}' + getComponentKey(children[key], 0),
                 indexSoFar + subtreeCount,
                 callback,
                 traverseContext