developerdizzle/react-virtual-list

Proposal: switch from HoC to render prop (or function as children)

Opened this issue · 10 comments

developerdizzle commented

Thinking about the possibility of replacing the HoC technique with the render prop technique.

The syntax would go from:

const MyVirtualList = VirtualList()(MyList);

return (
  <MyVirtualList
    items={myBigListOfItems}
    itemHeight={100}
  />
);

to

return (
  <VirtualList
    items={myBigListOfItems}
    itemHeight={itemHeight}
    renderVirtual={({ items, style} => (
      <ul style={style}>
        {items.map(item => (
          <li key={`item_${item.id}`} style={{height: itemHeight}}>
            Lorem ipsum dolor sit amet
          </li>
        ))}
      </ul>
    ))}
    />
);

Alternatively it could be function as children:

return (
  <VirtualList
    items={myBigListOfItems}
    itemHeight={itemHeight}
    >
    {({ items, style } => (
      <ul style={style}>
        {items.map(item => (
          <li key={`item_${item.id}`} style={{height: itemHeight}}>
            Lorem ipsum dolor sit amet
          </li>
        ))}
      </ul>
    ))}
  </VirtualList>
);

Any thoughts?

developerdizzle commented

Tagging contributors for opinions @vinnymac @jordansexton @Ramshackle-Jamathon @haxxxton @mrchief @aga5tya @andrwh

mrchief commented

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).

  • 👍1
  • 😄2
developerdizzle commented

I appreciate the input @mrchief!

I think one downside of the HoC is that MyVirtualList has to be recreated every render, or stored (likely via this.) in other component lifecycle methods (constructor, componentWillReceiveProps).

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.

mrchief commented

I think one downside of the HoC is that MyVirtualList has to be recreated every render

Not unless you're creating the HoC inside render or somewhere that leads to creation of a new instance everytime.

or stored (likely via this.) in other component lifecycle methods (constructor, componentWillReceiveProps).

Depends on implementation. Its not strictly needed. You can just as well have it as a const outside of your class/SFC and it'll be fine.

vinnymac commented

@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 react-portal has an API where they do this right now.

developerdizzle commented

Thanks @vinnymac; sounds like we're leaning towards a strict API with function as children.

You can just as well have it as a const outside of your class/SFC and it'll be fine.

@mrchief This will work for singular uses of the component, but having multiple instances of a component that use VirtualList() would cause problems.

mrchief commented

This will work for singular uses of the component, but having multiple instances of a component that use VirtualList() would cause problems.

Maybe I'm not picturing this right. What I had in mind was

const vListA = VirtualList()(MyList)
const vListB = VirtualList()(MyList)

// ...

const ComponentWithMultipleVLists = (props) => {
// ...
  return (
    <div>
      <vListA ... />
      <vListB ... />
    </div>
  )
}
developerdizzle commented

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 options parameter of VirtualList based on a parent component's props (like I do in the demo), it can cause problems. I'm hoping to alleviate that by removing the options parameter (no longer an HoC) and moving those values to properties (container, initialState, etc.) of VirtualList. Hope that makes sense; sorry for the miscommunication.

mrchief commented

@developerdizzle I like the sound of it. Do you have a branch or any sample code to look at?