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

Avoid indexOf() during unmounting a root in the hook #7496

Merged
merged 2 commits into from
Aug 15, 2016
Merged

Avoid indexOf() during unmounting a root in the hook #7496

merged 2 commits into from
Aug 15, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 14, 2016

Addresses @spicyj’s concerns in #7491 (comment).

@gaearon gaearon added this to the 15-next milestone Aug 14, 2016
itemMap.delete(id);
return;
} else {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced early returns with branches because it makes these two paths clearer in my opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I agree.

@ghost ghost added the CLA Signed label Aug 14, 2016
if (canUseCollections) {
return Array.from(rootIDSet.keys());
} else {
return Object.keys(rootByKey).map(getIDFromKey);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we stored the id as the value then this could be Object.values(rootByKey). I suppose we don't know that's polyfilled though.

@sophiebits
Copy link
Collaborator

lgtm


function getRegisteredIDs() {
if (canUseCollections) {
return Array.from(itemMap.keys());
Copy link
Contributor

@syranide syranide Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.keys() is not yet supported in all implementations, IE supposedly does not support this. EDIT: Array.from is even less widely supported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I check for Array.from in canUseCollections.
Do you mean that a native Map may exist without Map.prototype.keys?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems so (thanks for the tip!) I'll update to check for keys() specifically.

Copy link
Contributor

@syranide syranide Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad I missed it. It should sufficiently cover the case of .keys() I imagine 👍 EDIT: Yep, .keys() is not part of "basic support".

@gaearon gaearon merged commit 4847578 into facebook:master Aug 15, 2016
@gaearon gaearon deleted the use-set-in-hook branch August 15, 2016 14:47
@zpao zpao modified the milestones: 15.3.1, 15-next Aug 15, 2016
zpao pushed a commit that referenced this pull request Aug 15, 2016
* Avoid indexOf() during unmounting a root in the hook

* Check for (Map|Set).prototype.keys

(cherry picked from commit 4847578)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants