feature: Add logic to ensure only 1 UglifyJsPlugin's instance per compiler

[hulkish]

Problem:

Currently, I think it's too easy to accidentally apply 2 instances of UglifyJsPlugin to the same compiler/compilation.

Scenario:

• You are a webpack newb
• You need to make a production build
• You learn UglifyJsPlugin is the recommended way to minify your bundle, so you make a webpack.config.prod.js and include UglifyJsPlugin.
• You learn that it's recommended to use the --optimize-minimize/-p argument with webpack cli, so u make a production build script:

"scripts": {
  "build:prod": "webpack -p"
}

Since this webpack cli option auto-adds the UglifyJsPlugin:

• You now have attached 2 separate instances of the same plugin.
• Your production build is 'gon be sloowwwwww

Solution

Add a guard to the top of UglifyJsPlugin class method apply which restricts multiple instances of the plugin from being applied to the same compiler, like so:

class UglifyJsPlugin {
  static appliedCompilers = new WeakSet();

  // then use it as a guard:
  apply(compiler) {
    if (UglifyJsPlugin.appliedCompilers.has(compiler)) return;
    appliedCompilers.add(compiler);
    [...]
  }
}

"But... that's user error"

Yes, it absolutely is. However, I have personally come across the mistake several times. My thinking is... the negative impact this potentially introduces is worth safeguarding against.

[michael-ciniawsky]

Does UglifyJSPlugin.name work (Function.prototype.name) ? Would it be worthwhile to consider filtering plugin duplicates generally in webpack? Is it needed to add the whole compiler? Why not static isApplied = false // {Boolean} && if (isApplied) return\n isApplied = true?

[hulkish]

@michael-ciniawsky I think so... but if I recall correctly there is differences in this behavior between node@4.3 - node@8 etc. I'd have to look.

Filtering duplicates could break plugins that need to be duplicated, such as CommonsChunkPlugin. I suppose we could add an opt-in property that plugins would have to set in order to "be consumed uniquely". Ex:

class MyPlugin {
  constructor() {
    this.uniquePerCompiler = true;
  }
}

...then, in webpack/lib/Compiler:

constructor() {
  this.uniquePlugins = new WeakSet();
}

apply(...plugins) {
  for(let i = 0; i < plugins.length; i++) {
    if (!(plugins[i].uniquePerCompiler === true && this.uniquePlugins.has(plugins[i])))  {
      this.uniquePlugins.add(plugins[i]);
      plugins[i].apply(this);
    }
  }
}

if you did isApplied = true - you'd run the risk of incorrectly preventing it being applied to a sibling compiler controlled by MultiCompiler. It needs to be unique per compiler.

[hulkish]

@michael-ciniawsky Also, keep in mind that its adding the compiler to a WeakSet, therefore memory issues relating to "storing" the compiler can be absolved.

[bebraw]

I would go with a property that webpack core interprets. That pushes the problem to the right place.

[hulkish]

@bebraw So, problem with that... It only works if its always the compiler.apply that is called. However, when you call MyPlugin.apply - we then lose that control.

Maybe we can solve this by adding a new method to webpack/lib/Compiler: pluginUnique & using a WeakMap:

• Opt-in

pluginUnique(uniqueBy, name, fn) {
  const events = this.uniquePluginEvents.get(uniqueBy) || [];
  if (events.includes(name)) return;
  this.uniquePluginEvents.set(uniqueBy, events.concat(name));
  this.plugin(name, fn);
}

Or, another idea is to allow it to still be opt-in per plugin, but via helpers that live in webpack/lib/Compiler & using WeakSet):

• Opt-in
• Keeps the concerns in the right places

class Compiler {
  [...]
  constructor() {
    this.uniquePlugins = new WeakSet();
  }

  applyUnique(PluginClass) {
    if (this.uniquePlugins.has(PluginClass)) {
      return true;
    }
    this.uniquePlugins.add(PluginClass);
  }
  [...]
}

// Then, in your plugin:
class MyPlugin {
  apply(compiler) {
    if (compiler.applyUnique(MyPlugin)) return;
  }
}

[bebraw]

@hulkish I'm not sure. It's better to get Tobias' input on that.

[indutny]

What if I want to use the plugin anyway? Like I have different passes of optimizations and want to apply it after each pass?

[hulkish]

@indutny can you give an example? I know there are many plugins that need to be added multiple times (such as CommonsChunkPlugin. However, I can't think of why anyone would ever want to run UglifyJsPlugin twice for any benefit.

[indutny]

Several passes of tree shaking? (I don't use it currently, but it may have uses in the future).

1. Remove unused exports.a = ...
2. Remove dead code with uglify
3. Remove more unused exports.a = ... that appeared because the dead code is gone
4. Apply uglify again?

Anyway, I think it should be better to just print warning rather than skipping the step altogether.

[hulkish]

@indutny Wouldn't uglify running once after tree shaking solve this? I would hope that nothing would ever "need" to be uglified more than once. Since, it's such a heavy task.

I think adding a warning could be more confusing because - once again, many plugins need to be added multiple times.

[indutny]

Uh, right... bad example. I'll have to think if I can come up with anything else.

All in all, it still feels "unsafe" to disable the plugin if it is used twice. In my opinion, warning is better.

[hulkish]

@indutny True, and I totally agree this should not be applied to all plugins. I think it should be instantiated from the plugin itself. As explained in option #2 here: #72 (comment)

[sokra]

I guess we could a a unique check to the apply method of Tapable. If the plugin has a unique property:

• unique === true: We add plugin.constructor to a Set.
• unique is truthy: We add plugin.unique to a Set.

This way babli-plugin and uglifyjs-plugin could set unique = "asset-minimize".

---

Anyway there is a case where you want to apply the uglifyjs plugin multiple times:

new UglifyJsPlugin({
  include: /file\.js$/,
  compress: true
})
new UglifyJsPlugin({
  include: /other-file\.js$/,
  compress: { /* some special options */ }
})

[mikesherov]

I don't think this can be solved generically. As Tobias has shown above, there is no generic case where, even for Uglify, where we know uniqueness should happen.

There are two things we can do:

1. If user has supplied the p flag, ensure that the plugins that are added by that flag aren't also added manually.
2. Make sure the optimize plugins all use cache so that even if they are applied twice, the second run is essentially a no-op.

[hulkish]

Per that use case @sokra mentioned for using UglifyJsPlugin multiple - I feel like it would be better if we even still forced the unique behavior. And, instead provided a way for them to accomplish the same using during a single pass of the plugin''s event handlers.

Maybe something like:

new UglifyJsPlugin({
  optionsPerAsset: [
    {
      include: /file\.js$/,
      compress: true
    },
    {
      include: /other-file\.js$/,
      compress: { /* some special options */ }
    },
  ]
})

[mikesherov]

Point is, there is a lot we can do for individual plugins, and with that in mind, having a generic "unique plugin" system seems overkill for handling just the specific cases, but that's just my opinion, and I'm not stuck on it :-)

[hulkish]

@mikesherov I agree, this could just as easily be totally the responsibility of the plugin. That was my original idea but wondered what u folks think.

[mikesherov]

Exactly. I'm just saying what I think ;-)

[hulkish]

Closing this in light of webpack/webpack#5248 (comment)

[tstackhouse]

@hulkish I think I have an example use-case to @indutny's point of multiple uglify instances: Say I have all my application logic in main.bundle.js and I have my vendor libraries in vendor.bundle.js. I can uglify my app code, including mangling and everything still works, but if I apply mangling to my vendor code, things break horribly (and are very difficult to trace). Therefore I want to set up 2 copies of the plugin, one to uglify my application logic with mangling enabled, and another for my vendor libraries, without mangling.