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

Config overrides function #4377

Closed
wants to merge 9 commits into from

Conversation

shawnbot
Copy link
Contributor

This implements the proposal in #4374. TL;DR: configOverrides can be a function that takes a fully resolved config and either modifies it in place and/or returns a new one:

const anymatch = require('anymatch')
module.exports = {
  extends: ['some-other-config'],
  configOverrides: config => {
    // modify, add, or remove plugins config.rules and config.plugins
  }
}

--rules CLI option

As a demonstration of how this works, I've also implemented a fix for #4331 with a new CLI flag: --rules <comma-separated-list> adds an config override that filters the configuration's rules object to include only the named rules in the list.

I haven't added docs for this yet, but I wanted to be sure that this change was welcome before doing so. I think it's super useful and would love not to have to maintain stylelint-only. 😁

File filters?

This might pave the way for a solution to #3128, but I'm honestly not sure where in stylelint's configuration "lifecycle" the configuration is augmented, and it's entirely possible that this happens before the config is applied to individual files. If so, a follow-up would need to be necessary to either:

  1. Apply configOverrides() for each file, and pass the file information to it; or
  2. Use this function under the hood to implement one of the declarative file override patterns proposed in Add overrides property to configuration object #3128.

Either way, this mechanism is only available in JS configs, and won't work in JSON or YAML.

@shawnbot
Copy link
Contributor Author

@hudochenkov I'd love your input on whether the --rules option proposed here is good to move forward before I add docs and open it up for review. ❤️

@shawnbot
Copy link
Contributor Author

Looks like I also might need some help on resolving some of the Flow errors, too 😬

@vankop vankop marked this pull request as ready for review October 24, 2019 20:57
@vankop
Copy link
Member

vankop commented Oct 24, 2019

@shawnbot Thanks for PR, looks good! I think we need to support "smart" merge, too. (maybe in another PR) Could you please create test case with .stylelintrc file? We need to support it as well.

@hudochenkov new problem with Windows in CI =( Maybe our previous hack (with commands chaining) is unnecessary now

UPD: Oh, it is only about Node.js api =( Won't this approach work for object as well?

@vankop
Copy link
Member

vankop commented Oct 24, 2019

  1. Apply configOverrides() for each file, and pass the file information to it

I think this is more useful

  1. Use this function under the hood to implement one of the declarative file override patterns proposed in Add overrides property to configuration object #3128.

I think this is not possible in current stylelint design, we can not rely on syntax when overriding config. As I can see we can use babel approach here. Something like:

{
    ...
   overrides: {
           include: "../components",
           syntax: "css-in-js",
           ...
   }
}

So under the hood we can create overrides function and call it for each file using first approach API

@shawnbot
Copy link
Contributor Author

@shawnbot Thanks for PR, looks good! I think we need to support "smart" merge, too. (maybe in another PR) Could you please create test case with .stylelintrc file? We need to support it as well.

Thanks, @vankop; can you point me toward where "smart" merge happens?

@vankop
Copy link
Member

vankop commented Oct 25, 2019

I was meaning to use mergeConfigs option https://github.com/stylelint/stylelint/blob/master/lib/augmentConfig.js#L244 , since this feature available only for Node.js API I think it will be enough to just provide this function in public API. Code usage:

const configSCSS = require('./config.scss');
const {mergeConfigs} = require('stylelint');
module.exports = {
  extends: ['some-other-config'],
  configOverrides: config => {
    config = mergeConfigs(config, configSCSS)
  }
}

@hudochenkov
Copy link
Member

Thank you for investigating possibilities.

We should discuss configOverrides proposal in #4374 first.

Proposed new flag is not discussed yet. #4331

@hudochenkov hudochenkov added the pr: blocked is blocked by another issue or pr label Oct 27, 2019
@shawnbot
Copy link
Contributor Author

Closing, as we've decided to go a different direction. ✌️

@shawnbot shawnbot closed this Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: blocked is blocked by another issue or pr
Development

Successfully merging this pull request may close these issues.

None yet

3 participants