React-virtualized: Grid's `shallowCompare` causes update issues for FlexTable

[EvHaus]

Hey Brian,

I'm having issues with the FlexTable. It never updates the content of my cells when their data changes, resulting in stale content. I've created a stripped down example illustrating the problem here: https://plnkr.co/edit/FXAmSypCcNRvJcoCu3SW?p=preview

I've tracked down the problem.

• FlexTable renders the Grid component
• The Grid component has a shallowCompare for shouldComponentUpdate
• As a result, the Grid never re-renders and never calls the cellRenderers for its cells when the FlexTable is updated

The FlexTable component re-renders correctly, but the inner Grid doesn't.

The issue goes away if I remove the shouldComponentUpdate on the Grid. But obviously, this is not a good solution. Doing a forceUpdate on the FlexTable doesn't help either. The only way I've found to resolve this, is to call this.refs.table.refs.Grid.forceUpdate(); on every state change of my component. This feels awful too.

Can you please advise?

[bvaughn]

Hey @EvNaverniouk,

I recently removed some j.i.t. closures from FlexTable in an effort to speed things up. These were effectively bypassing the shallowCompare check since each time a new function instance was being provided. (It also incurred the overhead of creating the new function instance, although this was much lower.) In other words, the only reason this worked before was a FlexTable specific "bug".

In your example, react-virtualized is doing the correct thing by not re-rendering- since its properties have not updated. This can be fixed by telling it to forcefully update (via recomputeRowHeights). Here's a fixed Plunkr: https://plnkr.co/edit/lZeYcVeCCuoxlI9LDhfh?p=preview

That being said, that particular method is probably an overkill since- in your case- row heights haven't changed. Maybe I'll just add a new method that allows you to just forceUpdate the inner Grid too. Sound reasonable?

[bvaughn]

Version 7.9.0 just went out with a new forceUpdateGrid method that...does...what its name indicates. ;)

Here's an updated version of your Plunkr that works with this new method:
https://plnkr.co/edit/DPhBK1aJEsITXHLkcU5F?p=preview

[bgerm]

Awesome. I just ran into this. I could only come up with passing object.hashCode() to the aria-label prop, because it gets passed down to the Grid.

[EvHaus]

This behaviour feels... wrong to me somehow. It's hard for me to know when I need to call forceUpdateGrid since the component that renders the FlexTable isn't always the same component that manages the changes to its data. Each cell might have it's own components that it's rendering, each requiring different data.

I understand why it's needed for performance reasons, but for a table where data may be often changing in the cells, it feel very clunky to have to call forceUpdateGrid after every setState change the wrapper component may have to handle. It doesn't feel very "React-like".

I would almost prefer having FlexTable always re-render and have the components inside my cells have their own shouldComponentUpdate to control/limit/optimize that rendering.

[bvaughn]

Understandable...but I'm wary to enable the poorer-performing solution by default.

[ZeeStorm]

I feel this is a big enough miss that should be openly expressed in the documentation. To know that custom cell rendering on data change being essentially non-existant and requiring a constant forceUpdateGrid really doesn't work for setups that have data changing rapidly. It's a shame, I really wanted to use this, but I don't think it's a good fit for my cases, at least.

[bvaughn]

@ZeeStorm: It is pretty openly stated in the documentation here on the home page and here on the FlexTable docs page.

I understand your concern about the inconvenience but reversing the default on this would be too big of a performance hit for too many people IMO. It's not something I plan to do considering the primary purpose of this library is purely to improve performance.

That being said, @EvNaverniouk's previous concerns bothered me and so I created a local branch that adds a pure boolean attribute to FlexTable, Grid, and VirtualScroll that defaults to true but can be set to false to disable the shallow comparison. I need to test it more before I'd be ready to release it though.

[ZeeStorm]

@bvaughn: This is true to what @EvNaverniouk posted, where the data inside the cell changes, and not causing an update. However, my actual row data is changing, and not causing an update. For example, I have a record set with a column named "dataType". I have an external function that calls a connected action to update this record (via a reducer using FB's immutable update). Inside of the component that encases the <FlexTable>, the shallowCompare is seeing the change, and says an update needs to happen (dataTypeCell changed from 'ORDINAL' to 'CATEGORICAL'). From here, diving into custom cellRenderer, the properties are not seeing this change. It's not detecting the new property object for rowData. Here's a sample of the FlexColumn:

<FlexColumn
  cellRenderer={
    ({ rowData }) => (
      <ReviewGridDataTypeCell rowData={ rowData } />
    )
  }
  className="review-grid-cell-data-type"
  dataKey="dataType"
  disableSort
  width={ 135 }
/>

And the row getter is simple as:

const rowGetter = ({ index }) => filteredReviewData[ index ];

The filteredReviewData is constructed via reselect and the updated "dataType" is (of course) showing up in the higher level components when testing to ensure the field is changing.
Is this update being blocked by the same reason? I feel that this is a props change and that the cellRenderer should be seeing the new props

[bvaughn]

Both FlexTable and VirtualScroll are built on top of Grid. They're both HOCs that decorate a Grid. In the case of VirtualScroll it does little more than set some default properties on the "decorated" Grid but in the case of FlexTable - it's rendering some surrounding UI (eg table headers).

Sounds like you're describing a scenario where FlexTable properties change but the underlying Grid properties do not. That's understandably a bit confusing. Hm.

Maybe the solution here is to pass through FlexTable properties to Grid (even though it's not using them) so that changes in the outer component's properties will trigger a re-rendering of the inner component. I think I'd be willing to do that since I don't expect it would harm performance too much.

[ZeeStorm]

I guess that is where the drop off of my understanding of where the props change detection happens. I appreciate the explanation.

[bvaughn]

No problem. It's not very intuitive I guess, particularly if you don't know the internals of the library. I want to make react-virtualized as user-friendly as possible unless it comes at a big performance cost. In this case, I think we can compromise. Let me think on it some more and maybe this evening after work I can put out an update with this.

[bvaughn]

So I just released 7.11.2. The only change in this version was that I pass-thru all of FlexTable props to the inner Grid. This means that if a prop changes and triggers FlexTable to re-render, it will always also re-render the inner Grid. This doesn't seem to have harmed performance any (from my initial testing) and does hopefully make the component more intuitive and user-friendly to work with.

[ZeeStorm]

Thank you, I really appreciate it!

[bvaughn]

No problem. You guys made a good case.

I still intend on adding a pure property to let you opt out of shallow comparison entirely if you want, but i need to work through an infinite loop in FlexTable.

[EvHaus]

Thank you. 7.11.2 is working great for me. Removed all my forceGridUpdates :)

[bvaughn]

Excellent! I'm glad to hear that. I know they're awkward. :)