-
Notifications
You must be signed in to change notification settings - Fork 73
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
Proposal: switch from HoC to render prop (or function as children) #75
Comments
Based on my real world experience, here are my thoughts: Children props approach is the cleanest in terms of the component API - but it's also the quirkiest because children is an opaque data structure. That doesn't necessarily mean it's going to be harder than others - that solely depends on how we use the children prop - but in terms of flexibility, it does introduce constraints (we can only do what React lets us). render props - its looks slightly convoluted compared to children props - but since it's just a simple array, it gives us lots of flexibility on how we want to manipulate/use it. Between render props and HOC, with the example given in OP, I think HOC is cleaner (code-wise). It's well understood and works and there are no downsides as far as I can tell. So I'd vote for children props if its easy enough to support, otherwise keep the current HOC implementation. Last but not the least, I'm thankful to @developerdizzle for counting me as a contributor, even though I've had a very small contribution and I'm not innately familiar with the code (hence my uncertainty about using children prop support). |
I appreciate the input @mrchief! I think one downside of the HoC is that I wonder if it's possible to support all three? Children and render props are similar enough that it seems like it'd be easy to implement both. |
Not unless you're creating the HoC inside render or somewhere that leads to creation of a new instance everytime.
Depends on implementation. Its not strictly needed. You can just as well have it as a |
@developerdizzle I would advocate against offering multiple ways for someone to render the list. It will make debugging issues more difficult and will be harder to communicate how to properly use this component. Plus users don’t have to make any decisions when you only have a single API for them to use. I agree with all the things @mrchief has said. Since the HoC is going to rerender as you say it will, my vote is for the children prop being enforced as a function. I believe |
Thanks @vinnymac; sounds like we're leaning towards a strict API with function as children.
@mrchief This will work for singular uses of the component, but having multiple instances of a component that use |
Maybe I'm not picturing this right. What I had in mind was
|
Ah, okay! I believe that will work. I should have been clearer before, but if you run into a situation where you need to change the |
@developerdizzle I like the sound of it. Do you have a branch or any sample code to look at? |
Update on this - branch is in the works https://github.com/developerdizzle/react-virtual-list/tree/function-as-children |
Thinking about the possibility of replacing the HoC technique with the render prop technique.
The syntax would go from:
to
Alternatively it could be function as children:
Any thoughts?
The text was updated successfully, but these errors were encountered: