virtual
virtual copied to clipboard
TanStack
fix: Re-calculate range when options change
Since https://github.com/TanStack/virtual/pull/353, we don't re-calculate the virtualized range in the render function anymore, but instead only when on scroll or size changes. This introduced a regression, where dynamically updating the count or other options did not cause an update to the range, and thus no re-render.
Fix this by explicitly calling this.calculateRange() in this.setOptions(), effectively makes it reactive to any options changes.
Toggling the count option between 0 and 10000, items update accordingly:
https://user-images.githubusercontent.com/15364860/184197568-237593ae-e611-49d9-9812-73507da18e8f.mov
Fixes https://github.com/TanStack/virtual/issues/363
@christiankaindl any idea why the tests are failing? Is it a false positive or should the tests be updated? It'd probably be easier to get it merged if all checks are passing and I am interested in this fix as well 🙏
@zhouzi I quickly looked into it: One test doesn't pass and it doesn't seem to be a false positive. I'm not entirely sure what the issue is yet. Will try to do some more debugging in the next days.
For anyone interested, the failing test is: https://github.com/TanStack/virtual/blob/84663b35d0f71d499ed0d85a0f8f84b4a0391923/packages/react-virtual/tests/index.test.tsx#L113
It fails a few lines below at https://github.com/TanStack/virtual/blob/84663b35d0f71d499ed0d85a0f8f84b4a0391923/packages/react-virtual/tests/index.test.tsx#L128 The test scrolls to 400 pixels and then checks if the correct elements are being rendered. I noticed the range is re-calculated correctly initially, but then there are a few extra renders and the range is wrong after that. Don't know why that is happening yet 🤷♂️
TL;DR I think the failing test was not correct. The code that calculates the range was not touched in this PR so I updated the test to match the expected behavior.
Explanation:
- the container height in the test is 200px
- each item is 100px
- the
estimateSize()function supplied by the "user" estimates 50px for each item.
In the end the range should only include 2 items (2*100px=200px) to fill the container fully, but initially it contains 4 items (4*50px=200px) as react-virtual only knows what estimateSize() gives it. When the 4 items are rendered initially, their real sizes are then measured and the cache is updated with those new sizes (100px each). So during the next re-renders the range changes to only include 2 items.
Note: it actually always over-renders the range by 2. before it rendered 6 and now it renders 4 items
Without this PR: the end range is { startIndex: 5, endIndex: 8 }, which renders 6 items
With this PR: the end range is { startIndex: 4, endIndex: 5 }, which renders 4 items
@tannerlinsley I updated the tests to check for the new range. Does that work for you, or do you want to investigate further?
Now the tests pass but the other check doesn't ^^ but I don't think the check is related to my changes?
Hi, checked the PR and looks like better if we push this logic to adapters, as we can't know how the options are set there.
With this change on react adapter it will try to calculateRange on each render, but what we really want in, for example in react adapter
const afterFirstEffect = React.useRef(false)
useIsomorphicLayoutEffect(() => {
if (afterFirstEffect.current) {
instance.calculateRange()
}
afterFirstEffect.current = true
}, [
options.count,
options.initialRect,
options.overscan,
options.horizontal,
options.paddingStart,
options.paddingEnd,
options.scrollPaddingStart,
options.scrollPaddingEnd,
options.initialOffset,
options.rangeExtractor, // now this we need to wrap
])
We can also write a function that will do a deep comparison and skip functions, but still would include the rangeExtractor in deps. Overall we should expose calculateRange and range as public api of the Virtualizer
btw, till we fix this, one hack for react adapter can be
const virtualizer = useVirtualizer({
count: dataCount,
getScrollElement: () => parentRef.current,
estimateSize: () => 50,
});
const virtualizerRef = useLatestRef(virtualizer)
React.useLayoutEffect(() => {
const v = virtualizerRef.current
v._didMount()()
v._willUpdate()
}, [virtualizerRef, dataCount])
@piecyk where is useLatestRef from, in this case? I'd love to use a workaround for now. I assume it's just this?
export const useLatestRef = <T>(value: T) => {
const ref = useRef(value);
useLayoutEffect(() => {
ref.current = value;
});
return ref;
}
Edit: Confirmed, the workaround works well! Thanks for the hold-over.
@francislavoie yes, basic we don't want to have virtualizer as dep in that effect. It should be well know pattern by now in react community, but if someone don't know it, here is Kent blog post about it https://epicreact.dev/the-latest-ref-pattern-in-react/
@piecyk
Hi, checked the PR and looks like better if we push this logic to adapters, as we can't know how the options are set there. With this change on react adapter it will try to calculateRange on each render, but what we really want in, for example in react adapter
Right, I thought about this as well when I opened this PR. It's a bit more work, but ultimately the more correct/efficient approach. I'll probably be able to make the changes early next week, if the PR is blocked on this.
@christiankaindl on other hand, more robust would be as you did to call the calculateRange when options change, but not for every call of setOptions. Something like https://github.com/TanStack/virtual/compare/beta...piecyk:fix/calculate-range
Here we need to update docs that rangeExtractor and measureElement need to have stale ref as they also affect the range.
@piecyk Yes that would also work and scales better when new framework adapters are added over time. Then the only question is, if having to use useCallback() is acceptable API wise (it was like this in v2.x as well)?
Then the only question is, if having to use useCallback() is acceptable API wise (it was like this in v2.x as well)?
I think it's fine, what do you think @tannerlinsley?
