[bug] [16.5.0] shouldConstruct check

[brkalow]

Do you want to request a feature or report a bug?
Reporting a bug.

What is the current behavior?
With the recent change to shouldConstruct (5031ebf#diff-9ba7cab7f838d9f9071d2fb8ccbf48bfL282) the logic for checking the isReactComponent prototype property has become more strict. It turns out we are using a few dependencies that are defining a method called isReactComponent and this update caused these components to be treated as an IndeterminateComponent type internally.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:
https://codesandbox.io/s/v3qw39wky

What is the expected behavior?
It would be nice if React could recognize this case and maybe log an error in development. I'm happy to try implementing this if you want to go that route. People definitely shouldn't be defining isReactComponent on their own components, but if it happens a quick check and an error message would make it a quick fix.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
Impacts only v16.5.0. It wasn't an issue due to the less exhaustive check in earlier versions.

[aweary]

We could use the same object value or a Symbol for every component and then use an equality check instead of typeof in shouldConstruct.

Then we could warn if it exists, but isn't equal to this internal value. Or use a more obscure name like __isReactComponent___

[gaearon]

Can we just revert that particular change? I doubt breaking it was intentional. More likely an oversight.

[gaearon]

@aweary Checking specifically for prototype.isReactComponent is part of today’s contract and has been for years. We can’t easily change this now. I also think changing it wouldn’t be worth the churn since other libraries adopted the same check.

[aweary]

Using a shared object and equality check wouldn't break anyone though, right? I thought we used to do that somewhere else too.

[gaearon]

What does this solve? These components would still be broken because they wouldn’t pass the check. We could warn when you shadow it. But also seems not worth it.

[aweary]

A proper warning is what I had in mind. If it's an established contract it seems like we should discourage users from shadowing it

[gaearon]

Should be fixed in 16.5.1