Fix List with CellMeasurer on first paint

[OriR]

fix #866

The Problem

Going back to the original issue - the problem was that List with CellMeasurer rendered the children incorrectly but after the first scroll - everything went back to normal.

After the initial findings coming from @steinso, which turned out to be true I wanted to dig a bit deeper into why the Grid ref in List wasn't initialized when calling invalidateCellSizeAfterRender on the List but only on the first render (after mount).
I realized that the List._setRef is being called after List.invalidateCellSizeAfterRender which is called (indirectly) in CellMeasurer.componentDidMount.

So I looked at reacts source code and figured out that this is by design, component lifecycle hooks are being triggered before attaching refs.

(notice commitLifeCycles react-dom.development.js:11505)

(notice commitAttachRef react-dom.development.js:11579)

Plus, CellMeasurer is returned through rowRenderer which in turn, is returned to cellRenderer and is then rendered by Grid, so the component hierarchy is something like this:

List
|___ Grid
     |___CellMeasurer
     |___CellMeasurer
     |___CellMeasurer
     ...

This means that Grid.componentDidMount will only be called when all of componentDidMount of all of his children have been executed, and since attaching refs only happens after life cycle hooks, it means that react will attach the Grid ref to the List only after all the CellMeasurer children have been mounted.
To conclude, this is the call order (of the calls we care about):

1. Grid.props.cellRenderer (for each cell)
2. List.props.rowRenderer (for each cell)
3. CellMeasurer.componentDidMount (for each CellMeasurer)
4. List.invalidateCellSizeAfterRender (for each CellMeasurer)
5. Grid.ComponentDidMount
6. List._setRef (for getting the Grids ref)

The solutions

I thought of several solutions to solve this but couldn't decide which is better so I did all of them, each in its own commit.

Simple solution

This one is the easiest to understand, since the first call in the call chain is Grid.props.cellRenderer which calls List._cellRenderer, we can use that.
Grid invokes to cellRenderer with a parent parameter, we can use that in List._cellRenderer to do this simple check:

this.Grid = this.Grid || parent;

This way we're setting the "ref" for the Grid a lot sooner, thus ensuring Grid.invalidateCellSizeAfterRender will be called, even during CellMeasurer.componentDidMount.

Deferred solution

Instead of trying to set the ref every time on List._cellRenderer, we can do everything regularly, except that if we find that this.Grid is not initialized we add a desired callback (basically calling Grid.invalidateCellSizeAfterRender) to a deferred callback list that will be invoked once we actually set this.Grid.
The only problem is that now the Grid.componentDidMount is being called before all of our deferred callbacks (they're invoked in List._setRef) which means it'll miss the call to Grid._handleInvalidatedGridSize in Grid.componentDidMount, so we also add a call to List.forceUpdateGrid in List._setRef, that will cause a render but also invoke Grid._handleInvalidatedGridSize in Grid.componentDidUpdate.

Premount solution

This solution tries another approach, setting the Grids ref really early, even before the life cycle hooks. It does that by adding a new prop to Grid that's called preMountRef and it's invoked in Grid.componentWillMount with this as the Grid ref. That way the List will have the ref before the first CellMeasurer is being rendered - which is what we want.
To me it feels like a dirty trick, but maybe that's just me 🤷‍♂️

Testing each solution

I couldn't easily reproduce this issue with the local examples, so what I did was to edit source/demo/Application.js and replace everything there with:

import React, {PureComponent} from 'react';
import CellMeasurer, {CellMeasurerCache} from '../CellMeasurer';
import List from '../List';
const numberList = Array.from(new Array(500)).map((_, i) => i + '');

export default class DynamicHeightList extends PureComponent {
  constructor() {
    super();
    this.cellMeasurerCache = new CellMeasurerCache({
      fixedWidth: true,
    });
  }

  renderItem({index, key, style, parent}) {
    return (
      <CellMeasurer
        cache={this.cellMeasurerCache}
        key={key}
        parent={parent}
        rowIndex={index}
        columnIndex={0}>
        <div style={style} key={key}>
          {numberList[index]}
        </div>
      </CellMeasurer>
    );
  }
  render() {
    return (
      <List
        rowRenderer={this.renderItem.bind(this)}
        width={200}
        height={300}
        deferredMeasurementCache={this.cellMeasurerCache}
        rowHeight={this.cellMeasurerCache.rowHeight}
        rowCount={numberList.length}
        overscanRowCount={10}
      />
    );
  }
}

(This is the code from the plunker in the original issue)

Update: I managed to figure out why I couldn't reproduce the issue with the original CellMeasurer examples. That's because the examples use AutoSizer which transfer the width to each example, and on AutoSizer.componentDidMount it calls setState with the new dimensions, this triggers another render cycle so now all the refs exist so everything appears to be working.
So a side note here - I'd make each example page as isolated as possible to avoid example specific quirks like these.

I'll update the tests/examples/docs once I'll know which solution to use 😉

P.S.
I did nvm use once I saw there's .nvmrc, but yarn install resulted in a segmentation fault when running npm run lint 😳
So I deleted node_modules and ran nvm use 8 and later everything was fine.
I don't know if it's my machine or not but it's worth upgrading .nvmrc to 8.x.x.

[bvaughn]

This is a fantastic PR write-up. Thank you so much for taking the time to put all of these details together! 🙇

I think there's a simpler solution than the one you've proposed with this PR: Just pass Grid as the parent to CellMeasurer (since it is anyway, really):

diff --git a/source/List/List.js b/source/List/List.js
index 0822666..c2dbdfe 100644
--- a/source/List/List.js
+++ b/source/List/List.js
@@ -209,6 +209,7 @@ export default class List extends React.PureComponent<Props> {
   }
 
   _cellRenderer = ({
+    parent,
     rowIndex,
     style,
     isScrolling,
@@ -235,7 +236,7 @@ export default class List extends React.PureComponent<Props> {
       isScrolling,
       isVisible,
       key,
-      parent: this,
+      parent,
     });
   };

What do you think?

This is actually what Table already does. I think I was just being unnecessarily clever by masking that value for List.

[bvaughn]

Based on my own local testing (using the steps you outlined above) this solution fixes the problem. Please let me know if I've misunderstood or overlooked something!

And thank you so much, again!

[OriR]

Sure thing @bvaughn !
I really enjoyed diving this deep 🎉
Putting it all in writing really helped me make sure I wasn't missing anything 👍

I actually have thought about this option but assumed it was the least desirable 🙄
This actually makes the CellMeasurer bypass List.invalidateCellSizeAfterRender and directly call Grid.invalidateCellSizeAfterRender.
By itself it looks harmless, but I think it breaks the decorator pattern the List uses on top of Grid.

If and when there's gonna be a List specific logic in invalidateCellSizeAfterRender it's gonna be hard to trace back the bug to the parent-setting-line. While this is sort of a case of YAGNI, I think the other solutions can prevent these kinds of bugs while still not being too verbose about it.

Plus, I just realized that there are other components that decorate Grid the same way and this bug fix should handle them as well and not just List.
Actually MultiGrid implements the same logic found in Grid which is also part of the problem.
I think we can determine by the column & row index which grid(s) to invalidate and simply call invalidateCellSizeAfterRender them instead of duplicating the logic. This is a prime example of decorator-specific-logic and why bypassing the decorator could potentially result in bugs (or code duplication 😄 ).

Would love to hear your thoughts on this since I'm the outsider here and might not know all the inner workings of all the components.

[bvaughn]

I appreciate your sharing these follow-up thoughts with me. 😄

Here's my thinking: List#invalidateCellSizeAfterRender (and other, similar methods) are just forwarding methods to the decorated Grid. They don't have any custom logic. If they ever did, then you're right- this would bypass them. In this case, I know that is unlikely though. In fact, these forwarding methods mostly exist for this very case (CellMeasurer).

Plus, I just realized that there are other components that decorate Grid the same way and this bug fix should handle them as well and not just List.

There are two others:

1. Table never overrode the parent property in the first place and so avoided this bug.
2. MultiGrid is a different case though since it needs to forward 1 request to multiple children. It probably still has this bug. An approach like the one you mentioned is probably required to fix it in that case since it's more complex.

[bvaughn]

PS. Interested in contributing a PR to fix MultiGrid?

[OriR]

In fact, these forwarding methods mostly exist for this very case (CellMeasurer).

Makes sense 👍 I just wasn't sure about it and so I tried thinking of a broader solution.

MultiGrid is a different case though since it needs to forward 1 request to multiple children. It probably still has this bug. An approach like the one you mentioned is probably required to fix it in that case since it's more complex.

I think MultiGrid doesn't actually have this bug. It has the same logic found in Grid

invalidateCellSizeAfterRender({columnIndex = 0, rowIndex = 0} = {}) {
    this._deferredInvalidateColumnIndex =
      typeof this._deferredInvalidateColumnIndex === 'number'
        ? Math.min(this._deferredInvalidateColumnIndex, columnIndex)
        : columnIndex;
    this._deferredInvalidateRowIndex =
      typeof this._deferredInvalidateRowIndex === 'number'
        ? Math.min(this._deferredInvalidateRowIndex, rowIndex)
        : rowIndex;
  }

And later in componentDidMount & componentDidUpdate, there's a call to _MultiGrid._handleInvalidatedGridSize which calls recomputeGridSize on the right grids using _deferredInvalidateColumnIndex & _deferredInvalidateRowIndex, just like you said the fix should be.

Or am I missing something?

[bvaughn]

Ah, good observation. I haven't looked at MultiGrid in a while and I misremembered how this was implemented. 😄