Skip to content

Move minification check to ReactDOM #10640

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Move minification check to ReactDOM #10640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 44 additions & 0 deletions packages/react-dom/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,50 @@
'use strict';

function testMinificationUsedDCE() {
if (process.env.NODE_ENV === 'development') {
// We expect this method only to be called in production.
throw new Error('This is unreachable');
}
try {
// use scoped variable for our initial test, in case
// 'top-level' mangling is not enabled.
var source = testMinificationUsedDCE.toString();
var longVariableName = true;
if (longVariableName && source.match(/longVariableName/g).length === 3) {
// We are not minified.
// This might be a Node environment where DCE is not expected anyway.
return;
Copy link
Collaborator

@sebmarkbage sebmarkbage Sep 7, 2017

Choose a reason for hiding this comment

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

Why do we allow this to not be minimized at all?

The other concern was that if shipping works because of this bail out. Then if byte code reversing gets deployed, then it'll skip this and start failing. Without the site doing anything differently.

Copy link
Collaborator Author

@gaearon gaearon Sep 7, 2017

Choose a reason for hiding this comment

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

Maybe I don't understand the first question but in Node environment it is expected that code is not minimized or altered in any way.

Copy link
Collaborator

@sebmarkbage sebmarkbage Sep 7, 2017

Choose a reason for hiding this comment

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

This doesn't hit the server entry point. I guess you could still have a require on this for findDOMNode.

If you run unit tests with react-dom in production, I guess you'd hit this too.

Copy link
Collaborator Author

@gaearon gaearon Sep 7, 2017

Choose a reason for hiding this comment

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

Yes, I'm mostly referring to findDOMNode case.

I don't think we support running tests in production. For example test renderer throws in entry point if you try that. I guess we could relax that if there are valid use cases.

}
if (source.match(/toString/g).length !== 2) {
// We always look for two matches:
// The actual occurence and then the call to 'match'
//
// We know for a fact the above line exists so there should be 2
// matches.
// Therefore the browser gave us invalid source.
return;
}
if (source.match(/unreachable/g).length === 2) {
// We always look for two matches:
// The actual occurence and then the call to 'match'

// Dead code elimination would have stripped that branch
// because it is impossible to reach in production.
setTimeout(function() {
// Ensure it gets reported to production logging
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 (e) {}
}

if (process.env.NODE_ENV === 'production') {
testMinificationUsedDCE();
module.exports = require('./cjs/react-dom.production.min.js');
} else {
module.exports = require('./cjs/react-dom.development.js');
Expand Down
44 changes: 0 additions & 44 deletions packages/react/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,50 +1,6 @@
'use strict';

function testMinificationUsedDCE() {
if (process.env.NODE_ENV === 'development') {
// We expect this method only to be called in production.
throw new Error('This is unreachable');
}
try {
// use scoped variable for our initial test, in case
// 'top-level' mangling is not enabled.
var source = testMinificationUsedDCE.toString();
var longVariableName = true;
if (longVariableName && source.match(/longVariableName/g).length === 3) {
// We are not minified.
// This might be a Node environment where DCE is not expected anyway.
return;
}
if (source.match(/toString/g).length !== 2) {
// We always look for two matches:
// The actual occurence and then the call to 'match'
//
// We know for a fact the above line exists so there should be 2
// matches.
// Therefore the browser gave us invalid source.
return;
}
if (source.match(/unreachable/g).length === 2) {
// We always look for two matches:
// The actual occurence and then the call to 'match'

// Dead code elimination would have stripped that branch
// because it is impossible to reach in production.
setTimeout(function() {
// Ensure it gets reported to production logging
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 (e) {}
}

if (process.env.NODE_ENV === 'production') {
testMinificationUsedDCE();
module.exports = require('./cjs/react.production.min.js');
} else {
module.exports = require('./cjs/react.development.js');
Expand Down