Uglifyjs: Comments associated with discarded code are dropped

[alexander-akait]

Bug report or feature request?

Bug report. When i use output.comments as function (function (astNode, comment)), JSDoc-style comments that contain "@preserve", "@license" or "@cc_on" (conditional compilation for IE) don't passed in function.

ES5 or ES6+ input?

any

Uglify version (uglifyjs -V)

Latest

JavaScript input

/** @preserve comment should be extracted extract-test.1 */

The uglifyjs CLI command executed or minify() options used.

output: {
    comments: false
}

JavaScript output or error produced.

Empty file, all ok.

[kzc]

Please provide a runnable program that will reproduce your issue using the minify() API.

There are some known issues with comments: #88. Trailing comments are not preserved, nor are comments between certain keywords.

$ bin/uglifyjs -V
uglify-es 3.1.10

$ echo '/*@1*/ /*%2*/ if /*@3*/ (x) /*@4*/ foo(); /*@5*/' | bin/uglifyjs --comments all
/*@1*/ /*%2*/if(x)/*@4*/foo();

$ echo '/*@1*/ /*%2*/ if /*@3*/ (x) /*@4*/ foo(); /*@5*/' | bin/uglifyjs --comments /@/
/*@1*/if(x)/*@4*/foo();

$ echo '/*@1*/ /*%2*/ if /*@3*/ (x) /*@4*/ foo(); /*@5*/' | bin/uglifyjs --comments /%/
/*%2*/if(x)foo();

Since comments are anchored to AST nodes, if they are transformed or dropped as unused then their associated comments are also dropped:

$ echo '/*@1*/ /*%2*/ if /*@3*/ (0) /*@4*/ foo(); /*@5*/' | bin/uglifyjs -c --comments all
/*@1*/ /*%2*/

Not much can be done about that unless all comments are disassociated from code and put at the start or end of the uglified output.

[alexander-akait]

@kzc hm, maybe related.
Code:

"use strict";

const UglifyJS = require("uglify-es");

const code = {
  "file1.js": `/******/ (function(modules) { // webpackBootstrap
/******/        // The module cache
/******/        var installedModules = {};
/******/
/******/        // The require function
/******/        function __webpack_require__(moduleId) {
/******/
/******/                // Check if module is in cache
/******/                if(installedModules[moduleId]) {
/******/                        return installedModules[moduleId].exports;
/******/                }
/******/                // Create a new module (and put it into the cache)
/******/                var module = installedModules[moduleId] = {
/******/                        i: moduleId,
/******/                        l: false,
/******/                        exports: {}
/******/                };
/******/
/******/                // Execute the module function
/******/                modules[moduleId].call(module.exports, module, module.exports, __webpack_require__);
/******/
/******/                // Flag the module as loaded
/******/                module.l = true;
/******/
/******/                // Return the exports of the module
/******/                return module.exports;
/******/        }
/******/
/******/
/******/        // expose the modules object (__webpack_modules__)
/******/        __webpack_require__.m = modules;
/******/
/******/        // expose the module cache
/******/        __webpack_require__.c = installedModules;
/******/
/******/        // define getter function for harmony exports
/******/        __webpack_require__.d = function(exports, name, getter) {
/******/                if(!__webpack_require__.o(exports, name)) {
/******/                        Object.defineProperty(exports, name, {
/******/                                configurable: false,
/******/                                enumerable: true,
/******/                                get: getter
/******/                        });
/******/                }
/******/        };
/******/
/******/        // getDefaultExport function for compatibility with non-harmony modules
/******/        __webpack_require__.n = function(module) {
/******/                var getter = module && module.__esModule ?
/******/                        function getDefault() { return module['default']; } :
/******/                        function getModuleExports() { return module; };
/******/                __webpack_require__.d(getter, 'a', getter);
/******/                return getter;
/******/        };
/******/
/******/        // Object.prototype.hasOwnProperty.call
/******/        __webpack_require__.o = function(object, property) { return Object.prototype.hasOwnProperty.call(object, property); };
/******/
/******/        // __webpack_public_path__
/******/        __webpack_require__.p = "";
/******/
/******/        // Load entry module and return exports
/******/        return __webpack_require__(__webpack_require__.s = 8);
/******/ })
/************************************************************************/
/******/ ({

/***/ 8:
/*!**************************!*\\
  !*** multi ./extract.js ***!
  \\**************************/
/*! no static exports found */
/*! all exports used */
/***/ (function(module, exports, __webpack_require__) {

module.exports = __webpack_require__(/*! ./extract.js */9);


/***/ }),

/***/ 9:
/*!********************!*\\
  !*** ./extract.js ***!
  \\********************/
/*! no static exports found */
/*! all exports used */
/***/ (function(module, exports) {

/** @preserve comment should be extracted extract-test.1 */

var foo = {};

// comment should be stripped extract-test.2

/*!
 * comment should be extracted extract-test.3
 */

/**
 * comment should be stripped extract-test.4
 */

module.exports = foo;


/***/ })

/******/ });
`,
};
const options = {
  output: {
    comments: (astNode, comment) => {
      // No comment `/** @preserve comment should be extracted extract-test.1 */` here
      console.log(comment);

      return false;
    }
  }
};
const result = UglifyJS.minify(code, options);

console.log(result.code);

[kzc]

$ cat comments.js 
var uglify = require("uglify-es");
var source = "/*@1*/ /*#2*/ if (Math) /*@3*/ /*%4*/ foo(); /*#5*/ /*@6*/ bar();";
var result = uglify.minify(source, {
    output: {
        //beautify: true,
        comments: function(node, comment) {
            console.log("// comment: type = %s, value = %s", comment.type, comment.value);
            return /@/.test(comment.value);
        }
    }
});
if (result.error) throw result.error;
console.log(result.code);

$ node comments.js
// comment: type = comment2, value = @1
// comment: type = comment2, value = #2
// comment: type = comment2, value = @3
// comment: type = comment2, value = %4
// comment: type = comment2, value = #5
// comment: type = comment2, value = @6
/*@1*/Math&&/*@3*/foo(),/*@6*/bar();

[alexander-akait]

@kzc in my example we don't have comment :disappointed:

[kzc]

/** @preserve comment should be extracted extract-test.1 */

var foo = {};

As previously mentioned, comments associated with unused or optimized away code will be dropped:

$ echo 'function f(){ /*@1*/var unused; /*@2*/bar(); }' | node_modules/.bin/uglifyjs -c --comments /@/
function f(){/*@2*/bar()}

Had the minify input been the following it would make its way to the comments callback:

/** @preserve bar comment */

bar();

Perhaps the unused code removal functionality in the uglify compressor could be extended to optionally collect the comments from discarded AST and emit them at the end of the uglified output if the comments output option is set in minify(). The comments callback could be invoked with a null and/or fake node first argument. Comment ordering would be a mess, but you probably only care about license comments anyway. PR welcome.

[kzc]

Since this issue is not related to JSDoc-style comments that contain "@preserve", "@license" or "@cc_on", could this issue's title be changed to "Comments associated with discarded code are dropped". Thanks.

[alexlamsl]

@kzc is there a duplicate issue? I seem to recall reading one before but my quick search wasn't very fruitful.

[kzc]

Not quite a duplicate. There's a related issue #88 for comments dropped at end of function body.

[alexlamsl]

Actually, there's #2222

[kzc]

Good point, but this one has more detail (and is still open).

[alexander-akait]

@alexlamsl @kzc how can i help you to resolve this issue asap?

[kzc]

This is not a trivial issue and is at odds with the design assumptions made by uglify.

Some background:

Uglify output as written only operates on ASTs and the comments associated with them. Comments are attached to the first AST node that follows them. Some comments between keywords and at the end of functions/programs are never recorded:

$ echo '/*1*/ if /*2*/ (0) /*3*/ foo(); /*4*/ else /*5*/ bar(); /*6*/' | bin/uglifyjs --comments all
/*1*/if(0)/*3*/foo();else/*5*/bar();

The compress phase will discard unneeded AST nodes and their associated comments:

$ echo '/*1*/ if /*2*/ (0) /*3*/ foo(); /*4*/ else /*5*/ bar(); /*6*/' | bin/uglifyjs -c --comments all
/*1*/ /*5*/bar();

One could get all the comments preceding AST nodes by traversing the tree immediately after parse - but that would only access comments 1, 3 and 5 in the example. Comments 2, 4 and 6 are not attached to anything and as such are not available.

A new callback option could be created to be called just after parse to accomplish recording comments 1, 3 and 5 without too much effort. So you would have to store all comments received in callbacks into a data structure of your choosing to be emitted outside of uglify and not inline within minified code - the reason being that some code no longer exists and some comments would no longer have any context.

If one wanted absolutely every comment seen above then the parser would have to be altered to record all comments as they are parsed. Again a callback could be invoked for each comment but the user would have to store and emit the comments outside of uglify and not inline within minified code.

But both of these proposed solutions would interfere with the correct operation of source maps if the user were to emit the collected comments after the source map URL comment. Some other solution would have to be conceived.

[Enkrod]

I would have opened a new Issue but after giving it some thought I belive this belongs here.

Problem

If I include node_modules into my Javascript app and then uglify everything, the licensing information of some of those modules gets lost.

Cause

The modules sourcecode is the licensing-comment, followed by a self-executing function:

/*!
 * Copyright Example
 * has to be included
 */
(function($){...})(jQuery);

In a build-process all the source files first get uglified

/*!
 * Copyright Example
 * has to be included
 */
!function(h){..}(jQuery);

then concatenated and wrapped into a universal module definition

;(function(root, factory) {
  if (typeof define === 'function' && define.amd) {
    define(['jquery'], factory);
  } else if (typeof exports === 'object') {
    module.exports = factory(require('jquery'));
  } else {
    root.themodule = factory(root.jQuery);
  }
}(this, function(jQuery) {
  /*!
   * Copyright Example
   * has to be included
   */
  !function(h){...}(jQuery);
  !function(s){...}(jQuery);
  !function(o){...}(jQuery);
  !function(n){...}(jQuery);
  !function(s){...}(jQuery);
  ...
  return true;
}));

Now here comes the catch:

!function(){} is an expression and does not seem to count as an AST node.

So when this module gets included, required or howerver else introduced into the sourcecode of another app, that is then uglified itself, the comments are not attached to AST nodes and are inevitably dropped.

[alexlamsl]

@Enkrod much as I'd like to comprehend your issue at hand, without a concrete example I can't even begin to think of a possible workaround.

Please file a new issue by following the instructions on the template.

[Enkrod]

@alexlamsl Since this is really not a new issue but completely related to this one I'd rather prefer not to open a new issue.

The problem in short is:

/*!
 * Licence
 */
(function(){
    doSomething();
})();

uglified once becomes

/*!
 * Licence
 */
!function(){doSomething();}

and uglified again becomes

!function(){doSomething();}

(without the licence-comment)

That means that some vendors JS-Modules that already have been uglified once by the vendor may lose their licensing-comments if you include those modules into your code and uglify them again.

If you think this might be something unrelated to this issue and still want me to open a new one I'll gladly do it.

[alexlamsl]

@Enkrod please file a new issue and follow the instructions to provide a reproducible test case:

$ cat test.js
/*!
 * Licence
 */
(function(){
    doSomething();
})();

```js
$ uglifyjs test.js --comments all -bc
/*!

• Licence
  */
  doSomething();

```js
$ uglifyjs test.js --comments all -bc | uglifyjs --comments all -bc
/*!
 * Licence
 */
doSomething();

As you can see, your example above does not illustrate the issue which you claim.

[kzc]

It's the same as the other examples of comments associated with discarded code in this issue:

$ echo '/*A*/0; /*B*/1; /*C*/function f(){ /*X*/2; /*Y*/3; /*Z*/bar(); }' | bin/uglifyjs -c --comments all
/*A*/ /*C*/function f(){/*Z*/bar()}

Notice how comment A is retained despite 0; being discarded because it's at the top of the input, but comment X at the top of the function body is discarded when 2; is dropped.

Bundlers commonly put each source file into their own function body for variable namespace isolation. Perhaps extra logic can be added to uglify to handle leading comments in function bodies in a similar fashion to AST_Toplevel. This would solve most source header license issues in non-scope-hoisting bundlers. If a bundler hoists all sources into a single scope then this heuristic wouldn't work.

[alexlamsl]

@kzc in that case the bundler is doing it wrong – licence comments need to be at the top of the file, not in some random inlined location just to mess with uglification.

[kzc]

@alexlamsl These bundlers were not written with uglify or any minifier in mind. The effect of comment dropping is just more evident now with recent aggressive DCE in uglify - particularly with IIFEs which are common in bundled code:

$ echo '/*A*/ function f(){ /*B*/ foo(); /*IIFE*/ !function(){ bar(); }(); }' | bin/uglifyjs -c --comments all
/*A*/function f(){/*B*/foo(),bar()}

or with constant replacement:

$ echo '/*A*/ function f(){ /*B*/ var x = 1; console.log(x); }' | bin/uglifyjs -c --comments all
/*A*/function f(){console.log(1)}

Bundlers should not have to move all comments from each source file to the top of the bundle to accommodate the idiosyncrasies of one minifier.

A possible comprehensive solution in uglify would be to record all comments prior to the Compressor being run and tack all the orphaned/discarded comments to the end of the output. Comment order would not be preserved however. Preserving comment order with discarded code would be more complicated.

[alexlamsl]

@kzc I see it as uglify-js is not written with any bundlers in mind - it takes JavaScript code, then gives smaller JavaScript code that functions the same way.

In any case, features concerning comments are not on my list of priorities, so Pull Request is welcome for those who cares about this.

[xyzdata]

So, I have to write license info by hands?

1. src

/**
 *
 * @author xgqfrms
 * @license MIT
 * @copyright xgqfrms
 *
 * @description HC
 * @augments
 * @example
 *
 */
(function(){
    doSomething();
})();

2. target

I just want to keep the license as the origin.

/**
 *
 * @author xgqfrms
 * @license MIT
 * @copyright xgqfrms
 *
 * @description HC
 * @augments
 * @example
 *
 */
!function(){doSomething();}

3. result

lost license?

!function(){doSomething();}

[AlexandreBonneau]

Note: this comment comes from https://github.com/webpack-contrib/uglifyjs-webpack-plugin/issues/222

Somehow Uglify does not respect the @license, @preserve nor /*! tags in the comments, and removes all of them.

In my project I tried comments: 'some' and no comments are kept.
Just to be sure, I tried comments: 'all' and all comments are kept.

This is the extract of the Webpack configuration:

const webpackConfig = merge(baseWebpackConfig, {
    mode   : 'production',
    module : {
        rules: [ // Only activate the linting when building for the production
            {
                test   : /\.js$/,
                loader : 'eslint-loader',
                enforce: 'pre',
                include: [resolve('src'), resolve('test')],
                options: {
                    formatter: require('eslint-friendly-formatter'),
                },
            },
        ],
    },
    optimization: {
        minimizer   : [
            new UglifyJsPlugin({
                cache        : true,
                parallel     : true,
                sourceMap    : true,
                uglifyOptions: {
                    nameCache: {},
                    ie8      : false,
                    safari10 : true,
                    compress : { // see https://github.com/mishoo/UglifyJS2#compress-options
                        dead_code    : true,
                        drop_debugger: true,
                        keep_fnames  : false,
                        passes       : 4,
                    },
                    output   : { // See https://github.com/mishoo/UglifyJS2#output-options
                        beautify: false,
                        comments: 'some',
                    },
                    mangle   : { // see https://github.com/mishoo/UglifyJS2#mangle-options
                        keep_fnames: false,
                        toplevel   : true,
                    },
                }
            }),
        ],
    },
    devtool: '#source-map',
    output : {
        libraryTarget: 'umd',
        library      : 'AutoNumeric',
        filename     : 'autoNumeric.min.js',
        path         : resolve('dist'),
    },
    plugins: [],
});

You can reproduce the problem easily by cloning the project repo and trying to compile it yourself:

git clone -b next https://github.com/autoNumeric/autoNumeric.git
cd autoNumeric
yarn
yarn build

...then check the dist/autoNumeric.min.js file ; the license comment is not shown.

[AlexandreBonneau]

I see that this issue has been closed, but without any additional comments since last October.
Has the bug been resolved, or just ignored?