Add Encore.sortPlugins() method

[Lyrkan]

This PR tries to solve #163 (ping @stof) by adding a sortPlugins method to the public API.

Usage:

const webpack = require('webpack');
const Plugin1 = require('plugin1');
const Plugin2 = require('plugin2');

Encore.sortPlugins([
    Plugin2,
    webpack.optimize.UglifyJsPlugin,
    Plugin1
])

Since it relies on instanceof checks it won't allow to sort two instances of a given plugin relatively to eachother but I'm not sure that case needs to be handled anyway...

I also changed the default position of the ManifestPlugin so it is added after custom plugins. This is done in order to prevent issues with other plugins that emit assets (e.g. CopyWebpackPlugin and ConcatPlugin after #164 is merged).

[weaverryan]

Yo!

I’m not against this solution... but I want to exhaust all other possibilities, just be sure this solution is inherently a bit complex.

First, is the manifest plugin & copy plugin ordering issue confirmed? Do we know for sure that order matters? Second, do we know any other issues? Stop mentioned one about Uglify, can we find a real example?

Also, what about adding a prependPlugin() method. Along with the existing method, you could then add plugins before the core plugins or after... which doesn’t give you complete flexibility, but much more.

What do you think?

[Lyrkan]

Hi Ryan,

I have no idea if that's the right solution either... see this more as a POC of what I described in the comments of the issue :)

I tried to find a middle ground between something that wouldn't be flexible enough and something that would do the same than config.plugins = config.plugins.sort((plugin) => { ... })) (which would be a real PITA to configure).

First, is the manifest plugin & copy plugin ordering issue confirmed? Do we know for sure that order matters?

I think it is, I've also seen it mentionned in this thread (ping @davidmpaz).

From my understanding the order of plugins does matter. Let's say you have two plugins that both have a callback on the same event:

function Plugin1() {}
Plugin1.prototype.apply = function(compiler) {
  compiler.plugin('emit', function(compilation, callback) {
    // ...
  }
}

function Plugin2() {}
Plugin2.prototype.apply = function(compiler) {
  compiler.plugin('emit', function(compilation, callback) {
    // ...
  }
}

If you add [new Plugin1(), new Plugin2()] and run webpack, the callback from Plugin1 will be called before the one from Plugin2. So if Plugin1 does something that is needed by Plugin2 during that step it won't work properly.

I don't know if that's the case for UglifyJsPlugin since it highly depends on how it is implemented but I definitely think that's something that users should be able to configure.

Also, what about adding a prependPlugin() method. Along with the existing method, you could then add plugins before the core plugins or after... which doesn’t give you complete flexibility, but much more.

I thought about that too, but I'm not sure that it would be flexible enough... what if a plugin needs to be added between two of the plugins added by Encore?

[weaverryan]

If you add [new Plugin1(), new Plugin2()] and run webpack, the callback from Plugin1 will be called before the one from Plugin2. So if Plugin1 does something that is needed by Plugin2 during that step it won't work properly.

Yes, this is certainly true. The question is: how often will this be a problem? Since it will be a problem at least sometimes (even if it's an edge-case), it makes sense to add a solution for it (and most people won't need this solution anyways, so it's not something they will even need to know about).

but I'm not sure that (prependPlugin) would be flexible enough... what if a plugin needs to be added between two of the plugins added by Encore?

Very true again :)

I have 2 counter-proposals:

A) Add a copyFiles() method to help alleviate this problem

and

B) Internally, give each plugin a "priority" number - e.g. 10, 20, -10, 20. Then, add a 2nd argument to addPlugin() which is an optional priority number. This would allow you to "place" a plugin between 2 other plugins, if you care to.

Wdyt? This PR is perfect... but it's trying to work around an inflexibility in Encore itself... so it's not super user friendly. Maybe (B) is a bit nicer?

[Lyrkan]

A) Add a copyFiles() method to help alleviate this problem

We should probably add it anyway but that'll not be enough if other plugins have the same kind of issue (e.g. ConcatPlugin).

B) Internally, give each plugin a "priority" number - e.g. 10, 20, -10, 20. Then, add a 2nd argument to addPlugin() which is an optional priority number.

I thought about that before starting to work on this PR. My main concern was that it wouldn't be easy enough to use/understand either:

// We would have to add constants so we can still change priorities
// if needed later on without introducing breaking changes... but
// that'll only work if the users use them.
const PluginPriorities = require('@symfony/webpack-encore/lib/plugin-priorities');

Encore
  .addPlugin(new Plugin1())

  // Since we'll have the same priority than Plugin1 here (default),
  // are we sure that it won't end-up being added before it when sorting
  // all the plugins? AFAIK JavaScript sort is not guaranteed to be stable.
  .addPlugin(new Plugin2())
  
  // Unless we provide constants like "PluginPriorities.BeforeManifestPlugin"
  // or "PluginPriorities.AfterUglifyJsPlugin" the user will have to do it manually
  .addPlugin(new Plugin3(), PluginPriorities.ManifestPlugin - 1)
  .addPlugin(new Plugin4(), PluginPriorities.UglifyJsPlugin + 1)
;

[weaverryan]

A) Let's plan to add the copyFiles() method then. See #175

About the priority stuff, I'm happy you were also thinking about it :)

We would have to add constants so we can still change priorities
if needed later on without introducing breaking changes... but
that'll only work if the users use them.

It would still be a BC break to change the orders, even if we use the constants. This is exactly how Symfony does it with event listeners. It's a bit odd to choose random integers for priority. But, in practice, it works fine. I would expect to start with everything as a 0 priority, except (unless I'm missing something) the plugin in the future copyFiles() - it would maybe have a priority of -100.

Since we'll have the same priority than Plugin1 here (default),
are we sure that it won't end-up being added before it when sorting
all the plugins? AFAIK JavaScript sort is not guaranteed to be stable.

This is an implementation detail... but yea... we would need to make sure the sort is maintained when the priorities are equal. We just need to figure that out :)

I was picturing something like this:

Encore
  .addPlugin(new Plugin1())
  .addPlugin(new Plugin2())
  
  .addPlugin(new Plugin3(), -50)
  .addPlugin(new Plugin4(), 50)
;

Internally, if we had a file where all the plugin priorities were set to constants (even if most were 0), this would be a cool way of documenting them. In the future, if we realize that people need to be able to add plugins between two core plugins that have a 0 priority, we'll need to break BC (in a minor way) by giving those existing plugins different priority. We're not even sure if this is going to happen, so that's why I like this simple solution.

[Lyrkan]

If you don't mind the BC breaks I'm ok with that solution too 👍

Closing this PR!