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

Allow processing of @container at-rule, add tests for it #51

Merged

Conversation

vlad-elagin
Copy link

Hello!

The name of PR is self-explanatory. My team uses this plugin for a large project, recently we've been updating our UI and tried to use @container rules. And found that the plugin doesn't update variable values inside them.

Here is a quick fix (we'll be grateful for shipping it ASAP 🙏), but generally it would be better to allow user to define which at-rules he wants to be supported. There are a bunch of them.

For example:

// postcss.config.js 
postcss([
  require('postcss-modules-values-replace')({ atRules: ['media', 'container']  })
]);

// index.js in this repo
const defaultAtRules = ['media'];

const factory = ({
  atRules = defaultAtRules,
  // ...
  // ...
  AtRule: {
    ...atRules.reduce((acc, rule) => {
      acc[rule] = function(node) {
        // eslint-disable-next-line no-param-reassign
        node.params = replaceValueSymbols(node.params, definitions);
      }
    }, {}),
    value(node) {
      if (noEmitExports) {
        node.remove();
      }
    },
  },
  // ...

Cheers!

@princed
Copy link
Owner

princed commented Mar 5, 2024

Hi Vlad, thank you for the PR!

It looks good to me, apart from the fact it basically presents a breaking change. However, I do like your other suggestion, do you think you can actually implement it in this PR instead? I just don't have capacity for that.

@vlad-elagin
Copy link
Author

Sure. What test cases in your opinion should I cover along with that implementation?

@princed
Copy link
Owner

princed commented Mar 6, 2024

Thank you!

Sure. What test cases in your opinion should I cover along with that implementation?
A few combinations with different atRules settings, especially overriding the default.

Please make sure to update the README too!

@vlad-elagin vlad-elagin force-pushed the feature/container-at-rule-processing branch from 28f234a to 32a2b3a Compare March 6, 2024 13:58
@vlad-elagin
Copy link
Author

Done. Let me know if anything else should be done with this new logic

Copy link
Owner

@princed princed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfect, thank you!

@princed princed merged commit 7c5b077 into princed:master Mar 7, 2024
1 of 2 checks passed
@princed
Copy link
Owner

princed commented Mar 7, 2024

Published as 4.2.0

@princed
Copy link
Owner

princed commented Mar 7, 2024

Btw, is anyone using fs setting these days? I feel like it's not possible anymore with modern Webpack, is that true?

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.

None yet

2 participants