Report bad dead code elimination to React DevTools#10702
Conversation
sophiebits
left a comment
There was a problem hiding this comment.
Seems OK to me but I'll let @sebmarkbage review. Would the string ever get hoisted and changed to look up from a string table and therefore not be in the source?
|
It it does get hoisted that would be a false negative (no warning when we should warn) so doesn’t seem very dangerous. Worst case, we just remove this check. |
sebmarkbage
left a comment
There was a problem hiding this comment.
Neat. I would prefer this to check something already in the package so we don't need to add this particular piece of scope.
| // Don't change the message. React DevTools relies on it. Also make sure | ||
| // this message doesn't occur elsewhere in this function, or it will cause | ||
| // a false positive. | ||
| throw new Error('^_^'); |
There was a problem hiding this comment.
Shorter suggestions?
There was a problem hiding this comment.
throw 0;? :)
or
throw null;
There was a problem hiding this comment.
I wanted something that would be hard to accidentally introduce into the function elsewhere. Because then you'd get a false positive. I guess I could search for throw instead?
|
piece of scope? |
|
The problem with putting it inside of the bundle is |
|
|
As a bonus, that would help ensure we don't mess up our UMD builds. :) |
|
Hmm.. Maybe that works! It’s a bit hard for me to think about... |
|
I don’t think that would work. function checkDCE() {
DEVTOOLS.check(checkDCE);
}
checkDCE();and would seem successful. But then the user might not apply DCE in their code, and so they’ll actually ship both Am I wrong? |
|
DevTools PR: facebook/react-devtools#888 |
|
I’ll get this in as is for now but I’m happy to iterate if there are specific proposals for improvements. (The one @sophiebits suggested wouldn’t work because we DCE CJS bundles ourselves to avoid |
Fixes #9589 (again).
We started with #10446, then reverted it in #10673 over concerns in #10640.
This time we don’t rely on
toStringinside ReactDOM itself. Instead we report to React DevTools if they exist, passing the function itself as an argument.React DevTools can check for
^_^there and both produce the “red React” and potentially even do thesetTimeouttrick to report the error to analytics.This doesn’t have the same concerns as explained in #10640 because we’re not doing it for every user but only for React developers who visit React-powered websites which are also built with CommonJS. So it’s a smaller slice. If we suddenly can’t rely on
toStringanymore we can always cut that code from DevTools.