Allow to add custom loaders #11

pierredup · 2017-06-14T07:30:10Z

Currently Encore only supports specific loaders, and adding additional loaders is not very intuitive and adds additional (ugly) code to the config, E.G we are using handlebars with the handlebars-loader and have to manually add the loader to the config:

var webpackConfig = Encore.getWebpackConfig();
webpackConfig.module.rules.push({ test: /\.hbs$/, loader: 'handlebars-loader' });

So I propose allowing to add custom loaders through a simple addLoader(test, loader) function.
This changes the above code to the following:

// ...
    .addLoader(/\.hbs$/, 'handlebars-loader')

It is also possible to set custom config for the loaders by passing an object instead of a string:

// ...
    .addLoader(/\.hbs$/, { 'loader': 'handlebars-loader', 'options' : { 'debug': !Encore.isProduction()} })

I have also added an object to define include and exclude directories. There are just so many options for a loader that I'm not sure which ones to support. Include and exclude seemed like the most obvious. Instead of having too many parameters for the function, I opted for an object with the options as keys, which makes it easier to add support for more options, but the drawback is that it is less clear which options are supported.

stof · 2017-06-14T08:11:53Z

lib/WebpackConfig.js

@@ -49,6 +49,7 @@ class WebpackConfig {
        this.providedVariables = {};
        this.babelConfigurationCallback = function() {};
        this.useReact = false;
+        this.loaders = new Set();


This does not need to be a Set. It could just be an array.

You create a new object inside addLoader, so the uniqueness check of the Set will never exclude anything anyway. You're just using it as an array here

fvsch · 2017-06-14T09:49:05Z

What is the advantage of this API:

.addLoader(/\.hbs$/, 'handlebars-loader')

over:

.addLoader({ test: /\.hbs$/, loader: 'handlebars-loader' })

Is the slightly shorter API worth the complexity of "okay, I need to pass more options, so I need to change the second parameter to an object and the original string becomes the loader key in that object"?

It might be preferable to not introduce a magic difference over webpack's API here.

PS: speaking as a passerby who's interested in Encore, not a project member.

stof · 2017-06-14T10:04:10Z

yeah, I think we should either use addLoader(loaderRule) (the second snippet from @fvsch) or addLoader(test, loader, options = {}) rather than making the second argument change its structure depending on whether you want options or no

stof · 2017-06-14T10:04:58Z

lib/config-generator.js

@@ -235,6 +235,12 @@ class ConfigGenerator {
            });
        }

+        if (this.webpackConfig.loaders.size > 0) {


no need for this check. .forEach works fine on empty arrays too

pierredup · 2017-06-14T10:20:06Z

Having to pass the full object means the name can then just as well change to addRule, since it's not about adding a loader anymore, but just adding a custom rule object.

Having to construct the object manually (like the second example) doesn't feel clean though.
What about adding a Rule class where you can specify the options and config in a fluent interface? It will make it cleaner to use as opposed to constructing the full object?

// ...
    .addRule( // or addLoader
        Encore.Rule()
            .setTest( /\.hbs$/)
            .addLoader('handlebars-loader')
            .addOption('debug', !Encore.isProduction())
            .addExclude('node_modules')
    )

The benefit here is that we can then easily add support for all options for a rule, it reads easier and it fits in with the style of the rest of the config. WDYT?

weaverryan · 2017-06-15T01:15:14Z

First, woohoo! This is one of the first PR's I was hoping for - it's an obvious extension point that's needed :).

I would definitely to stick close to webpack here. So, I prefer @fvsch's suggestion:

.addLoader({ test: /\.hbs$/, loader: 'handlebars-loader' })

But @pierredup is totally correct that then... this is nothing more than a rule. But, as far as I can tell, rules is only used for adding loaders. loaders was renamed to rules from Webpack 1 to Webpack 2. In other words... as far as I can tell, they are the same thing... but with a different name.

Assuming that's true, right now, "loaders" is more common language, so I would still want it to be called addLoader(). We could also add an alias method called addRule().

pierredup · 2017-06-15T07:01:14Z

Thanks for the info regarding rules and loaders @weaverryan. I've never used Webpack 1, and only recently started using Webpack so some things are still a bit confusing.

I've made the changes as requested, but I still feel that it would be more difficult to use if there isn't a cleaner interface to define a loader config. By passing the raw loader config, it takes away the 'clean & powerful API' that Encore provides. It means you need to know exactly how the Webpack config should look like in order to construct the object.
If you need to use raw Webpack config, then TBH I don't fully understand what problem this package is then trying to solve, as you can then just as well manually create the normal Webconfig config instead of using the Encore api.

It also might make it more difficult for newcomers that don't know Webpack to get started with custom loaders. The handlebars-loader for example just states "The following query (or config) options are supported" with a list of the options, and no indication of how to use those config options. So you then need to go back to the Webpack documentation to try and understand how you should pass a config value. Instead you can have a cleaner way to add loaders with the Symfony docs telling you "to add a custom loader, just add your file extension and the the loader you want to use to this function", or "construct this object with this options to create a loader config" (Note: I'm talking about a custom rule or loader class here, not the initial way of passing an object that I described in the PR description, which wasn't actually an implemented feature but just a side-effect of how the loader was stored)

weaverryan · 2017-06-15T07:57:49Z

By passing the raw loader config, it takes away the 'clean & powerful API' that Encore provides.

This is a fair point - passing the entire configuration object doesn't have a "smoothness" to it. But, speaking about the handlebars example, it does lower the "translation" between reading their docs and seeing the raw webpack.config.js example, and figuring out how to do that in Encore.

The good news is, we could merge the PR as-is (i.e. with this simple/raw API)... and still add the Encore.rule idea later - it would certainly be easy to allow the addRule() argument to be a normal mixed object or a Rule object.

weaverryan

Very minor comments!

weaverryan · 2017-06-15T07:59:18Z

test/WebpackConfig.js

+
+            config.addLoader({ 'test': /\.custom$/, 'loader': 'custom-loader' });
+
+            expect(Array.from(config.loaders)).to.deep.equals([{ 'test': /\.custom$/, 'loader': 'custom-loader' }]);


We could remove the Array.from now (because it is an Array), right?

Thanks, fixed

weaverryan · 2017-06-15T08:00:54Z

lib/WebpackConfig.js

+    addLoader(loader) {
+        this.loaders.push(loader);
+    }
+


We need to expose this in index.js as well (that file/module holds the true public API). And I think we should (in that file) create addLoader() and also addRule() (which would just call addLoader())

Thanks, don't know how I missed this :) Functions added

weaverryan · 2017-06-15T08:37:27Z

👍

weaverryan · 2017-06-16T16:00:08Z

Thank you @pierredup! I've added a docs issue for this new method: symfony/symfony-docs#8053

Allow to add custom loaders

b21f7aa

stof requested changes Jun 14, 2017

View reviewed changes

stof reviewed Jun 14, 2017

View reviewed changes

pierredup added 2 commits June 15, 2017 08:25

Update addLoader to take a raw loader config object

e078772

Update test description

0ea9d74

weaverryan reviewed Jun 15, 2017

View reviewed changes

pierredup added 2 commits June 15, 2017 10:06

Add addLoader and addRule to public api

539fd11

Remove Array.from call

d6a1bc0

weaverryan approved these changes Jun 15, 2017

View reviewed changes

alOneh approved these changes Jun 16, 2017

View reviewed changes

weaverryan merged commit 1a3300e into symfony:master Jun 16, 2017

weaverryan mentioned this pull request Jun 16, 2017

[Frontend] Encore addLoader method symfony/symfony-docs#8053

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to add custom loaders #11

Allow to add custom loaders #11

pierredup commented Jun 14, 2017 •

edited

stof Jun 14, 2017

fvsch commented Jun 14, 2017

stof commented Jun 14, 2017

stof Jun 14, 2017

pierredup commented Jun 14, 2017 •

edited

weaverryan commented Jun 15, 2017

pierredup commented Jun 15, 2017

weaverryan commented Jun 15, 2017

weaverryan left a comment

weaverryan Jun 15, 2017

pierredup Jun 15, 2017

weaverryan Jun 15, 2017

pierredup Jun 15, 2017

weaverryan commented Jun 15, 2017

weaverryan commented Jun 16, 2017


		config.addLoader({ 'test': /\.custom$/, 'loader': 'custom-loader' });

		expect(Array.from(config.loaders)).to.deep.equals([{ 'test': /\.custom$/, 'loader': 'custom-loader' }]);

Allow to add custom loaders #11

Allow to add custom loaders #11

Conversation

pierredup commented Jun 14, 2017 • edited

stof Jun 14, 2017

Choose a reason for hiding this comment

fvsch commented Jun 14, 2017

stof commented Jun 14, 2017

stof Jun 14, 2017

Choose a reason for hiding this comment

pierredup commented Jun 14, 2017 • edited

weaverryan commented Jun 15, 2017

pierredup commented Jun 15, 2017

weaverryan commented Jun 15, 2017

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan Jun 15, 2017

Choose a reason for hiding this comment

pierredup Jun 15, 2017

Choose a reason for hiding this comment

weaverryan Jun 15, 2017

Choose a reason for hiding this comment

pierredup Jun 15, 2017

Choose a reason for hiding this comment

weaverryan commented Jun 15, 2017

weaverryan commented Jun 16, 2017

pierredup commented Jun 14, 2017 •

edited

pierredup commented Jun 14, 2017 •

edited