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

add Portal support to React.Children calls #11378

Merged
merged 1 commit into from Oct 31, 2017

Conversation

MatteoVH
Copy link

Solves #11373

Not sure if Portals should expose their children to the React.Children call or just the portal itself. Right now it's set up to expose the portal.

@gaearon
Copy link
Collaborator

gaearon commented Oct 26, 2017

Now that I think about it, maybe this is actually a breaking change and we can't get it in. Since existing Children users don't assume portals will be there (with different object shape).

@MatteoVH
Copy link
Author

MatteoVH commented Oct 26, 2017

Well, the React.Children.map documentation says:

Invokes a function on every immediate child contained within children

And since Portals are included in the instance.children data structure I'd argue that this is a bug that should be fixed, not a breaking change. Right now React.Children.map errors when a Portal is inside of the children passed into it.

    Invariant Violation: Objects are not valid as a React child (found: object with keys {$$typeof, key, children, containerInfo, implementation}). If you meant to render a collection of children, use an array instead.null

      at invariant (node_modules/fbjs/lib/invariant.js:42:15)
      at traverseAllChildrenImpl (packages/react/src/ReactChildren.js:200:25)
      at traverseAllChildren (packages/react/src/ReactChildren.js:234:10)
      at Object.forEachChildren [as forEach] (packages/react/src/ReactChildren.js:286:3)
      at Object.<anonymous> (packages/react/src/__tests__/ReactChildren-test.js:64:20)

That being said, I'm still not that knowledgable in this code base, so I understand if you think differently.

@gaearon
Copy link
Collaborator

gaearon commented Oct 26, 2017

Oh I assumed we skip over it. In this case it seems fine to make this work.

Maybe it'd be nice if we also released #11279 with this. So that people have a way to check if something is a portal when iterating over children.

@MatteoVH
Copy link
Author

Yeah, good idea! I agree. I was hesitant to expose Portals right to the React.Children call because I wasn't sure how to differentiate them from other components, but that seems like a good way to solve that.

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.

Confirmed with @sebmarkbage this should be fine.

@gaearon gaearon merged commit 0752a63 into facebook:master Oct 31, 2017
@gaearon
Copy link
Collaborator

gaearon commented Oct 31, 2017

Let's also support Call and Return in a followup PR, if you'd like to send one?

@MatteoVH
Copy link
Author

Great, I'll take care of that!

Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
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.

None yet

3 participants