-
Notifications
You must be signed in to change notification settings - Fork 558
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 React.Children.isRenderable #11
Conversation
|
||
# Detailed design | ||
|
||
A method takes a `child` component instance, finds it's corresponding fiber and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it's -> its
|
||
# Basic example | ||
|
||
`React.Children.isRenderable` lets us easily detect children without UI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would perhaps revise the naming to something like React.Children.hasRenderedOutput
. Components that render nothing are still "renderable" if that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
How is this different from |
@thysultan |
I'm not sure how feasible this is, as it would require rendering the component with a set of props to determine whether it actually renders something. Especially considering that the return of a component can change between renders and the props passed. |
@milesj I'm sorry if it's not clear from the RFC. This method accepts an instantiated component (a React element) and not a class or a function. This is how other methods from |
I wonder if you've seen Ryan's call-return experiment? |
@bvaughn, thank you for the link! I am not sure if I got the idea of call-return clearly (the video is not available anymore and all mentions lead to this video), and the use case doesn't seem obvious from the code. Although I noticed those |
```js | ||
const MaybeWrapper = ({children}) => | ||
React.Children.map(children, child => | ||
React.Children.isRenderable(child) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why you want this method on React.children
and not React.isRenderable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be convenient when we use it for whatever is passed to children
prop, although my own example provides a workaround for this situation. I agree, that this is an arguable detail.
|
||
Why should we *not* do this? Please consider: | ||
|
||
- the proposed feature can be implemented in user space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on how would this be achievable in user space? Perhaps if it can be shown that it would be complex enough to do so, it could help your case.
For instance, it may be naivety on my side, but I'm not sure how isRenderable(child)
is different than simply doing child != null
. Perhaps this will detect the rendering of blank strings and return false
for those too? Or perhaps it'll do the same that that condition does, but more efficiently? I'm not entirely familiar with fiber internals to fully make sense of the details in the "Detailed design" section above.
Thanks for the RFC! I realize this is a pretty late review, see #182 on some discussion around that. We'd like to not expand the Your RFC appears to be more ambitious — it would actually check children "deeply". This is not how any of the existing
I don't think this works. You're describing a check against the tree that already exists. But the code executes for the tree that is currently being rendered. That could be a completely different tree from the one that already exists — it could have different props or structure. So this implementation doesn't solve the problem and would be inconsistent. There is also a flaw in that it assumes you can get an internal instance (a fiber) based on a
An element is not an "instantiated component", it's only an object like
Call/Return is our much earlier exploration of this idea. Here's roughly how it worked. A child component could return a special value, called a "return". E.g. This feature kind of worked but we thought it's pretty confusing (however, it did specify solutions to the problems outlined above). So we did not proceed with that API and ended up deleting it. But a future attempt to bring something like this back would probably need to similarly "powerful" but also much more usable. I'll also add a comment from @sebmarkbage from when we were last discussing this feature internally:
I think what Seb meant by it being possible in userspace is that technically React Hooks are implemented using a "dispatcher" which we switch during runtime to the appropriate implementation. So a Hook like I'll close this proposal, since in the current form it's very unlikely to happen. |
This RFC specifies a
React.Children.isRenderable
method which allows checking whether a child component (or it's children) return a node or doesn't return anything.