Remove the usage of isMounted()

[oliviertassinari]

Following https://facebook.github.io/react/blog/2015/12/16/ismounted-antipattern.html.
We need to fix those component

• Snackbar [Doc] Convert the Snackbar to the new format #2562
• RefreshIndicator Utilize cancellation over isMounted for indicators #2739
• CircularProgress Utilize cancellation over isMounted for indicators #2739
• LinearProgress Utilize cancellation over isMounted for indicators #2739
• Menu
• IconMenu
• TextField (PR [Core] Remove isMounted() #3437)
• FocusRipple (PR [Core] Remove isMounted() #3437)
• CircleRipple (PR [Core] Remove isMounted() #3437)
• click-awayable.js (mixin removed in [Core] Remove click-awayable mixin #3360)
• Popover (PR [Core] Remove isMounted() #3437)
• Dialog (PR [Core] Remove isMounted() #3437)
• SlideInChild (PR [Core] Remove isMounted() #3437)
• ScaleInChild (PR [Core] Remove isMounted() #3437)

[alitaheri]

👍 👍 👍

[oliviertassinari]

@subjectix I have started doing it on my PR updating the Snackbar.

[alitaheri]

@oliviertassinari very nice 👍

[Illyism]

You seem to be having trouble with this sometimes because you're adding and removing events that don't refer to the same function such as in mixins/click-awayable.js. bind() returns a new function

Something like this would solve this:

this.func = this._checkClickAway.bind(this);
Events.on(document, 'mouseup', this.func);
Events.off(document, 'mouseup', this.func);;

[newoga]

@oliviertassinari @alitaheri Are we following a general strategy for removing this?

I know the anti-pattern blog post mentioned an easy workaround would be to set a property on the class like this._isMounted to true and false during mounting and unmounting. Is that enough? After looking at the code for the remaining components, our use of it seems mostly related to animations and timeouts. I don't know if there's a cleaner way to remove this...

[alitaheri]

@newoga there is. It's cancellation take a look at linear-progress. The better pattern is to have instance fields hold the handles. In other words the lifecycle is like the following:

1. assign the value returned by setTimeout to the instance field ( anywhere in the code that it's used)
2. for each unique concurrently executable timer there should be one field (linear-progress needs 2)
3. with each call to componentWillUnmount clear every timeout.

Simple, efficient and pattern friendly 👍

[newoga]

@alitaheri That makes sense, thanks!

[oliviertassinari]

I have updated the list of components that need to be fixed.
At least half of them is using isMounted() in a timeout, clearTimeout will do the job.
The others, may need a this._isMounted check.

[newoga]

Thanks @oliviertassinari!

I'm hoping to start tackling this after we finish the removal click-awayable.js 😄