Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Add bad dead code elimination detection for React 16 #888

Merged
merged 2 commits into from
Sep 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 40 additions & 22 deletions backend/installGlobalHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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';
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.


// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why we do this (vs Function.prototype.toString.call)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
},
Expand Down
Binary file added shells/webextension/icons/128-deadcode.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added shells/webextension/icons/16-deadcode.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added shells/webextension/icons/32-deadcode.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added shells/webextension/icons/48-deadcode.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions shells/webextension/icons/deadcode.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
24 changes: 24 additions & 0 deletions shells/webextension/popups/deadcode.html
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. &#x1f6a7;</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>
2 changes: 1 addition & 1 deletion shells/webextension/popups/development.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<style>
html, body {
font-size: 14px;
min-width: 430px;
min-width: 460px;
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. 👍

min-height: 101px;
}
</style>
Expand Down
2 changes: 1 addition & 1 deletion shells/webextension/popups/disabled.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<style>
html, body {
font-size: 14px;
min-width: 380px;
min-width: 410px;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
Expand Down
2 changes: 1 addition & 1 deletion shells/webextension/popups/outdated.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<style>
html, body {
font-size: 14px;
min-width: 430px;
min-width: 460px;
min-height: 117px;
}
</style>
Expand Down
2 changes: 1 addition & 1 deletion shells/webextension/popups/production.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<style>
html, body {
font-size: 14px;
min-width: 430px;
min-width: 460px;
min-height: 39px;
}
</style>
Expand Down
2 changes: 1 addition & 1 deletion shells/webextension/popups/unminified.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<style>
html, body {
font-size: 14px;
min-width: 430px;
min-width: 460px;
min-height: 133px;
}
</style>
Expand Down