WindowScroller with scrollElement changing from null

[OriR]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

When the scrollElement switches from null to a valid element and then unmounting WindowScroller - it crashes.
See here -> https://codesandbox.io/s/7wp9opl9n0

Click on the unmount window scroller and you'll see the error of cannot read property 'splice' of undefined.

I believe this is because to these lines

react-virtualized/source/WindowScroller/WindowScroller.js

Lines 142 to 154 in 07e9167

	if (
	prevScrollElement !== scrollElement &&
	prevScrollElement != null &&
	scrollElement != null
	) {
	this.updatePosition(scrollElement);
	unregisterScrollListener(this, prevScrollElement);
	registerScrollListener(this, scrollElement);
	this._unregisterResizeListener(prevScrollElement);
	this._registerResizeListener(scrollElement);
	}

If the previous scrollElement was null we're not unsubscribing the even handlers, which is a memory leak but not the error, but we're also not subscribing to the current scrolleElement scroll events.
The error arises when you unmount the component and try to unsubscribe while we haven't even subscribed.

I think it should handle registering of the new scrollElement events separately from unregistering to the previous scrollElement events, that way you always register and deregister when needed.
Something like this:

if (prevScrollElement !== scrollElement) {
  if (prevScrollElement) {
    unregisterScrollListener(this, prevScrollElement);

    this._unregisterResizeListener(prevScrollElement);
  }

  if (scrollElement) {
    this.updatePosition(scrollElement);

    registerScrollListener(this, scrollElement);

    this._registerResizeListener(scrollElement);
  }
}

What is the expected behavior?

Change the scrollElement from null to a valid element without crashing.

I have a workaround that's working right now, doing this:

<WindowScroller scrollElement={this.myElementRef} key={this.myElementRef}>
  ...
</WindowScroller>

I thought about it after reading what @bvaughn wrote here.
key controls whether to re-mount a certain component, if the key is different react will recycle everything and start again, that way I was able to skip the componentDidUpdate for cases when the scrollElement changes.

Which versions of React and react-virtualized, and which browser / OS are affected by this issue? Did this work in previous versions of react-virtualized?

Browser	all of them
OS	all of them
React	16.x.x
React DOM	16.x.x
react-virtualized	9.21.0

[dannycochran]

@bvaughn any updates here or recommended ways to workaround, other than using the ref as a key?

[dogofpavlov]

any more updates on this? Just ran into this bug myself. Although the key does work it feels dirty. I'm not smart enough to come up with a fix.