fix: use onceexit (instead of once) which runs after all children are processed

[charlessuh]

See: https://postcss.org/api/#plugin-onceexit

This gives other postcss plugins (like postcss-each) a chance to run first and modify the AST.

[charlessuh]

@alexander-akait: Any thoughts?

[alexander-akait]

Sorry, in this case we will break other scenarios, it is breaking change, css modules should be always last

[charlessuh]

Can you describe these other scenarios that would break?

---

Just to elaborate on the motivation for this change: there are plugins like postcss-each (see https://github.com/madyankin/postcss-each#readme) which can generate the CSS selector name dynamically:

@each $icon in foo, bar, baz {
  .icon-$(icon) {
    background: url('icons/$(icon).png');
  }
}

into:

.icon-foo {
  background: url('icons/foo.png');
}

.icon-bar {
  background: url('icons/bar.png');
}

.icon-baz {
  background: url('icons/baz.png');
}

Because this plugin currently runs first, not last, it parses and exports the selector name .icon-$(icon), before postcss-each had a chance to transform it into .icon-foo, .icon-bar, and .icon-each.

[alexander-akait]

postcss-each should not be run before css modules and I think it is already works https://github.com/madyankin/postcss-each/blob/master/index.js#L100

[charlessuh]

postcss-each should not be run before css modules and I think it is already works https://github.com/madyankin/postcss-each/blob/master/index.js#L100

That's only if beforeEach or afterEach is configured. If you look a few lines below, you can see by default the main hook runs on the AtRule instead of Once:

https://github.com/madyankin/postcss-each/blob/7a457dfabd8cf578ee963c0a0552af331a0e48a4/index.js#L109-L111

This is what postcss 8 recommends plugins do if possible, instead of running in Once:

  module.exports = {
    postcssPlugin: 'postcss-dark-theme-class',
-   Once (root) {
-     root.walkAtRules(atRule => {
-       // Slow
-     })
-   }
+   AtRule (atRule) {
+     // Faster
+   }
  }
  module.exports.postcss = true

[alexander-akait]

That's only if beforeEach or afterEach is configured. If you look a few lines below, you can see by default the main hook runs on the AtRule instead of Once:

But it should work too, there are a lot of other plugins and css-loader rely on our behavior, I am really can't change it, it will be big breaching change and require rewrite many things

[charlessuh]

But it should work too

No, Once runs BEFORE children (like AtRule) are processed while OnceExit runs AFTER children are processed.

My 2 cents is that while this could potentially be a breaking change, this should have been part of the 3.0 release to properly support newer postcss 8.x plugins which have been migrated away from running in the Once hook and as child hooks, as recommended by postcss 8's migration guide.

Out of curiosity, I checked out css-loader with this change and it seems like all the tests still pass.

[charlessuh]

For example, if you do postcss([ require('postcss-each'), require('postcss-modules-scope')]), if both plugins are using the Once handler, the order is this:

Once: postcss-each
Once: postcss-modules-scope

But since postcss-each uses a child hook (AtRule), which is what postcss recommends you do for new and migrated plugins, the order is the following:

Once: postcss-modules-scope
AtRule: postcss-each

The more appropriate handler for all plugins that need to walk the entire tree once like this plugin does, would be to use OnceExit beginning with postcss 8:

AtRule: postcss-each
OnceExit: postcss-modules-scope

[alexander-akait]

In this case cssnano will be broken, because they use OnceExit, so if you have cssnano before css modules plugins you will not get minified output, that is why it is breaking change

[charlessuh]

Agreed it would be a breaking change for someone who is relying on this implementation detail. However, there is a simple fix available in this case -- just switch the plugin order to make it explicit you want either postcss-module-scope or cssnano to run first.

For someone who wants to use postss-each (or any plugin that doesn't run on Once, which again is what postcss 8 recommends), what would be a solution here? Changing the plugin order wouldn't affect it.

[charlessuh]

@ai: If you have any thoughts here, it would be appreciated.

[ai]

Changing Once to OnceExit is a hack. They both do no fix the problem with plugin compatibility.

It is better to rewrite plugin to Rule.

[charlessuh]

@ai: Let's say this plugin does use Rule to collect classnames. If some code needs to run once all the Rules have been run, i.e. once all the classnames have been collected, what would be the hook to use (if not OnceExit)?

[ai]

Why we can’t change rule.selector in Rule or RuleExit?

[alexander-akait]

@ai Because we need collect all of them, I have talked about this potential issue earlier, the simplest solution - adding stage/priority to visitors, but it will be the same hack, but it will allow some things to be made more flexible. There is actually no correct solution here, the order matters.

[ai]

Why do you need to collect all selectors before changing any selector?

[alexander-akait]

@ai For collection purposes, we need collect all composes and then try to resolve them for selectors, we can't do two things in one time using Rule, we can do this if we will have two Rule and each will be used after the previous one

[ai]

Why we can't do it?

[alexander-akait]

Can you show how I can do it? We need collect all composes from declaration values and them collect all selectors and rewrite them

[ai]

1. You can change rule’s selectors in Rule.
2. You can replace composes if you already know a selector in Rule too
3. If you do not know the selector you can scan AST manually inside Rule

[alexander-akait]

It is not working, because we need collect all composes values firstly and then replace all selectors in Rule, also we can't mutate selectors in Rule, because other plugins can rely on original name, i.e. custom selectors will be broken