[Core] Remove isMounted()

[newoga]

Resolves #2573

[alitaheri]

In my opinion, we should go on and remove the check! as it is an anti pattern to hold a reference to an unmounted component. So an explicit error is more appropriate than a silent undefined return, in my opinion.

But let's see what others think too 😅

[newoga]

I don't disagree that it'd be nice to remove the check. I suppose I'm not completely sure why the check is there in the first place, so my gut was to keep it. But I'd be curious to hear if anyone has any idea about that.

as it is an anti pattern to hold a reference to an unmounted component.

In this implementation, according to the React documentation, we won't be holding a reference to the unmounted component because the callback is called with null when the component unmounts:

From the docs:

Note that when the referenced component is unmounted and whenever the ref changes, the old ref will be called with null as an argument. This prevents memory leaks in the case that the instance is stored, as in the first example.

Whether or not we do the check for the events, I think we should still (at some point) replace the string ref with a callback ref since it looks like React will be deprecating the string ref.

[alitaheri]

In this implementation, according to the React documentation, we won't be holding a reference to the unmounted component because the callback is called with null when the component unmounts

I meant the users of TextField. if they call getValue on an unmounted TextField they should get error instead of a silent undefined.

Whether or not we do the check for the events, I think we should still (at some point) replace the string ref with a callback ref since it looks like React will be deprecating the string ref.

True, but even better than that is to have no ref at all 😄

[newoga]

I meant the users of TextField. if they call getValue on an unmounted TextField they should get error instead of a silent undefined.

Oh I understand. I agree. I suppose another reason why they exist now is because of the blur and focus imperative methods.

True, but even better than that is to have no ref at all 😄

I'd love to have no refs. Not sure if we're ready to tackle that yet but I'm all for that too 😄

[alitaheri]

Well I think this is good to go for now 👍 no-ref > callback-ref > string-ref > isMounted this is still 2 step higher that what it is right now 👍 Thanks a lot @newoga 👍

[newoga]

2 step higher

🚀 😆

Thanks for the feedback @alitaheri!

[alitaheri]

Thanks for taking the lead on this 👍

[oliviertassinari]

no-ref > callback-ref > string-ref > isMounted

I completely agree.
As the side effects of removing the check aren't well know. I would say let's focus on removing isMounted 👍

[alitaheri]

As the side effects of removing the check aren't well know.

I was kinda scared of that too 👍 I agree with you

[newoga]

Can I get feedback on this? 😁

Honestly, I'm really confused with how the timeouts are being used in a lot of these cases. 😄 I'm also not quite sure how to reproduce the original issue that caused us to implement the isMounted() checks in the first place. This needs to reviewed carefully and I'd love to figure out how to test this properly.

[alitaheri]

I think we can use cancellation for this.

this.timer = setTimeout(callback, 450);

[newoga]

Woops, I thought I came back to fix that. I must've forgotten or lost it. Thanks!

[newoga]

• Switch removal of isMounted to use cancellation approach
• Removed relevant underscore methods from components

Looking at this again I wasn't always consistent with the names of the timers/timeouts. Maybe I should fix that, maybe it's not critical though.

[alitaheri]

It looks good, in my opinion. No need to be picky right now. 😁 👍 👍

Great job @newoga 😍 Also, cool git history you built there 😎

[newoga]

Great job @newoga 😍 Also, cool git history you built there 😎

Great, thanks @alitaheri! If we want to squash them all down still that's fine too! I was just trying to be extra clear with the commits since I'm unfamiliar these timeouts.

[alitaheri]

No I think we should keep them. the whole point of squash is to have a clean and self documenting history. These sets of commits do exactly that! So we should keep it as is 👍

Also, I don't see any smelly piece of change. It's very unlikely you've broken anything 😁

[newoga]

Awesome, sounds good to me! 😄

[oliviertassinari]

@newoga One more eslint rule enforced 😍.

[newoga]

@newoga One more eslint rule enforced 😍.

I know, #2903 is getting really close! And after this there's only one component left that can't be converted to an ES6 class using the react-codemon so react/prefer-es6-class: 2 is possible soon too.

[alitaheri]

@oliviertassinari I'm all good with how it is right now. feel free to merge after you review 👍 😍

[oliviertassinari]

@newoga Thanks for taking care of this 👍!