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

Document how javascript is being minified in production #15

Closed
trsteel88 opened this issue Jun 15, 2017 · 16 comments · Fixed by #152
Closed

Document how javascript is being minified in production #15

trsteel88 opened this issue Jun 15, 2017 · 16 comments · Fixed by #152
Labels
Feature New Feature

Comments

@trsteel88
Copy link

I can't find how javascript is being minified in production or what additional options we can give it (e.g. stripping special comments).

@deckchairlabs
Copy link

@weaverryan
Copy link
Member

Indeed - we need a way to directly set the uglify options. What about:

Encore
    .setUglifyJsOptions({...})

The options are here: https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/master/README.md. I can't think of any reason why it would need to be any more complex than this :)

@stof
Copy link
Member

stof commented Jun 15, 2017

If we expose the options, we should decide how this interacts with the default options provided by Encore (and we should document them so that people can know what to expect)

@weaverryan
Copy link
Member

@stof In this case, I was thinking it would be an array merge, where (obviously) the user's config takes precedence on conflicts. But, I did something a bit different for the babel config. In that case, since the default babel config has a deeper structure (and maybe you want to just tweak on embedded key), it's configured with a callback, where we pass you the default config and you modify it. We could stay consistent and use that approach everywhere.

@fabpot
Copy link
Member

fabpot commented Jun 15, 2017

I would avoid merging configurations if this is not just key => value pairs. Not sure if that's the case here though :)

@weaverryan
Copy link
Member

The options (that we add) are a simple, scalar key-pair values now... but that may not be true forever :).

Let's go with the callback way of doing it:

Encore
    .configureUglifyJsPlugin(function(options) {
        // options already has our default values
    });

@elmariachi111
Copy link

I tried to strip out license comments and minfify the results further in production by doing this (encore 0.12 / webpack 3.5):

Encore.configureBabel(function(babelConfig) { //this is actually documented.
    babelConfig.comments = false;
    return babelConfig;
})

but it doesn't strip the comments (tried a little bit but I cannot make it work :( ).

So in a 2nd attempt I tried using the webpack.UglifyJS plugin:

if (Encore.isProduction()) {
    Encore.addPlugin(new webpack.optimize.UglifyJsPlugin({
        comments: false,
        compress: {
            warnings: false,
            drop_console: true,
        }
    }))
}

but that breaks some (vendor) scripts on my page (parsley.js in my case :( ). I guess the most advanced way of achieving a real minification is using babili (https://webpack.js.org/plugins/babili-webpack-plugin/) that does minify >es5 code first but I didn't try that one yet.

@weaverryan
Copy link
Member

A quick search makes it look like comments: false is the default for UglifyJsPlugin. Exactly what comments are you seeing in the final, compiled code? There was a recent fix - see #132 - for extra comments in production. I need to create a release so people can get that. I'll see if I can do that right now :)

@weaverryan
Copy link
Member

0.13.0 is out with that fix! I'm curious if that makes a difference for you @elmariachi111 or if you're seeing comments for some other reason.

@elmariachi111
Copy link

Well that certainly looks much better now. Now only the license plates (starting with /*!) are left. That's fine for me as it seems to be Babel/Webpack's default behaviour but of course it would be nicer if the text extractor (forgot the real name) would put these into another LICENSES file.

screen shot 2017-08-09 at 17 22 58

@weaverryan
Copy link
Member

Indeed - it looks like those comments are kept on purpose: webpack/webpack#324 (comment)

And it looks like you can opt to remove them via the extractComments option: https://github.com/webpack-contrib/uglifyjs-webpack-plugin#user-content-options... which can easily be set once we actually make the options to Uglify configurable (which is just a chore we need to take care of)

@elmariachi111
Copy link

Yeah. As said above: when I added the uglifyjs plugin to the pipeline some parts of my frontend code broke -> that might be some problem on my side but I guess it mangles some symbols from the common chunk so that the page-specific script couldn't access it anymore. Just sayin :)

weaverryan added a commit that referenced this issue Sep 14, 2017
This PR was squashed before being merged into the master branch (closes #152).

Discussion
----------

Add various methods to configure default plugins

This PR adds some methods to configure default plugins (closes #148, closes #87 and closes #15):

* `Encore.configureDefinePlugin(callback)`
* `Encore.configureExtractTextPlugin(callback)`
* `Encore.configureFriendlyErrorsPlugin(callback)`
* `Encore.configureLoaderOptionsPlugin(callback)`
* `Encore.configureUglifyJsPlugin(callback)`

Other changes:

* Allows the `clean-webpack-plugin` to be configured using `Encore.cleanupOutputBeforeBuild(paths, callback)`
* Fixed a small mistake in the `Encore.configureManifestPlugin()` jsdoc
* Reorganized flags/callbacks in the constructor of `webpack.config.js` since it was starting to be a bit hard to read. I'm not sure about the way I splitted things though, let me know what you think about it.
* Renamed `common-chunks.js` to `commons-chunks.js` since the name of the plugin is `CommonsChunkPlugin`

@weaverryan: Not directly related but while doing all of that I noticed that `sassOptions` uses snake-case whereas I used camel-case in #144 for `enablePreactPreset()` and in #115 for `configureRuntimeEnvironment()`, should we do something about that?

***Edit 1:** Added `Encore.configureCleanWebpackPlugin()`*
***Edit 2:** Removed `Encore.configureCleanWebpackPlugin()` to use `Encore.cleanupOutputBeforeBuild(paths, callback)` arguments instead*

Commits
-------

286787a Minor text changes
f72614b Remove configureCleanWebpackPlugin and use cleanupOutputBeforeBuild arguments instead
90c8565 Add various methods to configure default plugins
@mifan-twan
Copy link

i got error in terser with message "minify" undefined when build into production

@Kocal
Copy link
Member

Kocal commented Feb 3, 2019

Please open a new issue.

@Lyrkan
Copy link
Collaborator

Lyrkan commented Feb 3, 2019

@mifan-twan That's caused by an issue with the latest version of Terser that was released yesterday (see webpack-contrib/terser-webpack-plugin#66).

It's breaking a lot of builds and a PR that should fix it has already been created (terser/terser#254). Not much we can't do on our side but wait for a new version.

In the meantime there are some workarounds available in the issue I linked.

@mifan-twan
Copy link

@Lyrkan thanks you for the information, I spent a day looking for causes. and I still haven't produced anything.

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

Successfully merging a pull request may close this issue.

9 participants