Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

charlessuh
Copy link

@charlessuh charlessuh commented Nov 17, 2021

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
Copy link
Author

@alexander-akait: Any thoughts?

@alexander-akait
Copy link
Member

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

@charlessuh
Copy link
Author

charlessuh commented Dec 6, 2021

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

charlessuh commented Dec 7, 2021

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

charlessuh commented Dec 8, 2021

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
Copy link
Author

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

@ai
Copy link

ai commented Dec 8, 2021

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
Copy link
Author

@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
Copy link

ai commented Dec 9, 2021

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

@alexander-akait
Copy link
Member

@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
Copy link

ai commented Dec 9, 2021

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

@alexander-akait
Copy link
Member

alexander-akait commented Dec 9, 2021

@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
Copy link

ai commented Dec 9, 2021

Why we can't do it?

@alexander-akait
Copy link
Member

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
Copy link

ai commented Dec 9, 2021

  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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants