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 86bfc80
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 12 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 !== child && mappedChild.key ?
escapeUserProvidedKey(mappedChild.key) + '/' :
''
) +
childKey
Expand Down
40 changes: 30 additions & 10 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 @@ -159,7 +159,7 @@ describe('ReactChildren', function() {
mappedChildren[2].key,
mappedChildren[3].key,
]).toEqual(
['giraffe/.$keyZero', '/.$keyTwo', '/.3', 'keyFour/.$keyFour']
['giraffe/.$keyZero', '.$keyTwo', '.3', 'keyFour/.$keyFour']
);

expect(callback).toHaveBeenCalledWith(zero, 0);
Expand All @@ -169,8 +169,8 @@ 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[1]).toEqual(<div key=".$keyTwo" />);
expect(mappedChildren[2]).toEqual(<span key=".3" />);
expect(mappedChildren[3]).toEqual(<div key="keyFour/.$keyFour" />);
});

Expand Down Expand Up @@ -241,15 +241,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 +272,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 +310,26 @@ 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 return 0 for null children', function() {
var numberOfChildren = ReactChildren.count(null);
expect(numberOfChildren).toBe(0);
Expand Down

0 comments on commit 86bfc80

Please sign in to comment.