Ensure flat bundles don't duplicate code with weak minifiers

[gaearon]

If the minifier doesn't eliminate constant expressions (like Uglify does), with the way our flat bundles currently work you would ship two bundles.

We should warn prominently in this case, as you would effectively ship a double bundle to production.

[flarnie]

Talking to @trueadm it sounds like this probably won't be a problem. We'll look into it more tomorrow. I'm not sure how to reproduce this issue though.

[gaearon]

I believe this is what happens if you apply Uglify first, and replace process.env.NODE_ENV after that.

[gaearon]

To detect this you might have a function that references process.env.NODE_ENV and then inspects its own source code. If the value is production but source code doesn't contain the "production" word then it wasn't envified correctly.

[flarnie]

Thanks @gaearon! I'll try to reproduce this whenever I get a chance today.

[trueadm]

What if we could add some kind of runtime warning to help users who suffer from this? Not sure if this is the right way.

if (process.env.NODE_ENV !== 'development' && process.env.NODE_ENV !== 'production') {
	// tell the user their minifier is not working?
}

[gaearon]

This is not the worst problematic case (in my opinion). In fact the above case is typical in development environment where NODE_ENV would usually be empty.

The problematic case is when you’ve set it to 'production' (so process.env.NODE_ENV is "production") but have not applied dead code elimination. Or if the bundler doesn’t realize it is statically accessible, and just leaves process.env.NODE_ENV calls that resolve to "production" at runtime. This is exactly the case Slack had a week ago.

To catch it, you could do something like:

function testMinification() {
  if (testMinification.name === 'testMinification') {
    // We are not minified.
    // This might be a Node environment where DCE is not expected anyway.
    return;
  }
  if (process.env.NODE_ENV === 'production') {
    if (process.env.NODE_ENV === 'development') {
      throw new Error('This is unreachable');
    }
    try {
      const source = test.toString();
      if (source.indexOf('toString') === -1) {
        // We know for a fact the above line exists.
        // Therefore the browser gave us invalid source.
        return;
      }
      if (source.indexOf('unreachable') !== -1) {
        // Dead code elimination would have stripped that branch
        // because it is impossible to reach in production.
        setTimeout(() => {
          // Ensure it gets reported to production logging
          throw new Error('React is running in production mode, but dead code elimination has not been applied.');
        });
      }
  }
}
testMinification();

Note that this code would need to be directly in CommonJS entry point (index.js) rather than in bundles so that we don’t run Uglify on it.

[trueadm]

@gaearon Awesome stuff – that was kind of what I was getting to as well (the intention that is).

Would we should try this in a beta and see what the feedback is?

[gaearon]

Sounds reasonable to me, yes.

[flarnie]

I confirmed that our own 'browserify' fixture actually demonstrates this issue - it ships both versions of React in the 'output.js' bundle, since we don't use the browserify 'uglify' plugin or any other thing to do dead code elimination.

I'll use that as a test case initially for adding this warning. Then later we might consider adding a dead-code elimination plugin to the browserify fixture build, just in case people use that as a demo of how to do a simple Browserify set-up with React.

Or we could not, since fixtures are not meant to be used as models for users anyway.

[flarnie]

Nit that I forgot - in the 'browserify' example we are not minifying. As gaearon pointed out earlier, this problem would only really happen if you ran 'uglify' before 'envify' and the dead-code elimination didn't catch the 'prod' conditionals. We probably don't have to worry about changing our browserify fixture since it's not really trying to demonstrate the proper way to do this.

[gaearon]

Reopened since we reverted.