Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

problem with getEstimatedTotalSize #31

Closed
nihgwu opened this issue Aug 1, 2018 · 11 comments
Closed

problem with getEstimatedTotalSize #31

nihgwu opened this issue Aug 1, 2018 · 11 comments
Labels
💬 question Further information is requested

Comments

@nihgwu
Copy link
Contributor

nihgwu commented Aug 1, 2018

I'm working on a select component with VariableSizeList of react-window and it works perfect, but when I search to narrow down the result (and the menu's height will update), the innerRef's clientHeight is wrong as getEstimatedTotalSize is not works right with dynamic data

const getEstimatedTotalSize = (
  { itemCount }: Props,
  { itemMetadataMap, estimatedItemSize, lastMeasuredIndex }: InstanceProps
) => {
  let totalSizeOfMeasuredItems = 0;

  if (lastMeasuredIndex >= 0) {
    const itemMetadata = itemMetadataMap[lastMeasuredIndex];
    totalSizeOfMeasuredItems = itemMetadata.offset + itemMetadata.size;
  }

  const numUnmeasuredItems = itemCount - lastMeasuredIndex - 1;
  const totalSizeOfUnmeasuredItems = numUnmeasuredItems * estimatedItemSize;

  return totalSizeOfMeasuredItems + totalSizeOfUnmeasuredItems;
};

The problem is that if the itemCount changes from 100 to 1, lastMeasuredIndex is larger then 1, so we can't rely on it anymore

Currently I solved the problem by pass key={itemCount} to List to make a refresh calculation. So perhaps that's not a bug, but I think we should denote that it could be problematic with dynamic data

@bvaughn
Copy link
Owner

bvaughn commented Aug 1, 2018

Can you provide a repro case? Either Code Sandbox or a unit test would be fine.

@nihgwu
Copy link
Contributor Author

nihgwu commented Aug 1, 2018

@bvaughn https://codesandbox.io/s/00mlymwvzp, I added itemKey this time and it still doesn't work correctly

@bvaughn
Copy link
Owner

bvaughn commented Aug 1, 2018

I see. There are a couple of things going on here.

Firstly, VariableSizeList defines a method resetAfterIndex(index: number): void that should be called any time the underlying data changes (since changes to the data indicate that the cached sizes and positions are incorrect).

Your example doesn't call this method when updating the data– but it should. Your handleChange method should look something like this:

  handleChange = e => {
    const inputValue = e.target.value;
    const data = DATA.filter(x => x.label.includes(inputValue));

    // Update state with the filtered list:
    this.setState({ data }, () => {
      // Once state has been updated, let VariableSizeList know to clear its cached measurements:
      this.listRef.current.resetAfterIndex(0);
    });
  };

The way you're reading the size of the innerRef will also be kind of flaky, because you really want to read it after the list has cleared its cached values– but this happens separately from your component's did-update lifecycle. Fortunately, I assume you were just doing that to illustrate the problem and you don't actually need to do it in a real application. 😄

Hopefully, this should be a working solution for you: https://codesandbox.io/s/mzxopv0v5p

@bvaughn bvaughn added the 💬 question Further information is requested label Aug 1, 2018
@nihgwu
Copy link
Contributor Author

nihgwu commented Aug 1, 2018

Thanks for your detailed explanation, I realized I should call resetAfterIndex when I was creating the demo 😅
But what's the best time to read the clientHeight then?
I think we should clear cache before update data, and read the value in cDU, something like that https://codesandbox.io/s/zkjoyxymym

I read the source code for resetAfterIndex now, so there is a forceUpdate be called, but when should I read the value after force updated?

@bvaughn
Copy link
Owner

bvaughn commented Aug 1, 2018

I think we should clear cache before update data, and read the value in cDU, something like that

This wouldn't work, because resetAfterIndex automatically re-renders which would read the old data if you called it earlier.

But what's the best time to read the clientHeight then?

This isn't really something that's intended to be done, but if you really wanted to read it– you'd have to read it after the data was updated (in your component's state) and the list re-calculated dimensions.

@nihgwu
Copy link
Contributor Author

nihgwu commented Aug 1, 2018

So in your post about the gDSFP, reseting the key is a recommended way, does that also fit this case ☺️

@nihgwu
Copy link
Contributor Author

nihgwu commented Aug 1, 2018

BTW, I’m wondering if we could add a second param like shouldForceUpdate to leave the update to the user instead?

@bvaughn
Copy link
Owner

bvaughn commented Aug 1, 2018

So in your post about the gDSFP, reseting the key is a recommended way, does that also fit this case

Yes, I think that would be a pretty reasonable solution in this particular case, since "remeasuring" the items is fast.

BTW, I’m wondering if we could add a second param like shouldForceUpdate to leave the update to the user instead?

Hm. This might work, but it would have some tricky edge cases. For example, if you sorted the data rather than filtering it– then the itemCount wouldn't change, and so none of the props passed to VariableSizeList would change, and it might skip re-rendering if the parent component re-rendered.

@nihgwu
Copy link
Contributor Author

nihgwu commented Aug 1, 2018

But the itemData changed, we should be clear when to use the param, as it defaults to true, so the change won’t beak anything, and for most cases, we would update the outter component to rerender the list, so I think that param would pretty useful

@bvaughn
Copy link
Owner

bvaughn commented Aug 1, 2018

But the itemData changed

This is true for your app (and probably most apps) but I'm not sure if it's necessarily true for every case.

If you want to implement it– including adding tests, and updating docs, (and adding a similar change to VariableSizeGrid component) then I'm happy to consider it! 😁

@bvaughn
Copy link
Owner

bvaughn commented Aug 4, 2018

Resolved via #32

@bvaughn bvaughn closed this as completed Aug 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants