Add a boolean parameter to Encore.disableCssExtraction()

[football2801]

Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	none
License	MIT
Allows for environment specific uses of disableCssExtraction that match conventions used in webpack.config.js by other methods such as enableSourceMaps or enableVersioning

[Lyrkan]

Hi @football2801,

Thank you for this PR, I renamed it since it isn't actually related to environment variables.

We thought a while ago about adding something like this but didn't because it could be a bit confusing for the users (see the responses to #564 (comment)).

If you look at the other methods with that kind of parameter you'll see that they are all named enable<something>(). For them having an "enable" parameter is not ambiguous at all: setting it to true will enable the related feature.

Now for a disable<something>() method it is a bit different, especially if it is between other enable<something>() methods in your webpack.config.js...

[football2801]

After reading everything in that comment that you tagged, @Lyrkan, I agree with the comments you were making. I don't find it the least bit confusing. I actually made this PR due to people that I work with that requested that this method have this optional param. I know it would make my config file that much prettier.

[Lyrkan]

Ping @Kocal & @weaverryan

[JShilling4]

I am in favor of this PR. I agree with @Lyrkan comment in #564. This PR makes total sense to me and keeps the config clean and consistent.

(do you want to disable CSS extraction?)
.disableCssExtraction(true) => yes, disable
.disableCssExtraction(false) => no, don't disable

Even placed with enables, this does not confuse me at all:

(do you want to enable source maps?)
.enableSourceMaps(true) => yes, enable
.enableSourceMaps(false) => no, don't enable

[Kocal]

Does it mean you enable the CSS extraction disabling? :D

[Kocal]

I'm not a big fan of .disable*() methods, especially when it take an argument which is named enabled and can lead to confusion (do we enable or disable the functionnality?).

IMO, the best option is to:

• rename disableCssExtraction() to enableCssExtraction
• it takes a new argument enabled, like other .enable*() methods
• it should be documented as a breaking change, and update symfony-docs if needed

[football2801]

@Kocal, enabling CSS extraction is the default behavior. The CSS extraction takes place without any webpack.config.js entries. This would not only be a breaking change for those who use the .disableCssExtraction() method, but wouldn't it also be a breaking change for everybody else too since their webpack.config.js file would not contain the new .enableCssExtraction()?

[Kocal]

Okay it makes sense, but I think we need to update the parameter enabled to disabled, see #756 (comment).

Encore.disableCssExtraction(disabled = true) to disable the feature, Encore.disableCssExtraction(disabled = false) to not disable the feature.

What do you think?

[Lyrkan]

Having an enableCssExtraction() that is mandatory for almost all projects wouldn't be a good thing in my opinion.

Encore.disableCssExtraction(disabled = true) to disable the feature,
Encore.disableCssExtraction(disabled = false) to not disable the feature.

What do you think?

I agree with that.

[football2801]

Encore.disableCssExtraction(disabled = true) to disable the feature, Encore.disableCssExtraction(disabled = false) to not disable the feature.

What do you think?

I can make this change. I see this as a valid change that may improve code readability and is not a breaking change.

[Kocal]

LGTM

[weaverryan]

Thanks @football2801! After initially not being sure about this, I've since wanted it in a few projects :)

[weaverryan]

I proposed a few extra docs in #867 :)