Show warning when componentDidUnmount is defined

[oluckyman]

Fixes #4194

[jimfb]

Looks good to me.

Just musing here: I wonder if we want to warn when the user instantiates the instance, instead of (or in addition to) when they unmount. The reason being that users may not test unmounting behavior while writing the component (though, if they wrote that function, they really should test it and not rely on 'correct-by-careful-inspection'). If we print the error on mount, they will see it sooner and are therefore less likely to miss it. The counterargument is that this displays the error at a more timely location. I could go either way on this one, so I'm mostly commenting in case @spicyj or @zpao feel strongly and want to jump in.

[sebmarkbage]

This is an never ending battle. IMO we shouldn't have accepted #4045 and the like. We decided against PropTypes and mixin are a few more. We can't just keep adding these ad-hoc. What's the proper solution here? A better type system? An automatic spellchecker?

We need to at the very least break this code out of composite component.

[mridgway]

It would be better if this kind of validation was done statically outside of React instead of at runtime. For instance, with Flow.

[oluckyman]

@sebmarkbage #4045 is a typo! Of course React can’t warn about all the typos. But componentDidUnmount is not a typo. This warning covers the missing part in the documentation. It should explicitly said that there is no such a method. Because by default expected that every *Mount method has its counterpart *Unmount.

[jimfb]

@sebmarkbage

This is an never ending battle.

True. But we can catch the ones that meet some minimum threshold of frequency. For instance, 100 or 1000 results in Google. The threshold is arguably debatable, but doing what we can to help engineers avoid sufficiently common mistakes is something we should do.

I actually think I'm in favor of this particular one primarily because otherwise it fails silently (which seems like our fault, since we provide a default implementation instead of throwing if we don't find theirs). For proptypes, the debug path is to open the component and print the props to see how they differ from what the component is expecting. For a lifecycle hook not firing, it's much harder to understand why it isn't firing (short of opening up the React framework source code), and for this reason a warning seems justifiable. It's even worse for the lifecycle hook, because if you're assuming componentDidUnmount exists, you're not going to disabuse yourself of that notion just by looking at your code (at least with a typo, you've got a snowball's chance of noticing the missing character).

Anyway, that's my two cents. I don't feel particularly strongly either way.

[sebmarkbage]

Yea, this is probably the most reasonable one to add. I'm just frustrated that we don't have a more permanent solution for catching typos. We're close to being able to that for propTypes but our current class API doesn't allow for that in a systematic way.

Maybe we can add something like a componentX namespace where everything that starts with "component" is assumed to be a React life-cycle, but what about all those cool extensions that reuse that namespace?