Skip to content
This repository was archived by the owner on Dec 5, 2019. It is now read-only.

Uglify Does Not Uglify Enough on Latest Webpack + React 16 #206

Closed
iMerica opened this issue Jan 8, 2018 · 38 comments
Closed

Uglify Does Not Uglify Enough on Latest Webpack + React 16 #206

iMerica opened this issue Jan 8, 2018 · 38 comments

Comments

@iMerica
Copy link

iMerica commented Jan 8, 2018

Hi,

Thanks for your work on this library. I recently upgraded to the latest version of webpack and react and I'm now seeing the following in my console (in Production).

screen shot 2018-01-07 at 10 25 19 pm

My webpack config looks kind of like this:

const UglifyJsPlugin = require('uglifyjs-webpack-plugin')

var config = {
  entry: {
    public: APP_DIR + '/public.main.jsx',
    private: APP_DIR + '/private.main.jsx',
  },
  output: {
    path: BUILD_DIR,
    filename: "[name].bundle.js",
    chunkFilename: "[name].bundle.js",
  },
  plugins: [
    new webpack.EnvironmentPlugin(['NODE_ENV']),
    new webpack.DefinePlugin({
      'process.env': {
         NODE_ENV: JSON.stringify("production")
       }
    }),
    new webpack.optimize.CommonsChunkPlugin({
     name: 'init',
      filename: 'init.js',
    }),
    new UglifyJsPlugin({
      uglifyOptions:{
        output: {
          comments: false, // remove comments
        },
        compress: {
          unused: true,
          dead_code: true, // big one--strip code that will never execute
          warnings: false, // good for prod apps so users can't peek behind curtain
          drop_debugger: true,
          conditionals: true,
          evaluate: true,
          drop_console: true, // strips console statements
          sequences: true,
          booleans: true,
        }
      },
    }),
    new webpack.optimize.AggressiveMergingPlugin()
  ],
  resolve: {
    extensions: ['*', '.js', '.jsx'],
    modules: [path.resolve(__dirname, 'react_components'), 'node_modules']
  },
  module: {
    loaders: [
      {
        test: /\.jsx?$/,
        loader: 'babel-loader',
        exclude: /node_modules/,
        query: {
          cacheDirectory: false,
          plugins: [
            require.resolve('babel-plugin-transform-decorators-legacy'),
          ],
          presets: [
            require.resolve('babel-preset-react'),
            require.resolve('babel-preset-env'),
            require.resolve('babel-preset-stage-0')
          ]
        }
      }
    ]
  }
}

module.exports = config;

Thanks,

Michael

@nickells
Copy link

nickells commented Jan 9, 2018

We were dealing with similar issues for a while. What it came down to was the following:

Uglify looks for the presence of if (process.env.NODE_ENV === 'production') blocks in your code and removes those because they always evaluate to false (through the evaluate rule). If React sees that there are any of these left, then the error pops up. You could do a manual search through your minimized code to see if these are appearing. Alternatively, you could wrap a console.log in that if statement to see if it shows up on your live site.

I see you already have evaluate on, so I would consider looking to make sure your process.env variables are being defined correctly. I'm wondering if you need both EnvironmentPlugin and DefinePlugin, as they seem to be doing the same thing and are possibly causing trouble with one another.

Another thing, DefinePlugin recommends you do the following to set variables:
'process.env.NODE_ENV': JSON.stringify("production"),, as it won't overwrite the existing process.env.

Finally, we had a problem with the -p flag in our package.json webpack script because it was overwriting our own Uglify options with a preset, "good-for-production" setting. So make sure you're not using that.

Hope one of these things helps!

Nick

@alexander-akait
Copy link
Member

@iMerica @nickells please create minimum reproducible test repo.

@kzc
Copy link

kzc commented Jan 15, 2018

Uglify looks for the presence of if (process.env.NODE_ENV === 'production') blocks in your code and removes those because they always evaluate to false (through the evaluate rule).

That's not correct. It's up to a webpack plugin to replace process.env.NODE_ENV with "production" before it is passed to uglify. At that point uglify would convert "production" === "production" to true and then retain or drop code accordingly.

@nickells
Copy link

@kzc Thanks for the clarification!

@csvan
Copy link

csvan commented Jan 16, 2018

We were seeing the exact same thing until we set devtool: false for our production builds. This did not simply eliminate sourcemaps - it solved the fact that pretty much nothing was being minified at all (comments were left in, dead code, etc..). I don't fully understand the connection, but would try that.

@kzc
Copy link

kzc commented Jan 18, 2018

Some webpack plugin ordering issue apparently:

webpack/webpack#6328

@kzc
Copy link

kzc commented Jan 18, 2018

To avoid webpack.DefinePlugin altogether, just set the uglify compress option global_defs.

$ uglifyjs -V
uglify-es 3.3.7
$ cat minify.js 
var uglify = require("uglify-es");
var code = "if (process.env.NODE_ENV == 'production') prod(); else dev();";
var options = {
    compress: {
        global_defs: {
            "process.env.NODE_ENV": "production"
        }
    }
};
var result = uglify.minify(code, options);
if (result.error) throw result.error;
console.log(result.code);
$ node minify.js 
prod();

@alexander-akait
Copy link
Member

@kzc very strange all works fine without global_defs in my repos

@kzc
Copy link

kzc commented Jan 18, 2018

Those having issues with webpack.DefinePlugin in conjunction with other plugins can try that alternate global_defs approach.

@richtera
Copy link

It seems to be a size problem. When I initially tried to reproduce this with a smaller project it worked, when I added more code it failed. I put together a test project (it's in the issue referred to earlier webpack/webpack#6328) The project is https://github.com/richtera/webpack-project-for-debugging and will easily show the problem.

@richtera
Copy link

Which version of the UglifyJsPlugin accepts the global_defs option?

@kzc
Copy link

kzc commented Jan 18, 2018

Which version of the UglifyJsPlugin accepts the global_defs option?

[email protected]

@richtera
Copy link

I added a branch to my test project. I am observing errors also happen with global_defs usage (at least using webpack and [email protected]) Curious, I see other people have success with this and it would make a lot of sense that it'd work. It's almost like something is minifying the code before it gets to either DefinePlugin or this UglifyPlugin and it already says something like e.env.NODE_ENV where e=process. This is not something either plugins will understand of course. I had previously tried to make "process" a variable that didn't get mangled and also had no success.

@kzc
Copy link

kzc commented Jan 18, 2018

It's almost like something is minifying the code before it gets to either DefinePlugin or this UglifyPlugin and it already says something like e.env.NODE_ENV where e=process.

The fact that process was mangled to e suggests it's a local variable.

Uglify will not perform the global_defs substitution unless the process variable in process.env.NODE_ENV is global - hence the option name "global_defs"`.

$ cat test.js
function f(process) {
    console.log(process.env.NODE_ENV);
}

console.log(process.env.NODE_ENV);
$ bin/uglifyjs test.js -d process.env.NODE_ENV='"production"' -b
function f(process) {
    console.log(process.env.NODE_ENV);
}

console.log("production");

@richtera
Copy link

Yes, agreed, however the uglified code is:

"production"!==t.env.NODE_ENV&&(r=function(e){if(void 0===e)throw new Error("invariant requires an error message argument")}),e.exports=n}).call(t,n(0))},
function(e,t,n){"use strict";function r(e){if(null===e||void 0===e)throw new TypeError("Object.assign cannot be called with null or undefined");return Object(e)
}

and the original source is:

/**
 * Copyright (c) 2013-present, Facebook, Inc.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 */

'use strict';

/**
 * Use invariant() to assert state which your program assumes to be true.
 *
 * Provide sprintf-style format (only %s is supported) and arguments
 * to provide information about what broke and what you were
 * expecting.
 *
 * The invariant message will be stripped in production, but the invariant
 * will remain to ensure logic does not differ in production.
 */

var validateFormat = function validateFormat(format) {};

if (process.env.NODE_ENV !== 'production') {
  validateFormat = function validateFormat(format) {
    if (format === undefined) {
      throw new Error('invariant requires an error message argument');
    }
  };
}

Something is just not right.

@richtera
Copy link

Could it be because webpackJsonp wraps files into a function causing "globals" to be "locals"?

@kzc
Copy link

kzc commented Jan 18, 2018

If that original source file is run through:

uglifyjs -d process.env.NODE_ENV='"production"' -mc

it produces:

"use strict";var validateFormat=function(t){};

I'm not a webpack expert, but I think there's some configurable option related to global.

Just a thought - is your bundle including node polyfills for process? That would probably explain all this breakage.

@richtera
Copy link

Not on purpose if it does. All I include is babel-polyfill.

@richtera
Copy link

You can see my failing setup in the sample project https://github.com/richtera/webpack-project-for-debugging and will easily show the problem.

@kzc
Copy link

kzc commented Jan 18, 2018

I didn't attempt to build the sample project but just looking at the yarn.lock you can see that webpack pulls in node-libs-browser which in turn pulls in process.

Try having DefinePlugin run first in your webpack config.

@kzc
Copy link

kzc commented Jan 18, 2018

Try having DefinePlugin run first in your webpack config.

Both the Webpack DefinePlugin docs and the React docs probably ought to mention that obscure detail:

https://webpack.js.org/plugins/define-plugin/

https://reactjs.org/docs/optimizing-performance.html#webpack

@kzc
Copy link

kzc commented Jan 18, 2018

@richtera
Copy link

richtera commented Jan 19, 2018

I set node: { process: false } or false and either helped.
The DefinePlugin is the things that's causing the problem in the first place. For some reason it's not substituting all instances of process.env.NODE_ENV. It's just doing the first few and depending on UnglifyJsPlugin being there or not it's doing a different amount of replacements.

I have also tried various sequences of UglifyJsPlugin and DefinePlugin as well as various different versions of UglifyJsPlugin and other Minify plugins. I think it all hinges on a problem with the DefinePlugin.

@kzc
Copy link

kzc commented Jan 19, 2018

If not a node polyfill or a plugin ordering issue, I'm out of ideas as to what is changing process from a global to a local before it reaches uglifyjs-webpack-plugin.

@richtera
Copy link

You are correct though....
In the built file without Uglify it shows process as a local variable being injected.
Using node.process=false doesn't seem to work by itself.

/* WEBPACK VAR INJECTION */(function(process) {/**
 * Copyright (c) 2013-present, Facebook, Inc.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 */



/**
 * Use invariant() to assert state which your program assumes to be true.
 *
 * Provide sprintf-style format (only %s is supported) and arguments
 * to provide information about what broke and what you were
 * expecting.
 *
 * The invariant message will be stripped in production, but the invariant
 * will remain to ensure logic does not differ in production.
 */

var validateFormat = function validateFormat(format) {};

if (process.env.NODE_ENV !== 'production') {
  validateFormat = function validateFormat(format) {
    if (format === undefined) {

Looks like the system will not work if any of the node_modules used are compiled using webpack since they will have an internal definition of process and therefore will destroy the build.
Examples are react-sparklines and react-loading for example. Not using those two in the test project will solve the problem but there might be a lot of javascript modules out there which are using webpack.
How should one use those kinds of libraries?

@kzc
Copy link

kzc commented Jan 19, 2018

It's truly a ridiculous situation. There must be a naïve webpack text replacement plugin or loader out there that does not pay attention to JS scope or grammar.

@kzc
Copy link

kzc commented Jan 19, 2018

On second thought, concentrate on removing the node polyfills. Something must be misconfigured. Once the polyfills are gone, the variable replacements will work.

https://webpack.js.org/configuration/node/
https://webpack.js.org/configuration/externals/

@richtera
Copy link

Maybe I didn't explain this well enough. Both of the modules react-sparklines and react-loading contain the process polyfill in their main file.
Example from react-loading

.
.
.
/******/ ([
/* 0 */
/***/ (function(module, exports) {

// shim for using process in browser
var process = module.exports = {};

// cached from whatever global is present so that test runners that stub it
// don't break things.  But we need to wrap it in a try catch in case it is
// wrapped in strict mode code which doesn't define any globals.  It's inside a
// function because try/catches deoptimize in certain engines.

var cachedSetTimeout;
var cachedClearTimeout;

function defaultSetTimout() {
    throw new Error('setTimeout has not been defined');
}
function defaultClearTimeout () {
    throw new Error('clearTimeout has not been defined');
}
(function () {
    try {
        if (typeof setTimeout === 'function') {
.
.
.

Any file included after that one in the bundle will get process injected instead of keeping the global.
After commenting out the dependencies and usage of those two modules webpack does the correct DefinePlugin substitutions.

@richtera
Copy link

I added a branch in the demo project called worksWithoutSparklinesLoading which will not contain any references to NODE_ENV in the output files. This is just by removing the import of those two modules.

@kzc
Copy link

kzc commented Jan 19, 2018

Both of the modules react-sparklines and react-loading contain the process polyfill in their main file.

I see. Nice detective work.

The react-sparklines and react-loading releases were not built with the webpack@3 option node: false and as such contain a local function process argument that prevents any subsequent user of these modules from getting DefinePlugin to work for process.env.NODE_ENV.

https://github.com/borisyankov/react-sparklines/blob/e3add809067416cffeaa7f119f0c9eec1d3fdfa8/build/index.js#L80-L114

And the process polyfill is embedded within these packages' releases - injected by webpack when their release was made - which cannot be eliminated:

https://github.com/borisyankov/react-sparklines/blob/e3add809067416cffeaa7f119f0c9eec1d3fdfa8/build/index.js#L280-L305

@richtera
Copy link

Turns out of all the modules I am using those two are the only ones without node:false or other ways of building the output file. I posted two pull requested to fix those two modules.

You can install temporarily directly from github.

@kzc
Copy link

kzc commented Jan 20, 2018

@richtera I think the other thread would be interested in your finding: webpack/webpack#6328

@cellis
Copy link

cellis commented Apr 30, 2018

I also had this issue with my production bundles. I was getting the React is running in production mode but dead code elimination has not been applied error. After trying everything, including global_defs, "move uglify to top", compress: { options: { passes: 3|4|5 }}, I decided to use babel-minify-dead-code-elimination as a secondary step. It fixed everything!

Code follows:

...
const babelEnvs = {
    development: {
      plugins: ['react-hot-loader/babel'],
    },
  };

  if (isProd && isClient) {
    babelEnvs.production = {
      plugins: ['minify-dead-code-elimination'],
    };
  }
// more webpack options here...
//...
// still use uglify
plugins: [new UglifyJsPlugin({
      uglifyOptions: {
        ecma: 8,
        compress: {
          warnings: false,
        },
      },
      sourceMap: false, // true,
    })]
// but also use babel dce
{
          test: /\.js$/,
          exclude: /node_modules/,
          use: [{
            loader: 'babel-loader',
            options: {
              babelrc: false,
              presets: ['es2015', 'react', 'stage-2'],
              plugins: [
                ['relay'],
                'universal-import'
              ],
              env: babelEnvs,
            },
          }],
        },
// ...

@chenesan
Copy link

chenesan commented Jul 6, 2018

I found that the configuration from an article of Slack's blog works for me. The only difference is that for uglifyjs-webpack-plugin@1.2.5 the cascade in compress option is not working now, just remove this line and it minifies react lib code as production code.

@amrita-syn
Copy link

amrita-syn commented Aug 23, 2019

We were seeing the exact same thing until we set devtool: false for our production builds. This did not simply eliminate sourcemaps - it solved the fact that pretty much nothing was being minified at all (comments were left in, dead code, etc..). I don't fully understand the connection, but would try that.

Where exactly do I set this 'devtool: false'?? Asking because I have not yet ejected my react app and don't want to....

@csvan
Copy link

csvan commented Aug 23, 2019

@amrita-syn in your webpack config object.

{
    entry: {/* ... */},
   // ...
   devtool: false
}

@amrita-syn
Copy link

@amrita-syn in your webpack config object.

{
    entry: {/* ... */},
   // ...
   devtool: false
}

So, essentially, can't without ejecting?

@csvan
Copy link

csvan commented Aug 26, 2019

@amrita-syn I unfortunately have no experience with create-react-app so I do not know how this would impact your project. You need a way to override your webpack config in either case.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants