Secondary options shouldn't be merged when an extending config overwrites a rules from within a extended config #1646

Closed
niksy opened this Issue Jul 14, 2016 · 8 comments

Projects

None yet

4 participants

@niksy
niksy commented Jul 14, 2016 edited

Description

Bug where merging user provided array of exceptions is ignored. Tested on rule at-rule-empty-line-before but probably relates to other ones with similar options.

How to reproduce

index.scss

.foo {

    @media screen and (max-width:42px) {

        @include bar('bar');
        @include baz('baz');
        @include bad('bad');
    }
}

config.json

{
    "rules": {
        "at-rule-empty-line-before": ["always", {
            "except": ["blockless-group", "all-nested"]
        }]
    }
}

.stylelintrc

e.g.

{
    "extends": [
        "./config.json"
    ],
    "rules": {
        "at-rule-empty-line-before": ["always", {
            "except": ["blockless-group"]
        }]
    }
}

CLI output when running stylelint index.scss --syntax scss:

index.scss
 3:2  ✖  Unexpected empty line before at-rule   at-rule-empty-line-before
 5:3  ✖  Unexpected empty line before at-rule   at-rule-empty-line-before

Expected output should be that all is valid since user provided configuration ignore all nested rules (doesn’t provide it in it’s array).

I suppose all configuration options are merged, even arrays. Is this by design? I suppose it is because it makes it easier to fully extend user provided options.

@evilebottnawi
Member

@niksy Thanks for reporting.

@jeddy3
Member
jeddy3 commented Jul 14, 2016

Is this by design? I suppose it is because it makes it easier to fully extend user provided options.

Yes, exactly. Listing a rule in your extending config creates a clean-slate for that rule.

@jeddy3 jeddy3 changed the title from User provided array of options is ignored to Secondary options aren't merged when an extending config overwrites a rules from within a extended config Jul 14, 2016
@niksy
niksy commented Jul 14, 2016

Yes, exactly. Listing a rule in your extending config creates a clean-slate for that rule.

Hm, yes, but that doesn’t seem to be the case. It seems like array of except options is merged instead of being overwritten.

@jeddy3
Member
jeddy3 commented Jul 14, 2016 edited

Hm, yes, but that doesn’t seem to be the case.

Ooo, I see what you mean now. I'm pretty sure it worked that way before 7.0.0. This change in behaviour is probably a side-effect of supporting plugins in extended configs. It's a good job it happened as a breaking change in 7.0.0 huh? :)

Anyhow back to the issue at hand. There seems to be pros and cons to each approach i.e. clean slate vs merging secondary options. As changing the behaviour back to the "clean slate" approach will be a breaking change, I suspect the behaviour will stay as "merging secondary options" going forward. I'm also in favour of the "merging" approach as I ran into the short comings of the "clean slate" one while setting up a recent project. @stylelint/core Anyone has a strong opinion on this?

@jeddy3 jeddy3 changed the title from Secondary options aren't merged when an extending config overwrites a rules from within a extended config to Secondary options are merged when an extending config overwrites a rules from within a extended config Jul 14, 2016
@niksy
niksy commented Jul 14, 2016

Huh, yes, it seems like I’m just submitting issues related to edge cases in configurations :) Merging options seems like obvious change given plugins and extending config issue.

@jeddy3
Member
jeddy3 commented Jul 14, 2016 edited

Huh, yes, it seems like I’m just submitting issues related to edge cases in configurations

It's good. It's better we catch this now :)

I've just realised that merging secondary options is not a viable approach as it's now not possible to turn off a secondary option e.g.

// config1.js
rules: {
  "at-rule-no-unknown": [ true, { ignoreAtRules: ["extends"] } ]
}

Say I want to extend the above config, but not ignore @extends and, instead, just ignore @include e.g.

// config2.js
extends: "config1.js",
rules: {
  "at-rule-no-unknown": [ true, { ignoreAtRules: ["include"] } ]
}

This won't work anymore as the @extends will be merged in from the extended config, and there is no way to unignore it.

@davidtheclark I suspect @niksy is the only one to have discovered this problem. What do you think about classifying this as a broken release and reverting this behaviour back to the one in 6.x?

@davidtheclark
Contributor

I don't think that the merging of plugins relates to this. The line about merging plugins is here https://github.com/stylelint/stylelint/blob/master/src/buildConfig.js#L168 --- and it is very plugin-specific. It should not have affected the way that rules/options are merged. We need to debug this problem independently, I think. I will try to look into it later tonight if nobody gets to it first.

Also, I prefer the clean slate approach that we've had & intend to have.

@jeddy3
Member
jeddy3 commented Jul 14, 2016

The line about merging plugins is here https://github.com/stylelint/stylelint/blob/master/src/buildConfig.js#L168 --- and it is very plugin-specific

Agreed. Something else is amiss here.

Also, I prefer the clean slate approach that we've had & intend to have.

Agreed also.

@jeddy3 jeddy3 changed the title from Secondary options are merged when an extending config overwrites a rules from within a extended config to Secondary options shouldn't be merged when an extending config overwrites a rules from within a extended config Jul 14, 2016
@jeddy3 jeddy3 closed this in 5b8c311 Jul 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment