-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add bad dead code elimination detection for React 16 #888
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,6 @@ function installGlobalHook(window: Object) { | |
} | ||
function detectReactBuildType(renderer) { | ||
try { | ||
var toString = Function.prototype.toString; | ||
if (typeof renderer.version === 'string') { | ||
// React DOM Fiber (16+) | ||
if (renderer.bundleType > 0) { | ||
|
@@ -31,29 +30,17 @@ function installGlobalHook(window: Object) { | |
// but might add 2 (PROFILE) in the future. | ||
return 'development'; | ||
} | ||
// The above should cover envification, but we should still make sure | ||
// that the bundle code has been uglified. | ||
var findFiberCode = toString.call(renderer.findFiberByHostInstance); | ||
// Filter out bad results (if that is even possible): | ||
if (findFiberCode.indexOf('function') !== 0) { | ||
// Hope for the best if we're not sure. | ||
return 'production'; | ||
} | ||
|
||
// By now we know that it's envified--but what if it's not minified? | ||
// This can be bad too, as it means DEV code is still there. | ||
|
||
// FIXME: this is fragile! | ||
// We should replace this check with check on a specially passed | ||
// function. This also doesn't detect lack of dead code elimination | ||
// (although this is not a problem since flat bundles). | ||
if (findFiberCode.indexOf('getClosestInstanceFromNode') !== -1) { | ||
return 'unminified'; | ||
} | ||
|
||
// We're good. | ||
// React 16 uses flat bundles. If we report the bundle as production | ||
// version, it means we also minified and envified it ourselves. | ||
return 'production'; | ||
// Note: There is still a risk that the CommonJS entry point has not | ||
// been envified or uglified. In this case the user would have *both* | ||
// development and production bundle, but only the prod one would run. | ||
// This would be really bad. We have a separate check for this because | ||
// it happens *outside* of the renderer injection. See `checkDCE` below. | ||
} | ||
var toString = Function.prototype.toString; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious why we do this (vs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No reason. It used to be used in more places but now it's not. |
||
if (renderer.Mount && renderer.Mount._renderNewRootComponent) { | ||
// React DOM Stack | ||
var renderRootCode = toString.call(renderer.Mount._renderNewRootComponent); | ||
|
@@ -141,14 +128,45 @@ function installGlobalHook(window: Object) { | |
} | ||
return 'production'; | ||
} | ||
|
||
let hasDetectedBadDCE = false; | ||
|
||
const hook = ({ | ||
// Shared between Stack and Fiber: | ||
_renderers: {}, | ||
helpers: {}, | ||
checkDCE: function(fn) { | ||
// This runs for production versions of React. | ||
// Needs to be super safe. | ||
try { | ||
var toString = Function.prototype.toString; | ||
var code = toString.call(fn); | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh, should we put something slightly more descriptive? 😁 Edit Looks like the corresponding change has already been merged to react so nevermind. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// Remember to report during next injection. | ||
hasDetectedBadDCE = true; | ||
// Bonus: throw an exception hoping that it gets picked up by | ||
// a reporting system. Not synchronously so that it doesn't break the | ||
// calling code. | ||
setTimeout(function() { | ||
throw new Error( | ||
'React is running in production mode, but dead code ' + | ||
'elimination has not been applied. Read how to correctly ' + | ||
'configure React for production: ' + | ||
'https://fburl.com/react-perf-use-the-production-build' | ||
); | ||
}); | ||
} | ||
} catch (err) { } | ||
}, | ||
inject: function(renderer) { | ||
var id = Math.random().toString(16).slice(2); | ||
hook._renderers[id] = renderer; | ||
var reactBuildType = detectReactBuildType(renderer); | ||
var reactBuildType = hasDetectedBadDCE ? | ||
'deadcode' : | ||
detectReactBuildType(renderer); | ||
hook.emit('renderer', {id, renderer, reactBuildType}); | ||
return id; | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<script src="shared.js"></script> | ||
<style> | ||
html, body { | ||
font-size: 14px; | ||
min-width: 460px; | ||
min-height: 133px; | ||
} | ||
</style> | ||
<p> | ||
<b>This page includes an extra development build of React. 🚧</b> | ||
</p> | ||
<p> | ||
The React build on this page includes both development and production versions because dead code elimination has not been applied correctly. | ||
<br /> | ||
<br /> | ||
This makes its size larger, and causes React to run slower. | ||
<br /> | ||
<br /> | ||
Make sure to <a href="https://facebook.github.io/react/docs/optimizing-performance.html#use-the-production-build">set up dead code elimination</a> before deployment. | ||
</p> | ||
<hr /> | ||
<p> | ||
Open the developer tools, and the React tab will appear to the right. | ||
</p> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
<style> | ||
html, body { | ||
font-size: 14px; | ||
min-width: 430px; | ||
min-width: 460px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. 👍 |
||
min-height: 101px; | ||
} | ||
</style> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
<style> | ||
html, body { | ||
font-size: 14px; | ||
min-width: 380px; | ||
min-width: 410px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's a different message that's less wide. |
||
min-height: 33px; | ||
} | ||
</style> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be as good at detecting unminified alphas. Which seems okay. It won't report false positives though.