Add bad dead code elimination detection for React 16 #888
Conversation
| @@ -2,7 +2,7 @@ | |||
| <style> | |||
| html, body { | |||
| font-size: 14px; | |||
| min-width: 430px; | |||
| min-width: 460px; | |||
There was a problem hiding this comment.
This is unrelated but Chrome probably changed default fonts in popups because they started wrapping onto the next line a few versions ago. I made all popups 30px wider to make them look nice again.
There was a problem hiding this comment.
Thanks for the explanation. 👍
|
Added the code to throw an error in |
| // (although this is not a problem since flat bundles). | ||
| if (findFiberCode.indexOf('getClosestInstanceFromNode') !== -1) { | ||
| return 'unminified'; | ||
| } |
There was a problem hiding this comment.
This change means DevTools won't quite support 16 alpha/beta/RC? I guess that's okay though.
There was a problem hiding this comment.
It won't be as good at detecting unminified alphas. Which seems okay. It won't report false positives though.
| } | ||
| var toString = Function.prototype.toString; |
There was a problem hiding this comment.
Curious why we do this (vs Function.prototype.toString.call)?
There was a problem hiding this comment.
No reason. It used to be used in more places but now it's not.
| // This is a string embedded in the passed function under DEV-only | ||
| // condition. However the function executes only in PROD. Therefore, | ||
| // if we see it, dead code elimination did not work. | ||
| if (code.indexOf('^_^') > -1) { |
There was a problem hiding this comment.
Heh, should we put something slightly more descriptive? 😁
I guess this pretty unique and tiny which is nice. It's just so arbitrary.
Edit Looks like the corresponding change has already been merged to react so nevermind. :)
There was a problem hiding this comment.
I figured I didn't want it to be long, but also didn't want it to be something that could accidentally appear in that function. Since then it would cause false positive.
| @@ -2,7 +2,7 @@ | |||
| <style> | |||
| html, body { | |||
| font-size: 14px; | |||
| min-width: 430px; | |||
| min-width: 460px; | |||
There was a problem hiding this comment.
Thanks for the explanation. 👍
| @@ -2,7 +2,7 @@ | |||
| <style> | |||
| html, body { | |||
| font-size: 14px; | |||
| min-width: 380px; | |||
| min-width: 410px; | |||
There was a problem hiding this comment.
I assume this is intentionally a different size from the others (given it was already that way) but just pointing it out.
There was a problem hiding this comment.
Yes, it's a different message that's less wide.
Counterpart to facebook/react#10702.
We need this because the hazard of shipping “two Reacts” is a new one due to flat bundles.
Our existing detection strategy doesn’t work here because it checks compiled code. But if you had bad DCE then you have two compiled versions, one of them dormant and never running but still present in your bundle.
So facebook/react#10702 sidesteps this problem by calling DevTools in the CommonJS entry point.