Refactor plugin configuration

[davidmpaz]

Extract encore built in plugin configuration logic into plugin utils.

First step toward allowing to be able to configure plugins through options and have more modular integration for plugins inside encore. From now on only public API with proper options is needed,
for most cases.

This changes are internals and tries to improve plugin architecture inside encore. Please take a look on it and feedback are welcome :). So far I see improvements for several issues:

• Add BrowserSync support #2 - Point .2 ...for plugins we should do the same as for loaders, from point of view of module split (utilities).
• Add support for separate type checking in Type Script #63 - Better (more modular) integration of the plugin as well its configuration.
• Add HappyPack support #65 - Same as Add support for separate type checking in Type Script #63
• Make UglifyJsPlugin optional/configurable #79 - Now it should be possible to pass options and/or check for flags to apply plugin or not.
• Make clean-webpack-plugin configurable #87 - Some small steps to make clean plugin more configurable. Public API still need to be changed though.

thanks in advance.

[weaverryan]

I'll check this out tonight! Excited about this!

[weaverryan]

Ok, just checked this out! Breaking things into smaller pieces is a win :). To be honest, I'm struggling on a few of the details about "how best" to do this... but since it's internal stuff, we shouldn't sweat it too much.

From now on only public API with proper options is needed, for most cases.

What do you mean by this?

About the individual plugins files, naming the method getPlugins() and allowing each to return an array of plugins looks weird to me (I realize you based it off of my getLoaders()... and it felt weird there too :) ). What if each plugin file exports just a function... and what if we pass plugins to that function and have it modify the array. So:

// clean.js
module.exports = function(plugins, webpackConfig, paths, cleanUpOptions = {}) {
    let config = Object.assign({}, cleanUpOptions, {
        root: webpackConfig.outputPath,
        verbose: false,
    });

    plugins.push(new CleanWebpackPlugin(paths, config));
};

config-generator.js would look like this:

if (this.webpackConfig.cleanupOutput) {
    cleanPluginUtil(plugins, this.webpackConfig, ['**/*']);
}

Actually, we could go a step further, and include the logic of whether or not a plugin should be enabled, inside of that file:

// clean.js
module.exports = function(plugins, webpackConfig, paths, cleanUpOptions = {}) {
    if (!this.webpackConfig.cleanupOutput) {
        return;
    }

    let config = // ...
    // rest of file
};

It still doesn't feel perfect, but maybe good enough for the next step :).

[weaverryan]

This isn't really a dev-server plugin... it's a "AssetOutputDisplayPlugin", which happens to be disabled when the dev-server is used.

[davidmpaz]

oops !! my bad. I'll update that.

[weaverryan]

This is clever, but a problem :). Because FriendlyErrorsPlugin.getPlugins() actually instantiates a new FriendlyErrorsWebpackPlugin internally, when FriendlyErrorsPlugin.getPlugins() is called by config-generator, it will receive one instance. When it's called here, it will receive a different instance. I "got away: with doing stuff like this with the loaders, because those return static config objects, not new instances of objects. In practice, this might not be a problem, but we can do better :).

The only idea I can think of right now, is to change plugins in config-generator to a hash - i.e. create keys for each plugin - e.g.

// old
plugins.push(new FriendlyErrorsPlugin());

// now
plugins.push('friendly_errors_plugin', new FriendlyErrorsPlugin());

then we could use that internal key to find other plugins. At the bottom of buildPluginsConfig() in config-generator, we would still return an actual array (not a hash/object). I don't love this magic, invented friendly_errors_plugin machine name... but it would work, and would still be an internal implementation detail, at least for now (i.e. not exposed to users in any way).

[davidmpaz]

I see your point. Is this issue present in general for all plugins or it is just this particular case with the FriendlyErrorsWebpackPlugin?

Regarding hashed solution:

... this magic, invented friendly_errors_plugin machine name...

I don't like it either. This case looks to me like a good scenario for a singleton. I did some research about the topic on the nodejs land. I did found some resources:

• post explaining approaches
• module implementing a solution
• another implementation approach

What do you think if we try to implement one of those solutions for that FriendlyErrorsPlugin?

I think this is the best way we can guaranty the single instance we need.

[weaverryan]

This case looks to me like a good scenario for a singleton

Yes.... but no :). We allow you to generate multiple webpack configs. And to be strict about it, each separate webpack config should have its own instances of plugins.

So, yes, we need to make sure that we don't create multiple FriendlyErrorsWebpackPlugin while generating one webpack config... but do need to create distinct instances, each time we ConfigGenerator.getWebpackConfig(). If you can take a shot at that, perfect :).

[davidmpaz]

To apply a solution fitting all cases known so far, I take into account:

• This is a problem applied only for FriendlyErrorsWebpackPlugin?
• We.... do need to create distinct instances, each time we ConfigGenerator.getWebpackConfig()
• We need to make sure that we don't create multiple FriendlyErrorsWebpackPlugin while generating one webpack config

From what we have talked I think the best solution is to not refactor this plugin at all :), we leave it as it was originally. That way we met all requirements.

Still, I am sorry to go over this but I would like to know what I am missing :)

FriendlyErrorsWebpackPlugin is configured currently without any options coming from user, we set up same config options, so every time we will be creating same object values. Different instances (memory references etc..) but behavior i believe is the same.

Having the previous in mind. In which way a singleton will be different to the original code when we call Encore.reset() to generate a new configuration?

thanks in advance for taking the time.

[weaverryan]

In which way a singleton will be different to the original code when we call Encore.reset() to generate a new configuration?

Honestly, in practice, it probably makes not difference for FriendlyErrorsWebpackPlugin. In fact, it's not clear to me what the proper behavior is for this plugin specifically when it comes to multi webpack config setups - there's an option question about it: geowarin/friendly-errors-webpack-plugin#56.

Could we just pass in the FriendlyErrorsPlugin instance as a dependency - i.e. add a friendlyErrorsWebpackPlugin argument to this plugin and pass the instance in from config-generator? It would work and no complexity or magic.

[davidmpaz]

To answer your first comment

Regarding:

From now on only public API with proper options is needed, for most cases.

I did mean that with this approach (broken into smaller pieces). Any future plugin (fork-ts-checker-webpack-plugin, browser-sync, etc..) can be easily integrated by creating another plugin module and adding respective public API. It was not a comment for internally used plugins, those are already integrated it ins owns way.

For:

What if each plugin file exports just a function... and what if we pass plugins to that function and have it modify the array.

I like this idea far more than current one, actually I was trying to get some result similar to an interface implementation, as long as all plugins integration follow same pattern i don't see any problem. To your suggestion though I would only change one thing, function signature:

module.exports = function(webpackConfig, paths, cleanUpOptions = {}) {

I think we don't need the plugin array any more because webpackConfig already contain plugins on its own, plugins that are added later on by:

this.webpackConfig.plugins.forEach(function(plugin) {
    plugins.push(plugin);
});

we could push to webpackConfig.plugins directly and get rid of that intermediate piece of code. Since plugin execution in webpack matters, we could ask as convention to users to add any custom plugin through addPlugin() api at the end of Encore calls.

What do you think?

[weaverryan]

I did mean that with this approach (broken into smaller pieces). Any future plugin (fork-ts-checker-webpack-plugin, browser-sync, etc..) can be easily integrated by creating another plugin module and adding respective public API

I understand!

I think we don't need the plugin array any more because webpackConfig already contain plugins on its own, plugins that are added later on by:

This sounds good... but so far, when we call ConfigGenerator.getWebpackConfig(), we never modify the WebpackConfig. I think we should continue to not modify this object: it's our input only.

Do we agree on enough to get this to a good spot? I feel pretty comfortable - it's internal stuff anyways, which we can continue to iterate on. I don't want to delay this PR too much more :).

Thanks!

[davidmpaz]

I feel also comfortable with the result. Thanks for the clarifications, did helped a lot.

[weaverryan]

Very nice! Thank you David!