fix terser webpack plugin compatibility #436

Bobgy · 2019-01-29T15:21:31Z

closes #426

Bobgy · 2019-01-29T15:56:46Z

I didn't update tests, but test is pulling in latest webpack by default. Seeing CIs passing should already prove the new version works with latest webpack without changing fixtures a little bit.

src/misc/get-uglify-plugin.js

Bobgy · 2019-02-01T04:08:30Z

New behavior:

when terser-webpack-plugin exists, it will use it instead of uglify-webpack-plugin
when using terser-webpack-plugin, it will pass the same options from uglify-webpack-plugin to it

GGAlanSmithee

A few comments, mostly on syntax, a small logical change, happy with this PR overall though.

tests/legacy/fixtures/sw-minify-false/webpack.config.js

tests/legacy/fixtures/sw-minify-auto/webpack.config.js

src/service-worker.js

GGAlanSmithee · 2019-02-01T14:31:26Z

src/service-worker.js

@@ -82,28 +82,28 @@ export default class ServiceWorker {
    };

    if (this.minify === true) {
-      if (!UglifyJsPlugin) {
-        throw new Error('OfflinePlugin: uglifyjs-webpack-plugin is required to preform a minification')
+      if (!makeUglifyJsPlugin) {


I think makeUglifyJsPlugin !== null would be more explicit

I changed to makeUglifyJsPlugin == null. I think that's what you mean here

GGAlanSmithee · 2019-02-01T14:44:07Z

src/misc/get-uglify-plugin.js

@@ -1,5 +1,11 @@
 const webpack = require('webpack');
 let UglifyJsPlugin;
+let isUsingTerser = false;
+
+try {


try { UglifyJsPlugin = require('terser-webpack-plugin'); isUsingTerser = true; } catch (e) {} try { UglifyJsPlugin = webpack.optimize.UglifyJsPlugin; } catch (e) {} if (!UglifyJsPlugin) { try { UglifyJsPlugin = require('uglifyjs-webpack-plugin'); } catch (e) {} }

Regarding this logic, depending on what optimization libs are present, terser-webpack-plugin will not be used. For instance, I do not know under what circumstances webpack.optimize.UglifyJsPlugin will be available, but given the current implementation, terser-webpack-plugin will have the least presidence, is this correct? Maybe explore a different solution, maybe something like:

try { UglifyJsPlugin = require('terser-webpack-plugin'); isUsingTerser = true; if (!UglifyJsPlugin) { UglifyJsPlugin = webpack.optimize.UglifyJsPlugin; } if (!UglifyJsPlugin) { UglifyJsPlugin = require('uglifyjs-webpack-plugin'); } } catch (e) {}

?

Alternatively, wrap this in a function with early exits. What do you think?

oops, you are totally right. Let me fix accordingly

I didn't pull out the try catch blocks, because it was meant for catching exceptions thrown by require as expected for each require.

src/misc/get-uglify-plugin.js

GGAlanSmithee · 2019-02-01T14:46:48Z

@NekR do you have time to check this? Will require a new version and npm deploy!

Bobgy · 2019-02-02T07:31:16Z

@GGAlanSmithee Thank you for the detailed CR. I will address the issues.

Bobgy

I've addressed issues accordingly. Let me know if there's anything else you've got some concerns

Bobgy · 2019-02-02T07:55:42Z

src/service-worker.js

@@ -82,28 +82,28 @@ export default class ServiceWorker {
    };

    if (this.minify === true) {
-      if (!UglifyJsPlugin) {
-        throw new Error('OfflinePlugin: uglifyjs-webpack-plugin is required to preform a minification')
+      if (!makeUglifyJsPlugin) {


I changed to makeUglifyJsPlugin == null. I think that's what you mean here

src/service-worker.js

Bobgy · 2019-02-02T08:12:53Z

src/misc/get-uglify-plugin.js

@@ -1,5 +1,11 @@
 const webpack = require('webpack');
 let UglifyJsPlugin;
+let isUsingTerser = false;
+
+try {


oops, you are totally right. Let me fix accordingly

Bobgy · 2019-02-02T08:29:21Z

src/misc/get-uglify-plugin.js

@@ -1,5 +1,11 @@
 const webpack = require('webpack');
 let UglifyJsPlugin;
+let isUsingTerser = false;
+
+try {


I didn't pull out the try catch blocks, because it was meant for catching exceptions thrown by require as expected for each require.

Bobgy · 2019-02-03T10:08:08Z

it seems CI is failing on the same code that passed before. I don't know why. I will debug soon

GGAlanSmithee · 2019-02-04T10:09:49Z

@Bobgy Thanks for your continued work. I see that you reverted your last commit, so I guess there is still something you want to address? Will await further comments from you.

Bobgy · 2019-02-09T15:24:59Z

@GGAlanSmithee I made the revert just to retry CI and it was still failing. It seemed that latest webpack was broken at that time. Now I just removed the revert and CI is passing again. Please review if you have time.

It seems I messed up a little bit when rebasing. Fixing it right now.

Bobgy · 2019-02-09T15:33:44Z

OK, all issues fixed and CI is passing again

GGAlanSmithee

@Bobgy

Thanks for your continued work on this PR, almost ready to merge this now. I've requested some additional changes, since it's the best time to do these now before this get merged. It's mostly a name change to signify that we might not be using UglifyJsPlugin, and some minor code change.

If possible, please do these changes. Othervise let me know if you don't have time and I can do a follow-up PR.

Thanks!

docs/FAQ.md

lib/misc/get-uglify-plugin.js

src/misc/get-uglify-plugin.js

src/service-worker.js

Bobgy · 2019-02-13T03:46:51Z

@GGAlanSmithee Thank you for your thoughtful comments.

GGAlanSmithee · 2019-02-13T06:35:49Z

So green 💯

Bobgy commented Jan 30, 2019

View reviewed changes

src/misc/get-uglify-plugin.js Outdated Show resolved Hide resolved

Bobgy mentioned this pull request Feb 1, 2019

Fix caching with failed requests #437

Merged

GGAlanSmithee requested changes Feb 1, 2019

View reviewed changes

Bobgy commented Feb 2, 2019

View reviewed changes

Bobgy force-pushed the terser_compatibility branch from ec008d1 to 5f53f09 Compare February 3, 2019 03:46

This was referenced Feb 4, 2019

add faq and troubleshooting entry regarding insecure response #333

Merged

add FAQ entry regarding network / server / proxy errors #415

Merged

Bobgy force-pushed the terser_compatibility branch from f57b6c3 to 0529951 Compare February 9, 2019 15:17

Bobgy added 5 commits February 9, 2019 23:25

fix terser-webpack-plugin compatibility

d829d28

commit generated files

9922f0f

fix ci test failures

2a3f0e8

update travis config to cover webpack 4 with uglify-webpack-plugin

ae0cc0b

clean up code according to CR

ad2190c

Bobgy force-pushed the terser_compatibility branch from 0529951 to ad2190c Compare February 9, 2019 15:27

GGAlanSmithee requested changes Feb 11, 2019

View reviewed changes

GGAlanSmithee mentioned this pull request Feb 11, 2019

getRegistration Scope #439

Merged

doc, code clean up and rename according to CR

ef1a087

GGAlanSmithee approved these changes Feb 13, 2019

View reviewed changes

GGAlanSmithee merged commit 66cde2c into NekR:master Feb 13, 2019

Bobgy deleted the terser_compatibility branch November 7, 2019 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix terser webpack plugin compatibility #436

fix terser webpack plugin compatibility #436

Bobgy commented Jan 29, 2019

Bobgy commented Jan 29, 2019

Bobgy commented Feb 1, 2019

GGAlanSmithee left a comment

GGAlanSmithee Feb 1, 2019

Bobgy Feb 2, 2019

GGAlanSmithee Feb 1, 2019

Bobgy Feb 2, 2019

Bobgy Feb 2, 2019

GGAlanSmithee commented Feb 1, 2019

Bobgy commented Feb 2, 2019

Bobgy left a comment

Bobgy Feb 2, 2019

Bobgy Feb 2, 2019

Bobgy Feb 2, 2019

Bobgy commented Feb 3, 2019

GGAlanSmithee commented Feb 4, 2019

Bobgy commented Feb 9, 2019

Bobgy commented Feb 9, 2019

GGAlanSmithee left a comment

Bobgy commented Feb 13, 2019

GGAlanSmithee commented Feb 13, 2019

fix terser webpack plugin compatibility #436

fix terser webpack plugin compatibility #436

Conversation

Bobgy commented Jan 29, 2019

Bobgy commented Jan 29, 2019

Bobgy commented Feb 1, 2019

GGAlanSmithee left a 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

GGAlanSmithee commented Feb 1, 2019

Bobgy commented Feb 2, 2019

Bobgy left a 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

Bobgy commented Feb 3, 2019

GGAlanSmithee commented Feb 4, 2019

Bobgy commented Feb 9, 2019

Bobgy commented Feb 9, 2019

GGAlanSmithee left a comment

Choose a reason for hiding this comment

Bobgy commented Feb 13, 2019

GGAlanSmithee commented Feb 13, 2019