do not call setState if combo box is no longer mounted #807
Conversation
|
LGTM |
| @@ -483,6 +489,8 @@ export class EuiComboBox extends Component { | |||
| } | |||
|
|
|||
| componentWillUnmount() { | |||
| this.incrementActiveOptionIndex.cancel; | |||
There was a problem hiding this comment.
I think you want to call cancel actually :-) .cancel()
| @@ -74,6 +74,8 @@ export class EuiComboBox extends Component { | |||
| this.searchInput = undefined; | |||
| this.optionsList = undefined; | |||
| this.options = []; | |||
|
|
|||
| this._hasUnmounted = false; | |||
There was a problem hiding this comment.
Since a component will also be in an unmounted state when initialized, I would rather recommend, having a variable _isMounted, that is false by default, will be set to true, when the component is mounted, and set to false again when the component will unmount, e.g. see also this comment.
But this is more theoretical, since we can be pretty sure, that updateListPosition will never be called before we mounted, so I leave this decision up to you :-)
There was a problem hiding this comment.
I thought componentWillMount was getting deprecated in react 16.3. How would _isMounted get set to true without componentWillMount?
There was a problem hiding this comment.
I think componentDidMount should do the trick here, since there is also nothing you can do in componentWillMount anymore that should triggers the promise load.
There was a problem hiding this comment.
Since this component tracks a ref to its top-level div you could instead check if this. comboBox is null or not, as React sets the ref on mount and re-executes the ref function again with null on unmount. They're all equally as hacky IMO.
fixes #794
I could not find a way to reproduce calling setState once the combo box component had been unmounted. This PR just avoids the potential of calling setState for 2 places where setState was called asynchronously.