postcss / postcss-custom-properties

Use Custom Properties in CSS
https://postcss.github.io/postcss-custom-properties
MIT License
596 stars 77 forks source link

Apply warnings configuration to all warnings #44

Closed TheSpyder closed 8 years ago

TheSpyder commented 8 years ago

Creating a new result object and overriding the warn function seemed easier than passing the warnings configuration setting into the resolveValue function :)

I can do it that way if you prefer, though.

TheSpyder commented 8 years ago

hmm. The Object.assign method was only added in node v4. How do you want me to fix that? Write an equivalent that works in older versions, or use a different technique to turn off warnings?

MoOx commented 8 years ago

Why would you want to hide all warnings? This seems insane and dangerous. I don't like the idea about allowing people to do this dangerous think without thinking.

TheSpyder commented 8 years ago

It already has a warnings configuration, but there is a note in the readme that it doesn't work for everything:

Allows you to enable/disable warnings. If true, will enable all warnings. For now, it only allow to disable messages about custom properties definition not scoped in a :root selector.

I thought that implied the intention was to allow hiding all warnings, it just hadn't been implemented yet.

All I specifically want to do is hide the undefined variable warning. I can switch to having individual configuration for each warning instead if that works for you? But then what should happen if a blanket warnings: false is passed in?

MoOx commented 8 years ago

Why do you want to allow undefined vars in the first place?

TheSpyder commented 8 years ago

I'm creating a tool to allow theming a complex CSS file compiled from multiple input files. I have a core set of variables I want to allow to be changed, and there are variables spread through the CSS that reference them. These are the ones used in my UI.

For example, once all the imports are resolved it looks a bit like this:

:root {
  --base: rgb(0,0,0)
}

/* further down the file */

:root {
  --menu-text: var(--base);
}

div.menu: {
  color: var(--menu-text);
}

I compile it once normally with my default theme, warnings on. I then compile a themeable.css with the "base" variables missing, which creates a bunch of warnings but produces exactly what I want:

div.menu: {
  color: var(--base);
}

I then give my users a tool that lets them specify the values for the base variables. It compiles themeable.css using those values into a new stylesheet, thus matching my default but with the colours changed.

I would prefer not to create themeable.css without any variable substitution at all.

MoOx commented 8 years ago

Since it's a very specific use case, can't you just hide those warnings by filtering stdout?

TheSpyder commented 8 years ago

I'm not very familiar with postcss plugins, and since the documented ability for this plugin to hide warnings wasn't working I figured this was the best place to fix the problem.

Right now I have to disable failing on a postcss error which is a terrible place to be :)

MoOx commented 8 years ago

Warnings are just "warnings" so this should not prevent anything from being run and built. Those warnings should not block anything.

TheSpyder commented 8 years ago

hmm, maybe that's an issue with grunt-postcss then. Right now they are blocking. I'll look into it :)

As far as this PR goes, then, do you not want the ability to disable warnings as the readme implies? In that case we can abandon it.

MoOx commented 8 years ago

I don't think it's a good idea to allow people to hide this warnings since it's dangerous. You are the first one in years to ask for this. I understand your use case but I think it's a better idea to let this with no options to hide.

TheSpyder commented 8 years ago

So should the warnings configuration option be removed completely then?

MoOx commented 8 years ago

We should probably don't do anything and close this pr.