feat: support `minify` function option #325

alexander-akait · 2018-07-03T14:43:04Z

⭐

One question: what do with extracting comment for custom minify function, if people use uglify-js/terser our logic https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/master/src/uglify/minify.js#L45 is work good, maybe we should export this function to avoid copy/paste this code

evenstensberg · 2018-07-04T03:55:28Z

README.md

@@ -89,6 +90,8 @@ module.exports = {

 ### `cache`

+If you use custom `minify` function please read `minify` option section for cache invalidation correctly.


a custom, see syntax below

evenstensberg · 2018-07-04T03:55:39Z

README.md

@@ -178,6 +181,8 @@ Number of concurrent runs.

 ### `sourceMap`

+If you use custom `minify` function please read `minify` option section for handle source maps correctly.


If you use custom minifyfunction please readminify option section for handle source maps correctly. -> If you use your own minifyfunction please read theminify section for handling source maps correctly.

evenstensberg · 2018-07-04T03:56:58Z

README.md

+    //   );
+    // },
+    minify(file, sourceMap) {
+      // Look https://github.com/mishoo/UglifyJS2#minify-options


Remove Look, a link is enough perhaps

evenstensberg · 2018-07-04T03:57:24Z

README.md

+    //   );
+    // },
+    minify(file, sourceMap) {
+      // Look https://github.com/fabiosantoscode/terser#minify-options


Strip Look

evenstensberg · 2018-07-04T03:59:15Z

test/minify.test.js

+        const errors = stats.compilation.errors.map(cleanErrorStack);
+        const warnings = stats.compilation.warnings.map(cleanErrorStack);
+
+        expect(errors).toMatchSnapshot('errors');


Don't think this is needed, jest will resolve it

@ev1stensberg will be done in other PR (refactor tests) 👍

evenstensberg · 2018-07-04T04:00:35Z

test/minify.test.js

+    function removeAbsoluteSourceMapSources(source) {
+      if (source.map && source.map.sources) {
+        // eslint-disable-next-line no-param-reassign
+        source.map.sources = source.map.sources.map(sourceFromMap => path.relative(process.cwd(), sourceFromMap));


any way to copy and reassign instead of directly mutating here? Perhaps extracting into a variable and reassigning would be better https://stackoverflow.com/questions/34716651/js-array-prototype-map-happens-to-be-mutable

@ev1stensberg why? It is increase lines of code and decrease perf, but we just need remove absolute path. I don't think it is critical here.

evenstensberg · 2018-07-04T04:01:42Z

test/minify.test.js

+
+        for (const file in stats.compilation.assets) {
+          if (Object.prototype.hasOwnProperty.call(stats.compilation.assets, file)) {
+            expect(removeAbsoluteSourceMapSources(stats.compilation.assets[file].sourceAndMap())).toMatchSnapshot(file);


assertion is a bit long, maybe a variable to hold the assertion value would be good

@ev1stensberg big in snapshots? I think it is normal, it is allow to catch some strange problem with source map and uglification

Alright. Thought it would be better to test variables containing the values, but that might be hard with compilation instances

ooflorent

The changes work but I don't like the minify option. I think this option is a too low-level. Having a plugin per minifier would have been better.

In a near future (🤞) when we will have specified plugins, this option would no longer be relevant. Hence my hesitation about merging this.

ooflorent · 2018-07-06T07:01:38Z

README.md

+]
+```
+
+By default plugin use `uglify-es` package.


@ooflorent same people want to use own minification (forks) and without this low level function it is impossible, creating new plugin for this situation is not good solution, because it is require copy/paste 99% code of plugin, also we other fix small problems, this will require people to constantly monitor this and cause a lot of inconvenience. Also it is simple way to test alpha/beta/rc/etc versions.

alexander-akait · 2018-07-09T14:10:11Z

/cc @ooflorent please review, it is one blocker for terser and future minimize plugins

kzc · 2018-07-12T15:50:55Z

Something to consider... if the default minify function is changed, the version remains "uglify-es":"3.3.9" in the uglifyjs-webpack-plugin cache key:

uglifyjs-webpack-plugin/src/index.js

Lines 177 to 179 in 0174605

    
           if (this.options.cache) { 
        
             const defaultCacheKeys = { 
        
               'uglify-es': versions.uglify,

and the cached assets will be invalid.

alexander-akait · 2018-07-12T16:25:55Z

@kzc it is not critical, uglify-es is not updated long time, cache will works as expected

kzc · 2018-07-12T16:46:27Z

uglify-es is not updated long time, cache will works as expected

Until the module with the minify replacement function is upgraded - terser, uglify-js, or whatever. You might mention in the docs if this minify field is overriden or dependencies are updated then the user should delete their build cache. You could also elect to allow the user to provide a custom version string to workaround this issue.

alexander-akait · 2018-07-12T17:27:59Z

@kzc PR welcome

alexander-akait · 2018-07-12T17:29:06Z

Also we provide way to invalidate cache https://github.com/webpack-contrib/uglifyjs-webpack-plugin#terser

alexander-akait mentioned this pull request Jul 3, 2018

feat: custom minify function #321

Closed

alexander-akait force-pushed the feat-minify-function-option branch 3 times, most recently from a0656df to d1152b2 Compare July 3, 2018 15:54

alexander-akait requested review from ooflorent and shellscape July 3, 2018 15:57

alexander-akait force-pushed the feat-minify-function-option branch 2 times, most recently from 0802668 to f8a79a1 Compare July 3, 2018 16:10

evenstensberg reviewed Jul 4, 2018

View reviewed changes

ooflorent reviewed Jul 6, 2018

View reviewed changes

alexander-akait force-pushed the feat-minify-function-option branch 2 times, most recently from d929a6a to 9024f0a Compare July 9, 2018 14:00

feat: support minify function option

876119d

alexander-akait force-pushed the feat-minify-function-option branch from 9024f0a to 876119d Compare July 9, 2018 14:07

ooflorent approved these changes Jul 12, 2018

View reviewed changes

alexander-akait merged commit 0174605 into master Jul 12, 2018

alexander-akait deleted the feat-minify-function-option branch July 12, 2018 11:04

snyk-bot mentioned this pull request May 17, 2022

[Snyk] Upgrade source-map from 0.6.1 to 0.7.3 AsherLi0103/uglifyjs-webpack-plugin#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support `minify` function option #325

feat: support `minify` function option #325

alexander-akait commented Jul 3, 2018 •

edited

evenstensberg Jul 4, 2018 •

edited

evenstensberg Jul 4, 2018

evenstensberg Jul 4, 2018

evenstensberg Jul 4, 2018

evenstensberg Jul 4, 2018

alexander-akait Jul 4, 2018

evenstensberg Jul 4, 2018

alexander-akait Jul 9, 2018

evenstensberg Jul 4, 2018

alexander-akait Jul 4, 2018

evenstensberg Jul 4, 2018

ooflorent left a comment

ooflorent Jul 6, 2018

alexander-akait Jul 6, 2018

alexander-akait commented Jul 9, 2018

kzc commented Jul 12, 2018

alexander-akait commented Jul 12, 2018

kzc commented Jul 12, 2018

alexander-akait commented Jul 12, 2018

alexander-akait commented Jul 12, 2018

		@@ -89,6 +90,8 @@ module.exports = {

		### `cache`

		If you use custom `minify` function please read `minify` option section for cache invalidation correctly.

		@@ -178,6 +181,8 @@ Number of concurrent runs.

		### `sourceMap`

		If you use custom `minify` function please read `minify` option section for handle source maps correctly.

feat: support minify function option #325

feat: support minify function option #325

Conversation

alexander-akait commented Jul 3, 2018 • edited

evenstensberg Jul 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ooflorent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Jul 9, 2018

kzc commented Jul 12, 2018

alexander-akait commented Jul 12, 2018

kzc commented Jul 12, 2018

alexander-akait commented Jul 12, 2018

alexander-akait commented Jul 12, 2018

feat: support `minify` function option #325

feat: support `minify` function option #325

alexander-akait commented Jul 3, 2018 •

edited

evenstensberg Jul 4, 2018 •

edited