Prevent calling setState on an unmounted WindowScroller component #689
Conversation
This a bit of an edge case, when rendering several WindowScroller components on the same page inside a virtual list, the scroll listener can sometimes call setState on an unmounted WindowScroller component (that been scrolled out of view in the virtual list), triggering react's warning. This PR prevents calling setState if the component has been unmounted.
liorbrauer
changed the title
liorbrauer
changed the title
| @@ -127,6 +131,8 @@ export default class WindowScroller extends PureComponent { | |||
|
|
|||
| // Referenced by utils/onScroll | |||
| __handleWindowScrollEvent (event) { | |||
| if (!this._isMounted) return | |||
There was a problem hiding this comment.
@liorbrauer I think ideally we should determine why the mountedInstances array is not accurate, rather than fixing it with a flag
There was a problem hiding this comment.
@mking-clari Good point, though looking at the code and playing around with it I couldn't pin point why it isn't accurate. New WindowScrollers are added to mountedInstances array on mount and are removed on unmount - so that logic is sound. My best guess is that there's a race condition since scroll events are fired rapidly. If that's the case, I'm not sure what the best course of action is, perhaps debounce the scroll event handler?
Alternatively, I can move the check to see if the instance is mounted to the onScroll.js helper, changing line 47 to:
if (instance._isMounted && instance.scrollElement === event.currentTarget) {though that still requires using a flag. Any other thoughts?
There was a problem hiding this comment.
I'm willing to accept this fix, but I'd like to know more about the case that it's fixing so I can determine if there's a better fix. Can you provide a Plnkr (or similar) repro @liorbrauer ?
There was a problem hiding this comment.
@bvaughn here's a plnkr with the problematic setup:
http://plnkr.co/edit/lquCfWiYbNnypf6JrvgW?p=preview
Just open the console and scroll fast in the preview pane (or simply drag the scrollbar fast) to see the setState warnings. Let me know what you think :)
|
Much appreciated! |
|
I just published this feature to NPM as Assuming no problems are reported with the RC, this feature will go out with 9.8.0 sometime this weekend. 😁 |
|
Haven't heard anything negative about the RC so I just released 9.8.0 |
This is a bit of an edge case, when rendering several WindowScroller components on the same page inside a virtual list, the scroll listener can sometimes call setState on an unmounted WindowScroller component (that has been scrolled out of view in the virtual list), triggering react's warning.
This PR prevents calling setState in this scenario if the component has been unmounted.