-
-
Notifications
You must be signed in to change notification settings - Fork 787
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
Ability to pass a render function for an item #85
Comments
I could provide an inline function component which then returns my full component with whatever props I passed into it, but that's doubling the amount the wrappers and seems unnecessary. |
Your timing here is funny. I'm having a similar conversation over on bvaughn/react-error-boundary/issues/26 this morning. 😄
The name of the prop itself isn't that significant, but I chose If you dislike this syntax <FixedSizeList
children={( index, style ) => (
// ...
)}
{...props}
/> (Although I would caution against that if your list item is doing anything complicated, since React wouldn't be able to memoize it.) The important distinction between
It's interesting that you see it this way. 🙂 I think it actually helps simplify memoization, because it allows you to use a class component that extends
To be clear, you could already do this by adding an inline function component wreapper with local scope, but recreating this wrapper on render would change its "type" and this would cause React to unmount and remount (rather than update) your items, which is definitely not what you want. This is a limitation of my API design.
My initial reaction is a negative one. I don't want to increase the complexity of the implementation (or the docs). I don't think there's a compelling argument for this, but let's keep talking. Maybe you can change my mind! |
It is an interesting discussion! I think we're coming at it from different angles, let me try to explain my side more.
I wasn't focused on the syntax, but more that other libraries provide a prop which is a function that returns a component, rather than passing a component type directly to it. As you mentioned, I don't want to create an inline function component since that has other limitations (can't memo, want to split out complicated stuff, etc)
With a render function you can return any type of component, no? I don't consider a render function a "function component" - the The point about
That's true, but I wonder how much of a benefit this really is? Since React components are lazy, it's not like it's actually rendering the components; rendering them could still be async. But returning ~100 components probably isn't a huge deal.
Not sure what you mean - that the lib can add the
I must be missing something? See the example - if you need to pass props down you now need to memoize the data object with a separate library. With a render function I get exactly what you say - I can use anything I want to, just return whatever component that has I think my main point is recognizing that you are always going to have to pass down additional data. Right now the Item component only gets |
Another option would be to spread the additional props so they are passed to your component like normal props: <List itemData={{ onSelect: this.onSelect }}>{Item}</List>
// The lib renders Item like `<Item {...itemData} />` so:
function Item({ onSelect }) {
// ...
} Now |
My experience has been the opposite– that they can be a burden because they require you to add an additional layer around any work to properly minimize it. In the context of a windowed list, this can hurt performance if people end up doing non-trivial work in their wrapper function. I often got asked with
You're right! Although this relies on people to put any "heavy lifting" behind their own (memoized) Component and in practice, I don't think that's as obvious as it should be.
I mean that you don't have to worry about the distinction of which functions are React elements and which are functions. You can just use API features like Let's say you wrote some code like this: import { FixedSizeList } from 'react-window';
import ListRow from './path/to/ListRow';
export default function List({items, ...rest}) {
<FixedSizeList itemCount={items.length} itemData={items} {...rest}>
{ListRow}
</List>
); If someone else came along later and wanted to try hooks in the I think it's confusing when the things you think of as your app's "components" are a mix of React elements and functions. There's something to be said about the consistency of "it's all just React elements".
This is a valid criticism. If memoization is important, and if you need to pass more than just the collection then you would need to add your own lightweight memoization wrapper around item data. Fortunately the new However I'm not sure it's as big of one as you are suggesting, because the two types of memoization we're talking about are different. I am mostly concerned with memoization while scrolling, since that is by far the most performance sensitive part of windowed UI. Since the only thing that is re-rendering during a scroll is the list/grid itself, even if you didn't memoize the incoming The use case you're describing is when the application re-renders at a higher level. In this case, if all you're passing down is the e.g.
This is an interesting suggestion. Spreading is sometimes convenient, but it has some downsides too, such as props naming conflicts. I'm also not sure how we could generically Flow type that API (which may or may not matter to you if you're not using Flow I guess). cc @ryanflorence who might find this discussion interesting. |
I think more power tends to be a burden because, yes, you are taking on more for yourself. Personally I prefer it that way, as it's more flexible, but I understand why others might be confused.
I guess my only reply would be: of course :P If people expect a normal function to act like a component, that seems like a very basic misunderstanding of the API. It's a function, and you return a component. In that component you can do whatever you want. That's almost like people expecting to be able to use hooks in a callback method, or use
Hooks are far from being stabilized, so until then I think it's worth trying to make the API easier to use for others. I tried hard converting this component to hooks actually, but they aren't ready yet. Even with I'm sure as a maintainer you have been met with a lot of confusion because of that flexibility, and I get why you'd want think to be more opinionated out of the box. I'm against a large surface area of APIs, but in this case I think it could be appropriate to provide an additional prop which accepted a normal function. I think it would be easier to devs to go from the basic case which has everything builtin, realize they need to pass a bunch of stuff down, and then convert it to a render function which applies props given from this lib, instead of having to install I just realized it also creates an API inconsistency because I use my I'm just thinking out loud here. It's OK if I don't end up convincing you. I'm used to the other API, and I guess I could create an inline function component that just renders mine. One small wrapper shouldn't effect perf very much.
That's a very good point. Personally I haven't hit a perf problem with the list rerendering since it's so fast because my individual items use
I don't use Flow, and yeah conflicts are a problem. Another options would be to invert it so that you pass the scroll info in as an object instead of individual props, and spread the user's props out so they are normal. Not sure though. |
I understand where you are coming from, though. You are after memoization so that you don't have to do anything when scrolling, and I just want to memoize my Item components so when I change my state they don't all rerender. It's worth noting even if you've memoized it so that scrolling is free, if I use Actually, I just realized that I can see that. I suppose I could have an "intermediate" |
fwiw the main reason I'm looking into |
We might just agree to disagree here. 🙂 I've seen people burned by this in other libraries when a render function isn't a real React component. (This sort of thing also regularly causes pain for e.g. Enzyme users after updates.)
I would advise against this, since recreating an inline function would change its element type which would cause React to unmount and remount it (and blow away any state in your inner component).
I don't think I agree that it's the "same as before". Strict equality checking a few values is going to be less work than rendering an item, even if the difference is negligable.
I would love to hear how |
That's very surprising, since the whole render prop concept is built on the idea that you can just pass a function in and return something. While hooks will eventually replace that, that concept is well-understood from my point of view.
I mean the same as if I was allowed to render my own items, and those items were memoized. It'll rerender the list but do the prop equality checking, and that's it. Just like if this lib memoized them itself. Also, you mention the perf sensitivity of rendering on scroll and the different cases of memoization that we want. But is it really that different? In I might experiment with a wrapper around |
Sorry if it sounds like I'm arguing, I'm commenting without much edits since I don't have much time :) |
A lot of people don't understand the subtle differences between
Check out some of the caching and cache-busting I'm doing in
Sounds great 😄 Keep me posted. Out of curiosity, if I were to spread |
I've seen references to the cache stuff in I'm not entirely sure if spreading will solve it entirely, since I understand now that it's not a per-item data object. I think I'll still need to change my code so generating the per-item state from the data object happen within the item itself. Spreading made sense to me more when I thought I could generate per-item data objects. I was going to say that I can't imagine this being over-optimizing, but I think the difference of understanding is that the |
I think this makes sense. Please keep me posted if this turns out to not work well. I'm going to close this issue (since it's not actionable) but let's keep the conversation going as you try things out! 😄 |
Thanks for taking the time to respond so thoroughly! |
Regarding spreading type RenderComponentProps<T> = {|
data: T,
index: number,
isScrolling?: boolean,
style: Object,
|}; ...to... type RenderComponentProps<T: Object> = {|
...T,
index: number,
isScrolling?: boolean,
style: Object,
|}; cc @TrySound to sanity check this 😄 Maybe it's worth thinking long and hard about the most intuitive API. Is it worth risking possible props naming conflicts (with both current props and, any future ones we might add, since they would become a backwards breaking change) to simplify the memoization story? I listed some pros and cons here: https://gist.github.com/bvaughn/39ee223c7503a834afc1c2e94df27fc5 |
I'm not sure. This forces us to always pass an object to itemData. And still it doesn't solve the problem with callbacks. I would go with Mixing data may be confusing for users. There are props which are may be overridden by I personally use And yeah, types will work but |
To be clear, I was only asking for confirmation that the Flow type was correct 😉 I linked to a Gist above that mentions the pros and cons of actually making the change. My personal feelings are that the cons outweigh the pros, but I wanted to give it fair consideration.
I don't know what this means. |
@bvaughn Oops, I meant "constrained" like |
Oh! Yes that makes total sense. Thank you. |
@TrySound Even with hooks, this is now what it will look like to pass callbacks down to the item: const data = useMemo(
() => ({
onHover,
onEdit,
onSave,
onDelete,
onMove,
onSplit
}),
[onHover, onEdit, onSave, onDelete, onMove, onSplit]
);
<List itemData={data} ... /> I still don't feel like it's a very ergonomic API, compared to: function renderItem({ index, style}) {
return <Item onHover={onHover} onEdit={onEdit} .../>
}
<List renderItem={renderItem} /> |
Agreed that's not very ergonomic, but it's only necessary if you're passing multiple props down (which I still suspect is uncommon, but I don't have any actual usage data) and that layer of memoization actually has a meaningful, positive impact on performance. Given those caveats, I'm not sure the gains outweigh the downsides I mentioned on the Gist. |
Might not be surprising, but I disagree 😀. I think it’s more rare that your rendering static data that doesn’t need to be interacted with. In almost all cases where I’ve rendered a list, I’ve needed to pass callbacks down (on devtools, client projects, and actual). I don’t have data generally from others though - might be useful to ask around? I also think it’s critical that the whole list doesn’t rerender when changing a row’s state. It sounds like we’re talking about two different use cases: you’re talking about rendering static data that doesn’t change, but I’m talking about interactive stuff. The perf of the latter is just as critical as scrolling peformance whenever I’ve use large lists. |
I’m not arguing for spreading the props specifically, but anything that makes it more ergonomic to use with dynamic data. |
That's why I tweeted/reddited this thread and gist 😉
I strongly disagree. Both might be critical, but scrolling is necessarily more critical because it's the same thing at a much higher rate. There are "escape hatches" if it's really important to you though, although they're not very ergonomic. You can always provide your own 😆 We should discuss this over a cup of coffee sometime. |
Yeah, too bad I'm on the east coast!
It didn't ask specifically though about that use case - it's a somewhat nuanced issue so I doubt people read all the way through and would respond whether or not they need to pass additional props down.
From a UX perspective, if either the scrolling is janky or you have an editable list and it's janky when moving around, both are just as serious UX issues from my perspective. Yes, in some ways scrolling is more sensitive, but both are just as important UX. I would be very careful about considering this as over-optimization, as I strongly disagree with that characterization. |
I guess I should rephrase. Both are important UX interactions, but what I meant was that scrolling is far more performance sensitive and so more important to optimize. (I believe users are far less likely to notice if you exceed your frame budget slightly on an update action vs a scrolling interaction.)
You want to champion this? You're passionate about it. I've got my hands kind of full at the moment 😄 Edit because my last sentence may have sounded passive-aggressive. Not my intention! Hope you didn't read it that way. |
In many of my uses of virtualized large lists, I immediately notice when I accidentally break shouldComponentUpdate and all the items start rerendering. Interactions are clearly janky - it's more than just dropping a frame here or there, but there's lag in any of the interactions. It's super important for interactive data. Yeah sure! I'll tweet about it. It's not a huge deal because I'm pretty sure I can wrap this library and expose the API I want, if nothing changes here. But I'm curious myself if my use cases are more special than I think. |
In the past 24 hours, I've chatted with Spencer about React Native's If you come up with a concrete suggested change to I'm not actually using my windowing components very much these days, so it's easy for me to get hung up on theoretical usage that's out of touch. I appreciate the feedback from people who have used them a lot. I think your original suggestion has been to provide a plain function render option rather than a component. If that's still your primary suggestion, then what would your proposed API be like given that I definitely want to continue treating item renderers as React elements by default. |
Thanks for engaging so much, the theoretical usage is just as important because sometimes current practical usage can blind you from a better API. It's been good to hash out the core motivation for our ideas. Sounds good - I will definitely work on wrapping this with a layer that will make it easier for me to switch my app, and that will help me figure out if there are any concrete changes I can suggest. Thanks! |
It seems this ship has sailed (I saw what's in store for version 2) but just in case, I wanted to echo jlongster's sentiment. I know React really well and found Cheers! |
react-virtualized takes a
rowRenderer
prop, andFlatList
in react native takes a similarrenderItem
prop. What is the reason you chose to go with an API where you pass a component as a child instead?Doing so forces you to go through this memoization process if you want to pass props down to the item. I would think this is an extremely common use case, and if you accept a render function instead we can pass down any props we want as normal props, allowing the user to leverage any of React's builtin checks for sCU (
PureComponent
,React.memo
).Would you be open to expanding the API a bit? Instead of passing a component as a child, we could instead provide two different props:
itemComponent
andrenderItem
, the former just passed a component (like now) and the latter passes the data into a function which returns a component.The text was updated successfully, but these errors were encountered: