Skip to content

Commit

Permalink
ReactChildren.map: only add slash if new child has key
Browse files Browse the repository at this point in the history
See the new test for the scenario I am trying to fix; if you clone an
element in React.cloneElement, vs just returning it directly, you will
get a different key (with a slash in front) even though the two
children are identical.
  • Loading branch information
ianobermiller committed Jan 21, 2016
1 parent 76da1f8 commit 30f7641
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 23 deletions.
4 changes: 2 additions & 2 deletions src/isomorphic/children/ReactChildren.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ function mapSingleChildIntoContext(bookKeeping, child, childKey) {
// traverseAllChildren used to do for objects as children
keyPrefix +
(
mappedChild !== child ?
escapeUserProvidedKey(mappedChild.key || '') + '/' :
(mappedChild.key && (!child || (child.key !== mappedChild.key))) ?
escapeUserProvidedKey(mappedChild.key) + '/' :
''
) +
childKey
Expand Down
80 changes: 59 additions & 21 deletions src/isomorphic/children/__tests__/ReactChildren-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ describe('ReactChildren', function() {
expect(ReactChildren.count(mappedChildren)).toBe(1);
expect(mappedChildren[0]).not.toBe(simpleKid);
expect(mappedChildren[0].props.children).toBe(simpleKid);
expect(mappedChildren[0].key).toBe('/.$simple');
expect(mappedChildren[0].key).toBe('.$simple');
});

it('should invoke callback with the right context', function() {
Expand Down Expand Up @@ -117,17 +117,15 @@ describe('ReactChildren', function() {
var three = null;
var four = <div key="keyFour" />;

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.
var fourMapped = <div key="keyFour" />;

var mapped = [
<div key="giraffe" />, // Key should be joined to obj key
null, // Key should be added even if we don't supply it!
<div />, // Key should be added even if not supplied!
<span />, // Map from null to something.
<div key="keyFour" />,
];
var callback = jasmine.createSpy().andCallFake(function(kid, index) {
return index === 0 ? zeroMapped :
index === 1 ? oneMapped :
index === 2 ? twoMapped :
index === 3 ? threeMapped : fourMapped;
return mapped[index];
});

var instance = (
Expand Down Expand Up @@ -159,7 +157,7 @@ describe('ReactChildren', function() {
mappedChildren[2].key,
mappedChildren[3].key,
]).toEqual(
['giraffe/.$keyZero', '/.$keyTwo', '/.3', 'keyFour/.$keyFour']
['giraffe/.$keyZero', '.$keyTwo', '.3', '.$keyFour']
);

expect(callback).toHaveBeenCalledWith(zero, 0);
Expand All @@ -169,9 +167,9 @@ describe('ReactChildren', function() {
expect(callback).toHaveBeenCalledWith(four, 4);

expect(mappedChildren[0]).toEqual(<div key="giraffe/.$keyZero" />);
expect(mappedChildren[1]).toEqual(<div key="/.$keyTwo" />);
expect(mappedChildren[2]).toEqual(<span key="/.3" />);
expect(mappedChildren[3]).toEqual(<div key="keyFour/.$keyFour" />);
expect(mappedChildren[1]).toEqual(<div key=".$keyTwo" />);
expect(mappedChildren[2]).toEqual(<span key=".3" />);
expect(mappedChildren[3]).toEqual(<div key=".$keyFour" />);
});

it('should be called for each child in nested structure', function() {
Expand Down Expand Up @@ -241,15 +239,15 @@ describe('ReactChildren', function() {
mappedChildren[3].key,
]).toEqual([
'giraffe/.0:$firstHalfKey/.$keyZero',
'/.0:$firstHalfKey/.$keyTwo',
'.0:$firstHalfKey/.$keyTwo',
'keyFour/.0:$secondHalfKey/.$keyFour',
'/.0:$keyFive/.$keyFiveInner',
'.0:$keyFive/.$keyFiveInner',
]);

expect(mappedChildren[0]).toEqual(<div key="giraffe/.0:$firstHalfKey/.$keyZero" />);
expect(mappedChildren[1]).toEqual(<div key="/.0:$firstHalfKey/.$keyTwo" />);
expect(mappedChildren[1]).toEqual(<div key=".0:$firstHalfKey/.$keyTwo" />);
expect(mappedChildren[2]).toEqual(<div key="keyFour/.0:$secondHalfKey/.$keyFour" />);
expect(mappedChildren[3]).toEqual(<div key="/.0:$keyFive/.$keyFiveInner" />);
expect(mappedChildren[3]).toEqual(<div key=".0:$keyFive/.$keyFiveInner" />);
});

it('should retain key across two mappings', function() {
Expand All @@ -272,15 +270,15 @@ describe('ReactChildren', function() {
</div>
);

var expectedForcedKeys = ['giraffe/.$keyZero', '/.$keyOne'];
var expectedForcedKeys = ['giraffe/.$keyZero', '.$keyOne'];
var mappedChildrenForcedKeys =
ReactChildren.map(forcedKeys.props.children, mapFn);
var mappedForcedKeys = mappedChildrenForcedKeys.map((c) => c.key);
expect(mappedForcedKeys).toEqual(expectedForcedKeys);

var expectedRemappedForcedKeys = [
'giraffe/.$giraffe/.$keyZero',
'/.$/.$keyOne',
'.$.$keyOne',
];
var remappedChildrenForcedKeys =
ReactChildren.map(mappedChildrenForcedKeys, mapFn);
Expand Down Expand Up @@ -310,6 +308,46 @@ describe('ReactChildren', function() {
}).not.toThrow();
});

it('should use the same key for a cloned element', function() {
var instance = (
<div>
<div />
</div>
);

var mapped = ReactChildren.map(
instance.props.children,
element => element,
);

var mappedWithClone = ReactChildren.map(
instance.props.children,
element => React.cloneElement(element),
);

expect(mapped[0].key).toBe(mappedWithClone[0].key);
});

it('should use the same key for a cloned element with key', function() {
var instance = (
<div>
<div key="unique" />
</div>
);

var mapped = ReactChildren.map(
instance.props.children,
element => element,
);

var mappedWithClone = ReactChildren.map(
instance.props.children,
element => React.cloneElement(element, {key: 'unique'}),
);

expect(mapped[0].key).toBe(mappedWithClone[0].key);
});

it('should return 0 for null children', function() {
var numberOfChildren = ReactChildren.count(null);
expect(numberOfChildren).toBe(0);
Expand Down

0 comments on commit 30f7641

Please sign in to comment.