microsoft / reactxp

Library for cross-platform app development.
https://microsoft.github.io/reactxp/
Other
8.29k stars 491 forks source link

VirtualListView - Re-rendering after state change in individual items #1182

Closed alariej closed 4 years ago

alariej commented 4 years ago

I have a VirtualListView showing a list of items sitting in a state variable. If that state changes, for example when adding a new item, the VLV re-renders correctly.

However, if the VLV items themselves are a component with a prop whose value is a state variable, then a state change of that variable doesn't trigger a re-rendering of the VLV.

In order to trigger a re-rendering of the VLV in that case, I must trick the VLV by removing then adding back the last item in the item list.

Is there better way to trigger a re-render of the VLV when changing the state in an individual item?

deregtd commented 4 years ago

Do you need the VLV to re-render because you're using variable-size items, or because you're rendering items from the state of the parent container? If the former, it should be "magically" working through onLayout checks. If the latter, we always encourage your VLV items to render a self-contained component with its own state/props that will re-render itself if its state changes.

alariej commented 4 years ago

Thanks for the quick answer! It's the latter case. I was trying to avoid having the child component react to an own state change, as I thought it would be inefficient, given the number of items in the list. That's why I was trying to pass the parent's (the VLV container) state change as a prop to the child (VLV item) component, and decide (with shouldComponentUpdate) from within the child if it needs to re-render or not.

My setup is similar to what is found in the TodoList sample, where the searchString prop of TodoListItem takes this.state.searchString as value:

    private _renderItem = (details: VirtualListCellRenderDetails<TodoListItemInfo>) => {
        const item = details.item;
        return (
            <TodoListItem
                todo={ item.todo }
                height={ _listItemHeight }
                isSelected={ item.todo.id === this.props.selectedTodoId }
                searchString={ this.state.searchString }
                onPress={ this._onPressTodo }
            />
        );
    };

Also, I am not quite sure what you mean by "variable-size items". Would you have a short example or description for that?

erictraut commented 4 years ago

I think David meant to say "items without a statically-defined height" (i.e. where you pass true for the "measureHeight" prop). This requires a much more expensive rendering path in the VLV. If you're following the TodoList sample, then it sounds like you're providing static height values.

The default behavior of the VLV is to re-render every visible item whenever it receives a new itemList, even if the corresponding item descriptor hasn't changed. This allows the item's render method to utilize external state (like state from the parent).

This behavior changes if you set the skipRenderIfItemUnchanged prop to true when you render the VLV. Are you using that option? It tells the VLV that you promise not to use any state other than that which is contained in the item descriptors, and it allows the VLV to skip re-rendering of items that have not been changed. It's a significant performance win, but it means that you need to take care to avoid using any state that's not specified in the item descriptor.

alariej commented 4 years ago

Oh I see what he meant. Although my implementation is in some respect similar to the TodoList sample, I do use dynamic height values. And I also use skipRenderIfItemUnchanged = { true } in the VLV.

Now according to your description, setting skipRenderIfItemUnchanged to true should still trigger a re-render of the item if a state in the item's descriptor has changed. Or do I understand this correctly?

Here is my code for the VLV item component:

<MessageItem
    id={ this.props.id }
    key={ cellRender.item.event.eventId }
    event={ cellRender.item.event }
    type={ this.props.type }
    readMarkerTime={ this.state.readMarkerTime }
    setReplyMessage={ this.props.setReplyMessage }
    forwardMessage={ this.props.forwardMessage }
/>

Shouldn't the <MessageItem ... /> component re-render if this.state.readMarkerTime (a number) has changed?

Perhaps you could expand on: It tells the VLV that you promise not to use any state other than that which is contained in the item descriptors. Do you mean that there should not be any state variable in the component containing the VLV, other than the one used in the item (this.state.readMarkerTime in my case)?

erictraut commented 4 years ago

Yes, if you set skipRenderIfItemUnchanged to true, it will re-render only those items whose descriptor has changed. It will use the cached (previously rendered) output for all other items.

If you want to use this option, you would need to include your readMarkerTime within the item descriptor.

Or you can set skipRenderIfItemUnchanged to false and keep the rest of your code as is.

alariej commented 4 years ago

I am now testing your suggestion. I have moved the readMarkerTime to the item descriptor, and kept skipRenderIfItemUnchanged={ true }. Here the descriptor:

interface EventListItemInfo extends VirtualListViewItemInfo {
    event: Event;
    readMarkerTime: number;
}

But unfortunately I get no re-render of the corresponding items when readMarkerTime changes value (and the item list state variable is accordingly re-set). The only things which re-render the VLV is either 1) skipRenderIfItemUnchanged={ false } or 2) a hack where I .pop and .concat the last item in the list before setting the state variable containing the item list.

By the way, I am using Resub in the VLV container component (which means the component class extends ComponentBase). Would there be some sort of conflict between the two libraries, which affects the rendering logic?

erictraut commented 4 years ago

Are you passing a new item list, or are you modifying the list that you previously passed to the VLV? If you're modifying the previous list, the VLV won't be able to tell that it changed. You need to send a new item list, since the VLV assumes that the previous list that you specified is immutable. It then compares the items in the previous list with the items in the new list to determine what has changed.

alariej commented 4 years ago

The item list passed to the VLV is in a state variable. I tried deep-copying the item list into a new local array, then modifying the relevant item descriptors in the new array, and finally setting the state variable to the modified array. Still no success in triggering a re-render of the VLV. Only adding new items to the list successfully triggers a re-render. Something in my setup must be disrupting the rendering logic in the VLV.

In any case, thanks for the help. I might keep using my .pop + .concat hack on the item list for now, since it seems to be more efficient than skipRenderIfItemUnchanged={ false }, which triggers far too many rendering of the child items (for example when scrolling the VLV).

fbartho commented 4 years ago

@alariej it looks like there's no further work to be done here on this ticket, but please reply if I've misunderstood! Closing this ticket for now.

alariej commented 4 years ago

I still haven't found a better way to trigger a re-render of the VLV than the .pop + .concat "hack". Not very elegant, but it works fine, so no need to re-open this issue.

GolfredoPerezFernandez commented 4 years ago

facing similar issue