Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReactChildren.map: only add slash if new child has key #5892

Merged
merged 2 commits into from Feb 11, 2016

Conversation

Projects
None yet
7 participants
@ianobermiller
Copy link
Contributor

commented Jan 21, 2016

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.

Test Plan:

npm test -- src/isomorphic/children/__tests__/ReactChildren-test.js

@ianobermiller

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2016

cc @spicyj

@ianobermiller ianobermiller force-pushed the ianobermiller:children-map-key-slash branch from 4111232 to 39410e7 Jan 21, 2016

@ianobermiller ianobermiller changed the title Children map key slash ReactChildren.map: only add slash if new child has key Jan 21, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 21, 2016

@ianobermiller updated the pull request.

@@ -117,7 +117,7 @@ function mapSingleChildIntoContext(bookKeeping, child, childKey) {
// traverseAllChildren used to do for objects as children
keyPrefix +
(
mappedChild !== child ?
mappedChild !== child && mappedChild.key ?
escapeUserProvidedKey(mappedChild.key || '') + '/' :

This comment has been minimized.

Copy link
@zpao

zpao Jan 21, 2016

Member

Assume Ben likes this, can you drop the || '' here since we already know we have it.

This comment has been minimized.

Copy link
@sophiebits

sophiebits Jan 21, 2016

Collaborator

Haven't thought about this in super detail yet but why not mappedChild.key !== child.key as I suggested in chat?

This comment has been minimized.

Copy link
@ianobermiller

ianobermiller Jan 21, 2016

Author Contributor

Yes, that makes sense, need to add a null-check then. Still need a truthy check on mappedChild.key, so mappedChild.key && (!child || mappedChild.key !== child.key)

@ianobermiller

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2016

Nice catch, thanks.

@ianobermiller ianobermiller force-pushed the ianobermiller:children-map-key-slash branch from 39410e7 to 86bfc80 Jan 21, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 21, 2016

@ianobermiller updated the pull request.

ReactChildren.map: only add slash if new child has key
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.

@ianobermiller ianobermiller force-pushed the ianobermiller:children-map-key-slash branch from 86bfc80 to 30f7641 Jan 21, 2016

@@ -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))) ?

This comment has been minimized.

Copy link
@ianobermiller

ianobermiller Jan 21, 2016

Author Contributor

I hate all the parens, but this makes it crystal clear.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 21, 2016

@ianobermiller updated the pull request.

@RoccoC

This comment has been minimized.

Copy link

commented Feb 11, 2016

Curious if/when this PR will be merged into master? This PR will fix a few problems we are dealing with in our project. Thanks!

sophiebits added a commit that referenced this pull request Feb 11, 2016

Merge pull request #5892 from ianobermiller/children-map-key-slash
ReactChildren.map: only add slash if new child has key

@sophiebits sophiebits merged commit 3e41da7 into facebook:master Feb 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Erid

This comment has been minimized.

Copy link

commented Feb 11, 2016

I'm unfamiliar with the process... When can we expect it to be published on npm?

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

After the next release major release, which is probably less than a month away :).

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.