Changed AtRule subscription

[pciarach]

Subscribed to the general AtRule event instead of specific ones to preserve the order of plugins when visiting nodes.

[ai]

It will slow down the plugin, because object structure is faster (callback will be called only on specific at-rule, not for every at-rule).

[pciarach]

I've run (a poor man's) benchmark on a simple scenario that includes different AtRule but to be honest I haven't seen any noticeable performance degradation.

  const times = [];

  for (let i = 0; i < 1000000; i++) {
    const before = performance.now();
    await run(
      '@mixin a { @supports (max(0px)) { color: black; } }',
      '.a { @supports (max(0px)) { color: black; } }',
      {
        mixins: {
          a() { return { '.a': { '@mixin-content' : {} } } }
        }
      })
    const after = performance.now();
    times.push(after - before);
  }

Old:

Total: 75515.94069999667
Mean: 0.07551594069999668
Standard deviation: 0.025997863098952936

New:

Total: 76030.33139999444
Mean: 0.07603033139999443
Standard deviation: 0.026244211710887977

[ai]

You need to do the benchmark on big CSS with many different at-rules (@apply, @media, etc).

If your benchmark there is only one at-rule, so there is no useless callback calls.

[ai]

Is it the same AtRule event? Why not AtRuleExit?

AtRuleExit will be called after all plugin will process this node (so, for instance, postcss-simple-vars will add variable.

We may need AtRuleExit only for mixin/add-mixin, not for define-mixin.

[pciarach]

But the issue is that postcss-mixins is executed after postcss-simple-vars right now. And the same will keep happening if we subscribe to AtRuleExit I think 🤔

[ai]

But the issue is that postcss-mixins is executed after postcss-simple-vars right now.

But postcss-mixins should be called after postcss-simple-vars. Otherwise, other users will not be able to use vars as mixin argument.

You have not a problem with PostCSS 8, but a problem of using the hack to pass argument with , to mixin. PostCSS 8 just broke the hack.

[pciarach]

Otherwise, other users will not be able to use vars as mixin argument.

I think it will not change anything in this matter.
Let's look at this example:
I assume this order of plugins in config: [mixins, vars]

@define-mixin a $b, $c { color: $b; margin: $c; }

.selector { $margin: 4px; @mixin a black, $margin; }

And because simple-vars performs replacements first right now this is what mixins plugin 'sees':

@define-mixin a $b, $c { color: $b; margin: $c; }

.selector { @mixin a black, 4px; }

which is then transformed into:

.selector { color: black; margin: 4px; }

After my changes the order from config will be preserved so mixins plugin will transform the code first ($margin definition would not be present in nodes at this point I think):

.selector { $margin: 4px; color: black; margin: $margin; }

which will be again transformed into:

.selector { color: black; margin: 4px; }

So as you can see the typical usage will not change at all. Or maybe you have a specific scenario in mind? If so, could you give some more details?

You have not a problem with PostCSS 8, but a problem of using the hack to pass argument with , to mixin.

I agree that this is a hack but on the other hand, I also think that there should be some kind of way to pass such parameters to mixins because they are 100% valid CSS values.
What would you say about introducing a new syntax, e.g. @mixin [a, b], c to do the same kind of transformation?

[ai]

I assume this order of plugins in config: [mixins, vars]

There is no order in PostCSS 8. In PostCSS 8 all plugins works together in the same moment by events.

What would you say about introducing a new syntax, e.g. @mixin [a, b], c to do the same kind of transformation?

We can add @mixin (a, b), c

But are we on right direction of simple sources, which is easy to read? Previously, you need to know the plugins order to read styles. Now you need to know tricky syntax.

[pciarach]

I don't think that adding square brackets would significantly complicate the use of this plugin. However, I would be rather opposed to using parentheses because they might be too closely associated with function calls. In this case, we are definitely closer to a list of values.

Furthermore, I personally perceive this change as a nice addition. Nobody will be forced to use it, but it can be extremely helpful in specific situations.

As for code readability, in my opinion, it mainly depends on the user and the naming of mixins/parameters and brackets would neither improve nor obscure readability.

Mixins themselves are non-standard CSS anyway and it gives a bit more freedom when it comes to introducing new things - without overdoing of course 😉

[ai]

Will another developer understand the meaning of [] without going deeply to postcss-mixins docs?

What about unquote("one, two")?

[pciarach]

It's a hard question to answer without asking a bigger group of developers 😉

But I really like the function-like approach. I would suggest asSingleArg(first, second) though - after all, it would do exactly that so it should be self-explanatory.
If we agree on that I'll be happy to rewrite this PR.

[ai]

OK, let’s try asSingleArg()

[ai]

Strange. Does this test fail if you do not change the index.js?

[pciarach]

No, it passes in both cases. But I wanted to make sure that such a scenario is covered by tests and protected from refactoring.

[ai]

I am closing this PR, it is better to open new one with new syntax