Skip to content
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.

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

Legal, License, Copyright comments should not be removed by default #222

Closed
jnachtigall opened this Issue Feb 2, 2018 · 29 comments

Comments

Projects
None yet
7 participants
@jnachtigall
Copy link

jnachtigall commented Feb 2, 2018

Steps to reproduce:

Version in package.json:

    "grunt-webpack": "^3.0.2",
    "uglifyjs-webpack-plugin": "^1.1.8",
    "webpack": "^3.10.0",
    "webpack-bundle-analyzer": "^2.9.2"

My webpack.config.js:

const path = require('path');
const webpack = require('webpack');
const UglifyJSPlugin = require('uglifyjs-webpack-plugin');
const BundleAnalyzerPlugin = require('webpack-bundle-analyzer')
  .BundleAnalyzerPlugin;

const config = {
  // entry: ["babel-polyfill", "./app/js"]
  entry: {
    // Further entry points can be added here
    // NB: If you add further ones, then you probably also want to move
    // dependencies like jQuery into their own bundle
    // see https://webpack.js.org/guides/code-splitting/#prevent-duplication

    // the babel-polyfill and fetch polyfill needs to be loaded only once,
    // hence only added to app.js which is executed on every page
    app: ['babel-polyfill', 'whatwg-fetch', './src/components/app.js'],
    resizer: ['./src/prototype-assets/viewport-resizer/main.js'],
    fractalmenuenhancer: [
      './src/prototype-assets/fractal-menu-enhancer/main.js'
    ]
  },
  module: {
    rules: [
      {
        test: /\.js$/,
        exclude: /node_modules/,
        use: {
          loader: 'babel-loader'
        }
      }
    ]
  },
  resolve: {
    extensions: ['*', '.js', '.jsx']
  },
  output: {
    filename: '[name].bundle.js',
    path: path.resolve(__dirname, 'public/js/')
  },
  plugins: [
    new webpack.EnvironmentPlugin({
      // pass 'development' unless process.env.NODE_ENV is defined to further tasks/loaders/babel
      NODE_ENV: 'development'
    })
  ]
};

// Extend config to use best, small build for prod and fast, big for dev
if (process.env.NODE_ENV === 'production') {
  config.devtool = 'source-map';
  config.plugins.push(
    new UglifyJSPlugin({
      sourceMap: true
    })
  );
  config.plugins.push(
    new BundleAnalyzerPlugin({
      analyzerMode: 'static',
      reportFilename: '../webpack-bundle-size-analyser.html',
      openAnalyzer: false
    })
  );
} else {
  config.devtool = 'eval-source-map';
}

module.exports = config;

Now in the ./src/components/app.js there are several 'vendor' libraries imported like: import $ from 'jquery';

Actual results:
When running npm run build which creates a uglified minimal bundle, then all comments are stripped.

For instance, jquery has this header which is not present any more in the uglified bundle:

/*!
 * jQuery JavaScript Library v3.3.1
 * https://jquery.com/
 *
 * Includes Sizzle.js
 * https://sizzlejs.com/
 *
 * Copyright JS Foundation and other contributors
 * Released under the MIT license
 * https://jquery.org/license
 *
 * Date: 2018-01-20T17:24Z
 */

Expected results:
Legal copyright comments should not be stripped by default. The comment at webpack/webpack#324 (comment) points out, that

it's intended that webpack preserves some comments. This is for legal reasons. By default comments with @license, @preserve or starting with /*! are preserved

At the moment this is not the case: By default all comments are stripped at the moment.

@jnachtigall

This comment has been minimized.

Copy link
Author

jnachtigall commented Feb 2, 2018

Interestingly, the correct legal comments are successfully extracted when adding extractComments:

    new UglifyJSPlugin({
      sourceMap: true,
      extractComments: true
    })

Strange, that the comments are not preserved in the bundle itself.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Feb 2, 2018

@jnachtigall Seems bug. Thanks for issue. PR welcome!

@julmot

This comment has been minimized.

Copy link
Contributor

julmot commented Mar 5, 2018

Is there any way to "downgrade" or similar in the meantime? I'd like to upgrade to Webpack v4, but I can't without license comments.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Mar 5, 2018

@julmot tomorrow i fix this or you can do PR 👍

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Mar 6, 2018

Done in #250

@michael-ciniawsky michael-ciniawsky removed this from the 1.2.5 milestone Mar 16, 2018

@michael-ciniawsky

This comment has been minimized.

Copy link
Member

michael-ciniawsky commented Mar 16, 2018

Released in v1.2.4 🎉

@michael-ciniawsky

This comment has been minimized.

Copy link
Member

michael-ciniawsky commented Mar 16, 2018

@julmot

This comment has been minimized.

Copy link
Contributor

julmot commented Mar 16, 2018

Thanks @michael-ciniawsky So when does this come into Webpack itself? I'm not directly depending on this plugin:

topic

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Mar 16, 2018

@julmot already in webpack https://github.com/webpack/webpack/blob/master/package.json#L24 just update cache (npm/yarn cache clear) and/or lock file

@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented May 21, 2018

Hi, I've been trying to follow the documentation and a few of the issues here, and I'm not sure this is actually working.

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

module.exports = {
    entry: path.resolve(__dirname, 'src/index.js'),
    output: {
        filename: 'production-2.js',
        path: path.resolve(__dirname, 'dist')
    },
    mode: 'production',
    optimization: {
        minimizer: [
            new UglifyJsPlugin({
                uglifyOptions: {
                    output: {
                        comments: true,
                    }
                },
                extractComments: true, // /(?:^!|@(?:license|preserve))/i
            })
        ]
    }
};

Shouldn't this in theory keep all comments and save them to the external file?

my package.json uses:

"uglifyjs-webpack-plugin": "^1.2.5",
"webpack": "^4.8.2",
"webpack-cli": "^2.1.3"
@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented May 21, 2018

@andrevenancio remove all options UglifyJsPlugin works right now as expected - doesn't remove legal comments

@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented May 21, 2018

Hey @evilebottnawi then I might be doing something wrong. cause I've been trying different options and so far no luck..

I've even went to my node_modules and comment the shit out of node_modules/uglifyjs-webpack-plugin/dist/index.js and the array that stores my comments is always empty [] (from: data.extractedComments)

Here's my code: https://github.com/andrevenancio/license-example/blob/master/webpack.production-2.js

@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented May 21, 2018

We've been trying to help fix an issue on gl-matrix (toji/gl-matrix#304) and hence my tests

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented May 21, 2018

@andrevenancio what you want remove all comments or leave legal comments?

@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented May 21, 2018

Yeah, sorry I've been explaining this poorly.

Yeah I'm expecting to scrap all the comments except the ones tagged with @license on gl-matrix we're currently missing the license itself on our src/gl-matrix.js (which is the entry point for webpack/rollup. We can add that easily, but for my tests I'm expecting that at least one of this comments is kept .

The idea is to strip every comment except the license from our gl-matrix repo.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented May 21, 2018

@andrevenancio i.e. only with @license directive, yes?

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented May 21, 2018

You can avoid set uglifyOptions.output.comments if you set extractComments (https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/master/src/index.js#L46)

@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented May 21, 2018

yeah, the idea is to only keep whatever has @license in our source. I've tried having just extractComments: true and no luck. Can you pull the repo and try to run npm run prod2?

Its either a problem with webpack or I dont know.. If I run the packages specified here I can't keep the license

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented May 21, 2018

Use extractComments: /@(?:license)/i? Also you can use extractComments: (node, comment) => { console.log(comment); return true; } to debug what comments parses. uglify has some problems with some comments mishoo/UglifyJS2#2500

@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented May 21, 2018

Still no luck.

If I use the function and log the comments I can see a bunch of comments coming through (mainly the comments used to generate the docs (from node_modules/gl-matrix),

However if I try to use the regexp to validate the comment the regexp fails and no comment is added

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

module.exports = {
    entry: path.resolve(__dirname, 'src/index.js'),
    output: {
        filename: 'production-2.js',
        path: path.resolve(__dirname, 'dist')
    },
    mode: 'production',
    optimization: {
        minimizer: [
            new UglifyJsPlugin({
                // extractComments: /@(?:license)/i
                extractComments: (node, comment) => {
                    console.log('comment', String(comment).search(/@(?:license)/i));
                    /*console.log(comment); */
                    return String(comment).search(/@(?:license)/i) !== -1;
               }
            })
        ]
    }
};

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented May 21, 2018

@andrevenancio just put here what you comments you have and what comments should be left and should be strip.

@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented May 21, 2018

This are the comments I've been trying to test against.

https://github.com/andrevenancio/license-example/blob/master/src/index.js

None of them pass the regexp.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented May 21, 2018

@andrevenancio looks like problem in uglify (i wrote about this above)

@andrevenancio

This comment has been minimized.

Copy link

andrevenancio commented May 21, 2018

I see... so its just a matter of waiting for uglify to resolve the bug you posted and hopefully then you'll update uglifyjs-webpack-plugin?... 😢

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented May 21, 2018

@andrevenancio i think this bug never was solve due in arch problem inside uglify ast parser, but if they fixed, you need only update deps and lock file, I would suggest that you try switch to babel-minify. I near future i will release new minor version allow to use own minify function. It is can may help.

@xyzdata

This comment has been minimized.

Copy link

xyzdata commented Jul 10, 2018

@xyzdata

This comment has been minimized.

Copy link

xyzdata commented Jul 12, 2018

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

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

3. result

lost license?

!function(){doSomething();}

mishoo/UglifyJS2#2500 (comment)

@AlexandreBonneau

This comment has been minimized.

Copy link

AlexandreBonneau commented Oct 10, 2018

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

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: [],
});

Could you please re-open that bug?

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Oct 10, 2018

@AlexandreBonneau we just wrapper around uglify-js, please create issue in uglify-js repo if you think what some comments are removed/not removed (uglify-es if you use uglifyjs-webpack-plugin@1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.