Skip to content

Reimplement React.Children #9339

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

Closed

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented Apr 5, 2017

@aweary
This is a fix for #9325 (comment)

@koba04 koba04 changed the title Remove to use PooledClass for traverseAllChildren Remove usage of PooledClass for traverseAllChildren Apr 5, 2017
@aweary aweary self-assigned this Apr 5, 2017
@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2017

What do you think about completely rewriting React.Children implementation? It seems like traverseAllChildren is very generic and might be suboptimal by itself for React.Children implementation. Do you think you could re-implement React.Children with less abstraction overhead (it’s fine to copy and paste common parts). We could then look if the abstraction is worth it.

Would you be interested?

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

See my comment—maybe we can both avoid allocation and make the code less abstract.

@aweary
Copy link
Contributor

aweary commented Apr 10, 2017

@koba04 happy to work with you on that ^ if you'd like!

@koba04
Copy link
Contributor Author

koba04 commented Apr 11, 2017

@gaearon @aweary That makes sense! I'll work on that!

@koba04 koba04 changed the title Remove usage of PooledClass for traverseAllChildren [WIP] Reimplement React.Children Apr 12, 2017
@koba04
Copy link
Contributor Author

koba04 commented Apr 14, 2017

@aweary @gaearon

Could we remove ReactChildren.mapIntoWithKeyPrefixInternal?

It's not used in other React sources. I'm not sure whether it's being used in outside React, but it is an undocumented API.

@aweary
Copy link
Contributor

aweary commented Apr 14, 2017

I don't see it being used in react-native or react-art. This comment implies it's used by an addon, but I'm not sure which one. Maybe @trueadm can clarify.

@gaearon
Copy link
Collaborator

gaearon commented Apr 14, 2017

We have an internal dependency on it at FB but we'll fix it in another way. So feel free to skip it.

@koba04 koba04 force-pushed the remove-pooling-traverse-all-children branch 2 times, most recently from 63c5ed2 to 5c79c79 Compare April 21, 2017 01:06
@koba04 koba04 changed the title [WIP] Reimplement React.Children Reimplement React.Children Apr 21, 2017
@koba04 koba04 force-pushed the remove-pooling-traverse-all-children branch from 8e151f8 to d2626ad Compare May 8, 2017 09:27
forEachFunc: () => mixed,
forEachContext?: Object,
): void {
mapChildren(children, forEachFunc, forEachContext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't want to implement forEach in terms of map because we don't want to allocate the result just to ignore it. It's fine to duplicate the code in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I've fixed it!

@koba04 koba04 force-pushed the remove-pooling-traverse-all-children branch from 1379048 to 771a532 Compare June 12, 2017 07:17
@koba04 koba04 force-pushed the remove-pooling-traverse-all-children branch from 90fc5e5 to 61981da Compare June 29, 2017 05:22
@koba04
Copy link
Contributor Author

koba04 commented Jul 21, 2017

duplicate of #10227

@koba04 koba04 closed this Jul 21, 2017
@gaearon
Copy link
Collaborator

gaearon commented Jul 21, 2017

Hey, sorry I didn't provide more feedback on this. As you see from my implementation I still wanted to keep the pooling. Thanks for working on it though, it helped me gather my thoughts and figure out how to go about this.

@koba04
Copy link
Contributor Author

koba04 commented Jul 21, 2017

No problem! 😄
I've enjoyed this implementation and learned a lot from this!

@koba04 koba04 deleted the remove-pooling-traverse-all-children branch July 21, 2017 11:45
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