Find callsites that would call toString() if we pass attributes through

[gaearon]

The downside of merging #10385 is that we don’t know if we have internal callsites that would break because of toString once we start passing attributes through. This is not a problem in open source where we’ve had a warning for a long time.

I propose we add this warning now, and let it live for a few days. Then look at the top callsites, and how many there are (and if they are benign or if calling toString could cause issues).

[gaearon]

I didn’t add a test but I verified it works in the Babel standalone example.

Note the intention is to not show this warning but only log it internally.
We'll remove it after we get a sense of whether #10385 is dangerous or not.

[flarnie]

I'm curious whether we want to try and get this merged/synced this week? @spicyj @sebmarkbage
Seems like the only risk is blocking further beta releases until we make a decision and either remove or keep this warning. ?

[gaearon]

Even for open source releases leaving it in isn't a big deal because we already warn about unknown attributes (so people who read warnings don't have them).

[sophiebits]

I guess we don't think arrays are problematic? Or would we not pass them through?

[nhunzaker]

@spicyj This block here should prevent it:
https://github.com/facebook/react/pull/10385/files#diff-320a1076e356380287a6e67f7c894dbfR252

[sebmarkbage]

This isn't finding things that we used to call toString on but no longer do. The built-in toString just goes through valueOf so sometimes only valueOf is overridden to cover both.

E.g. This isn't detected as a custom toString.

var x = {valueOf(){ return 123; }}

[nhunzaker]

@sebmarkbage Ah man. I didn't think of valueOf. Should we do a similar check in #10385 for that as well?

[flarnie]

It sounds like the next steps are:

1. update this to remove the !Array.isArray(value) && value.toString !== Object.prototype.toString checks
2. land it and sync to Facebook
3. get data on how widespread our issues might be with unknown props, and clean up any problems
4. Land the attribute thing without the whitelist but toString all objects, not using a complex heuristic

I think we should consider continuing to work on it for 17.0, since steps 1-3 will require some time. I could be wrong, but I'd rather take the time to test this internally before releasing the change.

[flarnie]

*that said, I'll try to take this over and get it in anyway.

[flarnie]

Already landed this at FB using a work-around to do it quickly, and we are trying to collect the data we need to do the fixes. I'm still not sure we will get this process done in time for 16.0.

[nhunzaker]

@flarnie That sounds good. While I'm excited for this to land, I don't want my enthusiasm to pressure a decision. I'll be ready to add attributes back to the whitelist in #10385 if need be.

[flarnie]

Thanks @nhunzaker - really really really happy to be collaborating with you. :)

[flarnie]

Did the same manual test as @gaearon and it still fires as expected, although as he said, we don't really want to release this, but to add it for our own logging purposes.

[flarnie]

Landing this for now, but we are still working on this:

get data on how widespread our issues might be with unknown props, and clean up any problems

Hopefully will have an update by Monday or Tuesday about how this goes and what we can do next.