Replace didScrollStopSync() call with async one when props are about to change

[httnn]

Description

when controlling the scroll position, there is a significant drop in performance with scrolling after the changes in PR #226. didScrollStopSync() should indeed be called in componentWillUnmount() to prevent setState from getting called after the component is unmounted, but i'm pretty sure that the async version can still be called in componentWillReceiveProps.

when calling the synchronous (non-debounced) version in componentWillUnmount, the onScrollEnd callback is called on every scroll "step". we are doing some pretty heavy re-rendering after that function is called which explains the dramatic drop in performance.

Motivation and Context

we are stuck in 0.8.0 since the performance in 0.8.1 is so much worse.

How Has This Been Tested?

i manually tested unmounting the component while scrolling was in action and there was no warning about calling setState on an unmounted component.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.

[wcjordan]

@pradeepnschrodinger could you look at this since you've been working with the onScroll handlers recently?

Since grid _didScrollStopSync calls setState, I believe this change will open us up to calling setState after the component has been unmounted and break #226.

I'm guessing your issue here is coming because you're updating the props as a result of an scroll stop, perhaps to control scrolling. One alternative approach which wouldn't break things might be to add a check within componentWillRecieveProps which does a shallow equals check of props and avoids running updates if nothings changed. The downside is that might break use cases where a user passes props which aren't immutable, so it might be better implemented in user space, as a HOC, or on the beta branch.

[pradeepnschrodinger]

@wcjordan , yea, I'll take a look into this soon.
@bodyflex, btw, this issue was reported for the beta branch (see #382) and was since fixed, as part of #385. Sadly, the fix wasn't applied to the master branch, probably in fear of this comment.

[httnn]

thanks guys! @pradeepnschrodinger i tried using beta.12 but the performance was as bad as in 0.8.1. i'm not sure if it's exactly the same problem?

[pradeepnschrodinger]

@wcjordan Strangely, I couldn't see the original issue. In fact, I can't figure any way (looking from the code) how an issue can happen. So I'll approve changes made by @bodyflex, as it does improve scrolling, and removes unnecessary user scroll handler triggers.

[httnn]

thanks a lot @pradeepnschrodinger!

i did some more investigation re. the performance with the latest beta: i removed all callbacks passed to <FixedDataTable> and memoized the props.children, so no props should change between renders expect for scrollTop and scrollLeft. still, the performance is really bad.

looking at the profiler, when performance is good (in 0.8.0, and with the change in this PR), it seems like heavy re-rendering is prevented in FixedDataTableCellGroup.shouldComponentUpdate. when performance is bad however, everything is always re-rendered. to me, this seems like a serious regression in the beta and i'm not quite sure what could be causing it.

but that's not related to this PR.

[pradeepnschrodinger]

thanks a lot @pradeepnschrodinger!

i did some more investigation re. the performance with the latest beta: i removed all callbacks passed to <FixedDataTable> and memoized the props.children, so no props should change between renders expect for scrollTop and scrollLeft. still, the performance is really bad.

looking at the profiler, when performance is good (in 0.8.0, and with the change in this PR), it seems like heavy re-rendering is prevented in FixedDataTableCellGroup.shouldComponentUpdate. when performance is bad however, everything is always re-rendered. to me, this seems like a serious regression in the beta and i'm not quite sure what could be causing it.

but that's not related to this PR.

Yep, definitely not related to the issue in this PR, as the didScrollStart and didScrollEnd calls don't exist in componentWillReceiveProps for beta. I too have seen some extra renders for cell group along with mount/unmount calls, but couldn't nail it down on what caused it. Feel free to investigate on this, and open a separate PR/issue if you find something.

Again, thanks for looking into this :)

[httnn]

thanks for the help and quick response-time on this one! we'll have to keep looking into the beta performance issues as well since we really want to keep fixed-data-table-2 up to date in our repo. i'll open a PR if i find something :)

[wcjordan]

Great, this works for me. Thanks for working through this @pradeepnschrodinger. Feel free to merge and cut a release.

[pradeepnschrodinger]

@bodyflex Released with v0.8.20

[httnn]

awesome 🎉 thanks again for working on this so quickly!