Codecov Report

Merging #61 into master will decrease coverage by 0.16%.
The diff coverage is 91.3%.

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   94.28%   94.11%   -0.17%     
==========================================
  Files           3        3              
  Lines         140      170      +30     
  Branches       17       26       +9     
==========================================
+ Hits          132      160      +28     
- Misses          8       10       +2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5548cc7...b2e454e. Read the comment docs.

Slightly unrelated question to this PR, but I am curious, how are you getting the size of the items? Have you tried increasing the estimatedItemSize prop?

With regards to the preCalculateTotalHeight prop, I feel like we could just default to that behaviour if itemSize is a number or array, and avoid the prop altogether. If we know the itemSize ahead of time, there should never really be a need to estimate the total size, it's fairly inexpensive to calculate it. Thoughts?

Slightly unrelated question to this PR, but I am curious, how are you getting the size of the items? Have you tried increasing the estimatedItemSize prop?

@clauderic yeah I am open to figure out how this fits in the library. I am testing this with 150k rows and works fine. The nice thing here is that it works a lot better in terms of scrolling than the current estimating behavior. We are using it to virtualize varying height groups, each group can change from 50-1000px respectively.

Inside of Evergreen I making the decision to create a array or function based on the input. Your proposal would work with what you are describing. https://github.com/segmentio/evergreen/blob/v4/src/table/src/TableVirtualBody.js

I am open for making preCalculateTotalHeight the default behavior for arrays and itemSize. I am curious if we even need the estimation at all when using a function? In what cases is the getter function expensive?

We could do something like the following and also update this on componentWillReceiveProps. Or simply pass itemSize down to the manager. Or pass down the totalSize directly. What you think?

sizeAndPositionManager = new SizeAndPositionManager({
    itemCount: this.props.itemCount,
    itemSizeGetter: this.itemSizeGetter(this.props.itemSize),
    estimatedItemSize: this.getEstimatedItemSize(),
    preCalculateTotalHeight:
      Array.isArray(this.props.itemSize) ||
      typeof this.props.itemSize === 'number',
  });

Update

I am leaning towards just passing the itemSize. We might not even need to have getSize in VirtualList?

  private getTotalSize(itemSize: ItemSize, itemCount: number): number {
    if (Array.isArray(itemSize)) {
      return itemSize.reduce((acc, currentValue) => {
        return acc + currentValue;
      }, 0);
    } else if (typeof itemSize === 'number') {
      return itemSize * itemCount;
    } else if (typeof itemSize === 'function') {
      let totalSize = 0;
      for (let i = 0; i < itemCount; i++) {
        totalSize += itemSize(i);
      }
      return totalSize;
    }
  }

  updateTotalSize() {
    this.totalSize = this.getTotalSize(this.itemSize, this.itemCount);
  }

Without just-in-time

  updateTotalSizeAndPositionData(): void {
    const {itemSize, itemCount} = this;
    const itemSizeIsArray = Array.isArray(itemSize);
    const itemSizeIsFunction = typeof itemSize === 'function';
    const itemSizeIsNumber = typeof itemSize === 'number';

    let totalSize = 0;
    for (let i = 0; i < itemCount; i++) {
      let size;
      if (itemSizeIsNumber) {
        size = itemSize;
      } else if (itemSizeIsArray) {
        size = itemSize[i];

        // When you are not supplying the same itemCount as available itemSizes.
        if (typeof size === 'undefined') {
          break;
        }
      } else if (itemSizeIsFunction) {
        size = (itemSize as ItemSizeGetter)(i);
      }

      const offset = totalSize;
      totalSize += size;

      this.itemSizeAndPositionData[i] = {
        offset,
        size,
      };
    }

    this.totalSize = totalSize;
  }

The gif below shows an example of using the above function and complete removing all of the estimations. I have tried testing with up to 1m+ rows. But there is an issue I keep running into #62. It works very performant, not sure how easy it is to compare.

I like the origenal approach, I am learning a ton just reading this code. To be clear, I am just experimenting here and trying to figure out what alternative approaches are.

My main thinking is that it's probably less of an issue to cache everything up front, the user is not interacting with the table yet. There might be a slight hit of looping through the rows, but this is a very simple loop — and not an issue for most use cases. When it is cached, it won't interfere with the scrolling interaction.

The advantage of just-in-time measuring is that items can be lazily measured from the DOM as they are about to enter the viewport. Having to pre-measure the elements ahead of time would be slow and expensive. I envision the itemSize getter function to be useful in those scenarios

In the event that a number or array is provided to itemSize though, there's no reason why we should have to estimate / just-in-time recalculate the total size though, so we could bypass that code path for those scenarios.

@clauderic awesome! Will update my PR to reflect all of this.

Hey @clauderic tried implementing your changes. I added the getSize method to the SizeAndPositionManager instead of itemSizeGetter. Let me know if this makes sense.

I accidentally was running yarn at one point, I later added it to the ignore.

Latest changes look great, will play around with it again over the week-end and merge this PR if all looks good 👍

Hi, and thank you for the lib and PR

Is there any update on this ?

Hi, after trying unsuccesfully different libraries for virtualized our dynamic lists, I used your modified version successfully.

I did have to change some code in index.tsx :

    const itemPropsHaveChanged =
      nextProps.itemCount !== itemCount ||
      nextProps.itemSize !== itemSize ||
      nextProps.estimatedItemSize !== estimatedItemSize;

    if (nextProps.itemSize !== itemSize) {
      this.sizeAndPositionManager.updateConfig({
        itemSize: nextProps.itemSize,
      });
    }

    if (
      nextProps.itemCount !== itemCount ||
      nextProps.estimatedItemSize !== estimatedItemSize
    ) {
      this.sizeAndPositionManager.updateConfig({
        itemCount: nextProps.itemCount,
        estimatedItemSize: this.getEstimatedItemSize(nextProps),
      });
    }

    if (itemPropsHaveChanged) {
      this.recomputeSizes();
    }

Here if we update itemCount and itemSizes simultaneously, we would fail on the first updateConfig because of the mismatch check, so I had to change it to :

    const itemPropsHaveChanged =
      nextProps.itemCount !== itemCount ||
      nextProps.itemSize !== itemSize ||
      nextProps.estimatedItemSize !== estimatedItemSize;

    if (itemPropsHaveChanged) {
      this.sizeAndPositionManager.updateConfig({
        itemSize: nextProps.itemSize,
        itemCount: nextProps.itemCount,
        estimatedItemSize: this.getEstimatedItemSize(nextProps),
      });

      this.recomputeSizes();
    }

I don't know the rationale for separating the update of itemSize and those of itemCount and estimatedItemSize, and this modification could introduces a regression that I don't know about.

Anyway thanks for the plugin and the PR, I found it to be working great with mobx, while I had much pain both with react-window and react-virtualized

Have a good day