Changed predictive overscan cell shifting to improve perf

[bvaughn]

How does overscan work now?

Background information: This is how react-virtualized overscanning works. (Edit: These slides have been updated to reflect the new way that overscanning works.)

For simplicity I'll mention rows only. Columns work the same though. It works like this:

1. Overscanned rows are initially rendered above and below your visible rows (eg overscan of 5 means 5 rows above, 5 rows below)
2. When you start scrolling, overscanned rows are stacked in the direction you're scrolling, to help cut down on visible lag (eg overscan of 5 means 0 rows above, 10 rows below)
3. Once you stop scrolling, react-virtualized returns to its default behavior (eg overscan of 5 means 5 rows above, 5 rows below)

How is overscan changing?

This PR changes that behavior so that:

1. Overscanned rows are initially rendered below your visible rows (eg overscan of 5 means 5 rows below)
2. If you scroll down, nothing changes. If you scroll up, overscanned rows are shifted (eg 5 above, 0 below).
3. When you stop scrolling, nothing changes- until/unless you change scroll direction.

Why the change?

React reinstantiates components that have been unmounted and remounted. If components are expensive this leads to a perforamnce hit. RV changing overscan on scroll-stop caused half of the overscanned rows to be reinstantiated due to this (since half of the cells were previously unmounted- even if their elements were cached).

This changeset modifies the shifting behavior. Predictive shifting will remain until/unless scroll direction is reversed, in which case it will be reversed as well. Hopefully this should not have any noticeable impact on UX regarding 'scroll lag' around the edges of the list being scrolled.

It may further help performance as RV was previously doubling down on the number of rows/cols being rendered in the direction being scrolled. (I think this was the wrong initial implementation, but...live and learn.) Now it will render only the exact number specified by the overscan props.

Test the build

I'd love for you to give this change a test before I release it. You can install the beta release:

npm install react-virtualized@8.5.3-beta

[benthehenten]

Hey @bvaughn, I'm working with @arbesfeld on the project where #476 came up.

I just tested out this fix in our app and it works as expected.

Thanks so much for working so quickly to help out here!

[bvaughn]

Thanks for the quick feedback! 😁 I'll leave this PR up a bit longer to hopefully give the Housing Eng team and some others a chance to test it out if they're interested. Stay tuned~

[ritz078]

how about releasing this as a beta ?

[bvaughn]

Great suggestion. Done!

npm install react-virtualized@8.5.3-beta

[ritz078]

its working as expected 👍

[codecov-io]

Current coverage is 98.27% (diff: 100%)

Merging #478 into master will increase coverage by <.01%

@@             master       #478   diff @@
==========================================
  Files            61         61          
  Lines          4336       4338     +2   
  Methods         916        916          
  Messages          0          0          
  Branches        332        331     -1   
==========================================
+ Hits           4261       4263     +2   
  Misses           75         75          
  Partials          0          0          

Powered by Codecov. Last update 007a939...7ff1dae

[jpollard-cs]

is this still in beta / need help testing?

[bvaughn]

I released it a few moments ago as 8.5.3 based on the positive feedback I'd gotten so far (and based on my own testing). But feedback is always welcome!

[jpollard-cs]

👍

[amlynarski]

Thx for library :) , but... with this change (pull) my component stop working. I am writing list carousel with scrolling based on buttons (scrollToColumn) and animating it with margin left on element with class ReactVirtualized__Grid__innerScrollContainer. Is it possible to put some props, which always keep overflow left and right with the same number? I have realized that this change don't work with scrollToColumn (Grid) param - I put number i.e. 2, 4, 6 (scrolling right) but have no overscan on right side...

[bvaughn]

This change should not break any pre-existing components. It only affects overscanning.

If you are using overscanning to display items (sounds like you are) then I think you're misusing it. It isn't intended to work that way. Its only purpose is to prevent there from being flickers of empty space when a user scrolls quickly. (Check out this section of my recent talk about RV.)

Rather than making your carousel the size of a single image and relying on overscanning to show the next/previous images- make your carousel wider, to include the next/previous images, and use scrollToAlignment="center" (or similar) to manage the position of the "focused" image.