csstools / postcss-plugins

PostCSS Tools and Plugins
https://preset-env.cssdb.org/
MIT No Attribution
904 stars 72 forks source link

[postcss-nesting] Nested mixins declaration order #1297

Closed pciarach closed 8 months ago

pciarach commented 8 months ago

Bug description

Hello!

We have recently upgraded postcss and its plugins (including postcss-nesting) and noticed unexpected style orders in some places. After some digging and debugging, it turned out that the differences come from postcss-nesting and nested @mixin rules interaction implemented in the groupDeclarations function.

Our previous assumption was that all styles from mixins would be placed exactly where the mixin was used. However, this behavior has been affected by the new grouping and reordering mechanism.

I’ve attempted to address this issue in the forked repo, but before submitting a pull request, I’d like to confirm whether this behavior is intentional or unintended. If it’s intentional, any hints/details will be welcome 😃 Link to my changes: https://github.com/csstools/postcss-plugins/compare/main...pciarach:postcss-plugins:fix/nested-mixins-reorder

Source CSS

@define-mixin mixinToOverride {
    display: flex;
}

@define-mixin mixinWithDecl {
    color: blue;
}

@define-mixin nestedMixins {
    @mixin mixinWithDecl;
    @mixin mixinToOverride;

    &:disabled {
        color: white;
    }
}

a {
    color: yellow;

    & b {
        @mixin nestedMixins;
        display: inline-flex;
    }
}

Expected CSS

a {
    color: yellow;
}

a b {
    color: blue;
    display: flex;
    display: inline-flex;
}

a b:disabled {
    color: white;
}

Actual CSS

a {
    color: yellow;
}

a b {
    color: blue;
    display: inline-flex;
    display: flex;
}

a b:disabled {
    color: white;
}

Playgound example

No response

Does it happen with npx @csstools/csstools-cli <plugin-name> minimal-example.css?

None

Debug output

No response

Extra config

No response

What plugin are you experiencing this issue on?

PostCSS Nesting

Plugin version

12.0.3

What OS are you experiencing this on?

Windows

Node Version

20.11.0

Validations

Would you like to open a PR for this bug?

romainmenke commented 8 months ago

Hi @pciarach,

I think you are right that this is a bug. It's always fine to open a pull request by the way.

That makes it easy for us to pull the proposed changes locally and inspect them a bit more :)

pciarach commented 8 months ago

PR created 😉

1298

romainmenke commented 8 months ago

I've tried to figure out what is going on.

Input CSS :

a {
    color: yellow;

    & b {
        @mixin nestedMixins;
        display: inline-flex;
    }
 }

Mixins :

nestedMixins

@mixin mixinWithDecl;
@mixin mixinToOverride;

&:disabled {
  color: white;
}

mixinWithDecl

color: blue;

mixinToOverride

display: flex;

If we fill in all the mixins first:

first pass

a {
    color: yellow;

    & b {
        @mixin mixinWithDecl;
        @mixin mixinToOverride;

        &:disabled {
            color: white;
        }

        display: inline-flex;
    }
 }

second pass

a {
    color: yellow;

    & b {
        color: blue;
        display: flex;

        &:disabled {
            color: white;
        }

        display: inline-flex;
    }
 }

Resolve nesting :

a {
    color: yellow;
}

a b {
    color: blue;
    display: flex;
    display: inline-flex;
}

a b:disabled {
    color: white;
}

So from this perspective we expect blue before white and flex before inline-flex.


If we resolve nesting first :

a {
    color: yellow;

    & b {
        @mixin nestedMixins;
        display: inline-flex;
    }
}
a {
    color: yellow;
}

a b {
    @mixin nestedMixins;
    display: inline-flex;
}

Nothing nested remains, but we can fill in mixins :

a {
    color: yellow;
}

a b {
    @mixin mixinWithDecl;
    @mixin mixinToOverride;

    &:disabled {
        color: white;
    }

    display: inline-flex;
}

Now can further resolve nesting :

a {
    color: yellow;
}

a b {
    @mixin mixinWithDecl;
    @mixin mixinToOverride;

    display: inline-flex;
}

a b:disabled {
    color: white;
}

More mixins :

a {
    color: yellow;
}

a b {
    color: blue;
    display: flex;
    display: inline-flex;
}

a b:disabled {
    color: white;
}

So from this perspective we expect blue before white and flex before inline-flex. That is the same as before.

So I think that either way there is a bug if the outcome doesn't match this manual transform.

pciarach commented 8 months ago

Yes, this is exactly what was wrong with the transform - display: flex appeared after display: inline-flex.

But I do have one additional question to understand the overall idea of the groupDeclarations function which is responsible for this logic. I thought that the order of the plugins in the PostCSS configuration matters. But as we have the mixins plugin specified before the nesting one, the presence of this issue would mean that the order is not preserved (because the nesting plugin still needs to care about @mixin) Am I right? Was it introduced in one of the 'recent' (after 7.0.35 in our case) PostCSS changes?

romainmenke commented 8 months ago

I thought that the order of the plugins in the PostCSS configuration matters.

It does matter and it also doesn't. This changed with PostCSS 8.

PostCSS will re-visit nodes that have been altered. So the actual order that each node will be visited after being altered by any other plugin is quite complex.

romainmenke commented 8 months ago

This has been released : https://github.com/csstools/postcss-plugins/blob/main/plugins/postcss-nesting/CHANGELOG.md#1204

Thank you again 🙇