Unable to add custom options for sass compilation

[d4r5c0d3]

When trying to use foundation I ran in to problem where I have to add custom include paths.
Adding them as options is not working as only resolve_url_loader is a valid option.

[weaverryan]

+1

This will be easy, we just need to find an API we like :).

The sass-loader has options (e.g. look for includePaths https://github.com/webpack-contrib/sass-loader#examples) and that's what needs to be exposed. Our one current option - resolve_url_loader - is something totally different (it's not passed to sass-loader, it's used internally to activate a different loader).

The tricky part is finding a good API for these 2 ideas:

1. We could mix all the options into one thing:

.enableSassLoader(function(loaderOptions) {
    // this is our special option
    loaderOptions.resolve_url_loader = true,

    // this is an option to be passed to sass-loader
    loaderOptions.includePaths = [...]
});

I've refactored this into a callback (instead of just passing a hash) because - so far- we're avoiding situations where we might need to "merge" user config and default config (see #15 (comment)).

2. Keep the options separate somehow? We could do this easily... but finding a nice API is more difficult.

.enableSassLoader(function(config) {
    // this is our special option
    config.disableResolveUrlLoader();

    // this is an option to be passed to sass-loader
    config.loaderOptions.includePaths = [...]
});

[d4r5c0d3]

How about making this function work with variadic arguments. Where we would have an object where we store the proxy config used by encore and a callback that gets the generated options passed as an argument. That way you could modify the generated options as needed.

Example I : Setting only config
When only setting config values you could write:

.enableSassLoader({ resolve_url_loader: false });

Example II : Setting only options
When only setting options for compiler/transpiler you could write:

.enableSassLoader(function(options) {
    // add or set custom options
    options.includePaths: ["absolute/path/a", "absolute/path/b"];
});

Example III : Setting both config and options
When wanting to do both you could write:

.enableSassLoader({ resolve_url_loader: false },function(options) {
    // add or set custom options
    options.includePaths: ["absolute/path/a", "absolute/path/b"];
});

Example IV : Setting both config and options alternatively
Because we know that we only use an object as config argument and callback for manipulating options it would be possible to change the argument order without any side effects

.enableSassLoader(function(options) {
    // add or set  custom options
    options.includePaths: ["absolute/path/a", "absolute/path/b"];
},{ resolve_url_loader: false });

[weaverryan]

@DaRamirezSoto Sorry for the delay - I was pushing through a few other things first :).

My vote is to combine the config. Basically, we would say something like:

supports all node-sass options (https://github.com/sass/node-sass) and also

• resolve_url_loader Set to false to...

I think this is pretty clear and I don't see any technical disadvantage to combining them. The solutions that keep them separated (which you laid out clearly) are really ugly. And to stay consistent with #49 and #50, I think we should use a callback. So.... it would be an option V ;)

.enableSassLoader(function(options) {
    // add or set  custom options
    options.includePaths: ["absolute/path/a", "absolute/path/b"];

    // set our custom option
    options.resolve_url_loader = false;
});

[d4r5c0d3]

@weaverryan I am really confused now.
I thought encore where going to make things easier and now we are just proxying Webpack with callbacks that have more switches and values you can set. I have no idea how you can call this easier.

There is going to be some confusing in regards to the file name used. People seeing webpack.config.js
but having to deal with some weird proxy API that has bad naming convention never seen before in Nodejs landscape. With childlike solutions that use everything in callbacks without a good reason to do so. It is not like we using promises at which point the heavy use of callbacks would make sense.

The solution change the file name to encore.config.js?

I understand that there is a need for a webpack.config.js generator that helps people who don't spend much time dealing with NodeJS (or don't want), but that is not what this is becoming. I already pointed this out and you validated but insisted that reinventing the wheel is what we were going for.

[davidmpaz]

Hi @DaRamirezSoto,

I thought encore where going to make things easier and now we are just proxying Webpack with callbacks

That is the part where you hit what an opinionated solution can/can not do. Encore has an opinionated solution regarding sass-loader configuration. It only accept resolve_url_loader. At the moment it is needed more flexibility, then your API will become more like a proxy, like you described.

Honestly I don't see a better way to deal with that, at least if you want to allow people to be able to configure all aspects of the loader freely (not so opinionated). You need to do it passing missing options, through an object, callback or variadic at the end, all of those are proxying.

A compromise should be made. In this case, callback can be useful for example when you want to configure also happypack to improve sass build time. If you do that through object config, you will need to have something like:

{
    'sassLoaderOptions': {},
    'happyPackOptions': {}
}

with callback would be something like:

Encore.enableSassLoader(function(sassConfig, happyPackConfig) {
    // change the sassConfig and happyPackConfig
});

I found more value on lastest since it is explicit as a callback parameter. For the object syntax one would need to know that configuration is split, being more error prone since most of the time you will end up always looking how is the format of that object you want to write for configure all things you want. Was it happyPackOptions or just happyOptions? Or was something else inside sassOptions?

Also regarding proxying configuration, recall it is not only about configuring webpack. Some other validations/checks are also done on the way.

hope it helps.

[d4r5c0d3]

Hi @davidmpaz
I understand what you mean with opinionated. That is why I proposed not using file name webconfig.config.js, because Encore != Webpack. It will confuse people.

To me, it made more sense to separate encore options from pass-through options used for the loaders.
This would make easier to understand what is available on the object to configure.
Now it feels more like a mess mixing up custom options/settings and loader options and what not.

Your comment made me realize that I find Webpack already pretty opinionated and maybe troubled about using/ setting up more opinions on top of it and losing, even more, flexibility and interoperability Therefore encore might be not the right tool for me. Thank you for your response.

[davidmpaz]

Therefore encore might be not the right tool for me.

Sad to hear that @DaRamirezSoto :( ...

Please notice that you can always opt out to not use some features. Later (after exporting configuration from encore) any setup can be added as normal webpack options. Currently that's is what I do right now since for me it is not a complete fit also.

[weaverryan]

To me, it made more sense to separate encore options from pass-through options used for the loaders.

Hmm @DaRamirezSoto, then what about an option that's very similar to your option III:

.enableSassLoader(function(options) {
    // add or set custom options
    options.includePaths: ["absolute/path/a", "absolute/path/b"];
}, { resolve_url_loader: false });

I've just reversed the first and second argument for consistency with other loaders, where we would consistently have an options callback as the first argument (and sometimes we would have a second argument for Encore-specific stuff when needed - so far, this is only for the sass-loader).

With childlike solutions that use everything in callbacks without a good reason to do so

Please try to keep things positive and respectful :). I do appreciate your help and thoughts, but I don't want anyone in the Symfony community (myself included) to feel insulted for their ideas.

@davidmpaz About your example with the happyPackConfig - I haven't looked into it too far, but HappyPack would likely have its own configuration/enabling method, since it's not really specific to any one loader (e.g. enableHappyPack(function(config) {});.

[d4r5c0d3]

@weaverryan

Hmm @DaRamirezSoto, then what about an option that's very similar to your option III:

yes; would be a better solution IMO.

Please try to keep things positive and respectful :). I do appreciate your help and thoughts, but I don't
want anyone in the Symfony community (myself included) to feel insulted for their ideas.

Was not aware I was being unrespectful or negative just a spicy discussion in a similar fashion as you did ( see below ) but of course, I agree I am all for positivity and respect.

The solutions that keep them separated (which you laid out clearly) are really ugly.

@davidmpaz

any setup can be added as normal Webpack options.

I tried this and did not like it. Mainly because configuration would get so fragmented so maintaining it would become harder. If you are working alone on project maybe not such a problem, but when working with a team not something you should do I think,

[weaverryan]

@DaRamirezSoto Ah, you're right! I realize my comment also wasn't respectful - my apologies :).

Unless someone beats me to it (which would be awesome), I'll open a PR soon for the version I mentioned above (#14 (comment)).

[weaverryan]

PR #76