React-virtualized: Do not use `shouldPureComponentUpdate`

[zalmoxisus]

Hey @bvaughn,

I found the lib useful a lot, but it seems to deal only with static data.

Here's an example. Let's say I have and immutable list with some nested data inside and use VirtualScroll as usually:

class SomeContainer extends Component {
  constructor(props) {
    super(props);
    this.getRowHeight = this.getRowHeight.bind(this);
    this.rowRenderer = this.rowRenderer.bind(this);
    this.noRowsRenderer = this.noRowsRenderer.bind(this);
  }

  getRowHeight(index) {
    return this.props.data.get(index).size * 40;
  }

  rowRenderer(index) {
    return <ShowMyData data={this.props.data.get(index)}/>;
  }

  render() {
    return (
        <AutoSizer>
          {({ height, width }) => (
            <VirtualScroll
              height={height}
              width={width}
              rowsCount={this.props.data.size}
              rowHeight={this.getRowHeight}
              rowRenderer={this.rowRenderer}
            />
          )}
        </AutoSizer>
      );
    }
}

Now, when I get this.props.data changed (not size), VirtualScroll will not get updated as shouldComponentUpdate will return false (none of the props were changed), I have to scroll up and down to get this.rowRenderer called.

I suggest to remove shouldComponentUpdate = shouldPureComponentUpdate and let this optimisation for users to be implemented in their own components. I tested only with VirtualScroll, and it helps (except the updating old rowHeight which is the responsibility of Grid component). Maybe there're any drawbacks for other components.

[bvaughn]

Hi @zalmoxisus,

I understand where you're coming from here but I'm reluctant to make this kind of change because of the potential performance impact. You could always wrap react-virtualized with a high-order component (that doesn't use shouldPureComponentUpdate) and call forceUpdate() when your underlying data changes.

Maybe I could even include such a wrapper with the react-virtualized dist?

[zalmoxisus]

Thanks for the prompt answer. If I am not missing anything, it wouldn't help. We can add a ref to VirtualScroll and do forceUpdate there, but we still need to update the Grid component which has the same problem. AFAIK, forceUpdate doesn't force updating the children.

[bvaughn]

Calling forceUpdate() will cause render() to be called on the component, skipping shouldComponentUpdate(). This will trigger the normal lifecycle methods for child components, including the shouldComponentUpdate() method of each child. React will still only update the DOM if the markup changes.

Ah. That does seem like it would be the case since the child Grid's properties would presumably not have changed either. Hmm... I could maybe add a method, similar to recomputeRowHeights, that allowed this kind of deep update... but it seems a little hacky.

This issue seems like it would benefit from some more discussion.

[zalmoxisus]

Actually, forceUpdate in combination with recomputeRowHeights should solve the issue. We need Grid updated only to update the row's height. I'll check it and report back.

[bvaughn]

Ah, if you _need_ to recompute the row-height then you don't need to use forceUpdate at all. Just use recomputeRowHeights. (Since it sets state, that will trigger a render.)

[zalmoxisus]

Oh, recomputeRowHeights updates the Grid. So even if we don't have the height changed, we could use this method to update the content. Thanks for the explanations!

[joewood]

Just for information, React-Native's ListView component supports virtualization. It solved this change notification issue by providing a rowHasChanged function that the component uses for testing when a new data source is provided. This lets you do a row level deep or shallow compare. see here

[bvaughn]

Interesting info @joewood. Thanks for sharing.

I'll have to give that a little thought and see if I couldn't come up with an improved way of checking for changes.

[Surreal9]

I've run into this, where I am implementing my own sorting and not seeing my changes being updated with VirtualScroll. How would I call recomputeRowHeights on my VirtualScroll component, which is being wrapped by InfiniteLoader (and getting it's ref set to registerChild)? Or how else can I force a render?

[Surreal9]

I guess I can just fork this lib to take advantage of the fact I have immutable data and implement shouldComponentUpdate to not use shallowCompare

[bvaughn]

I guess I can just fork this lib to take advantage of the fact I have immutable data and implement shouldComponentUpdate to not use shallowCompare

I assume you mean that you're using _mutable_ data?

Either way, you're obviously free to fork react-virtualized if you would like but I think that would be unnecessary and overkill. You can still use references to children created by child functions (like those of InfiniteLoader). It's pretty simple actually:

<InfiniteLoader {...props}>
  {({ onRowsRendered, registerChild }) => (
    <VirtualScroll
      ref={ref => this._virtualScroll = ref}
      {...props}  
    />
  )}
</InfiniteLoader>

At this point if you need VirtualScroll to recompute its metadata it's just a matter of calling this._virtualScroll.recomputeRowHeights().

[Surreal9]

No I'm using immutable data, so I can simply use an equality check to see if props have changed. Currently you're using shallowCompare, which looks at arrays and says "if they're the same length, the contents probably haven't changed".

In any case, from the sounds of it registerChild isn't necessary then, and I can use my own ref?

[Surreal9]

All I know is this fixed it: https://github.com/Surreal9/react-virtualized/commit/b9df4c02af32674e42197cd50c5afd90aa2a663b

[Surreal9]

I don't mean to sound unappreciative by the way, you've done a great job on this lib and the examples and docs are pretty great as well.

[bvaughn]

No I'm using immutable data, so I can simply use an equality check to see if props have changed. Currently you're using shallowCompare, which looks at arrays and says "if they're the same length, the contents probably haven't changed".

React's shallowCompare add-on replaced @gaearon's react-pure-render mixin. The whole purpose of both of these add-ons is to avoid rendering _unless_ prop instances have changed. This means it _works_ with Immutable data but it _doesn't_ work with mutable data (eg. Arrays).

Taken from the docs:

shallowCompare performs a shallow equality check on the current props and nextProps objects as well as the current state and nextState objects. It does this by iterating on the keys of the objects being compared and returning false when the value of a key in each object are not strictly equal.

shallowCompare returns true if the shallow comparison for props or state fails and therefore the component should update. shallowCompare returns false if the shallow comparison for props and state both pass and therefore the component does not need to update.

I think what's maybe tripping you up is that React Virtualized doesn't ever work with your arrays/collections/whatever _directly_. It just works with a row-count (and/or column-count for Grid). That means if your Array (or List, or whatever) changes then React Virtualized won't know _unless_ the length also changes. That's why I expose the recomputeRowHeights function. I tried to mention this in the docs as well:

Recompute row heights and offsets.

VirtualScroll has no way of knowing when its underlying list data has changed since it only receives a rowHeight property. If the rowHeight is a number it can compare before and after values but if it is a function that comparison is error prone. In the event that a dynamic rowHeight function is in use and the row heights have changed this function should be manually called by the "smart" container parent.

I hope this clears things up a bit for you. FWIW I think you should revert the change you linked to (https://github.com/Surreal9/react-virtualized/commit/b9df4c02af32674e42197cd50c5afd90aa2a663b) as it's likely to cause you a lot of unnecessary renders.

[Surreal9]

Ah, I see, I misread the source for shallowEqual (which is what shallowCompare uses); I'm not too sure why my fix works then and that didn't. In any case, I'm not too sure how I would know when to call recomputeRowHeights. I'm using Redux so the API call is happening in an action, and the state is in a reducer (and none of these have access to the component or its refs). Unless I do something hacky like set a bit in my state for new data and then set it to false after a render..

[Surreal9]

Perhaps in componentWillReceiveProps..

[bvaughn]

It's okay. Even the Facebook docs are worded confusingly on that helper method in my opinion.

Hm. In my apps, all of my sortable lists are based on FlexTable which itself has a sortBy and sortDirection property. (If either of those change then shallowCompare acknowledges the need for a render.)

In your case, if your rows are all a constant height then I think you can skip the recomputing metadata phase and just use forceUpdate to refresh the visible rows. If your rows are dynamic height then you'll need to recompute (since row orders have changed, the previous metadata is likely no longer valid).

Detecting when this needs to be done has to be part of your Redux code though. React Virtualized _can't_ have any way of knowing. That being said, I think you could use a similar approach to FlexTable- and just track your sortBy field/criteria as part of your Redux state. Then when the component containing your VirtualScroll gets an updated sortBy property (in componentWillUpdate perhaps?) it knows to call the appropriate update method.

[Surreal9]

Cool yeah it looks like adding your ref (although I pulled that out to a component function so as to avoid an arrow function in render.. I don't know why FB docs AND Redux docs recommend that which is considered bad practice and gets called out by eslint..) and then adding this works:

  componentWillReceiveProps(newProps) {
    if (newProps.companies !== this.props.companies) {
      this._virtualScroll.forceUpdate();
    }
  }

In that way whether it's new data being loaded in, or sorting or filtering being changed, it all gets picked up. I ended up using a VirtualScroll and implementing my own filtering and sorting to use it with InfiniteScroll. So far so good! 👍

I suppose componentWillUpdate works as well. Thanks so much for your help!

[bvaughn]

Yeah, sorry for writing the shorthand in my example. It's just so much more succinct- it's an easy thing to do. :)

Glad you got it worked out!

[vzaidman]

Just add the desired property for update, for example "companies" to virtual scroller.
<VirtualScroll {...virtualScrollProps} companies={companies} />
And it will force update when the property changes

[sergeyMelentyev]

Works great for me. Thank you @vzaidman