Skip to content
This repository has been archived by the owner on Feb 18, 2022. It is now read-only.

Output preprocessed variables option #22

Closed
hgl opened this issue Mar 28, 2015 · 37 comments
Closed

Output preprocessed variables option #22

hgl opened this issue Mar 28, 2015 · 37 comments

Comments

@hgl
Copy link

hgl commented Mar 28, 2015

With this variables option:

{
  "--border-color": "#aaa",
  "--background-color": "color(var(--border-color) alpha(0.2))"
}

It would be great if cssnext could give back a variables option that have all values preprocessed:

{
  "--border-color": "#aaa",
  "--background-color": "rgba(170, 170, 170, 0.2)"
}

Otherwise it's very hard to consume the values for other js code.

I wonder what might be the best way to output this option? Should it be handled at cssnext level or this plugin here?

@hgl
Copy link
Author

hgl commented Mar 28, 2015

Also while testing this option, the -- variable name prefix doesn't seem to be optional as the readme says. Removing them cause cssnext to complain the variables being undefined and need a fallback.

@hgl
Copy link
Author

hgl commented Mar 28, 2015

After thinking about it again, I think the solution really shouldn't be preprocessing the passed variables option, but outputting the final computed custom properties, no matter if a variables option is passed or not.

In other words:

:root {
  color: #aaa;
}
body {
  color: var(--color);
}

Preprocessing this should output the final css along with the computed custom properties, if an option is passed:

$ cssnext --custom-properties props.json file

props.json

{
  "color": "#aaa"
}

Not sure if it's doable with postcss?

@MoOx
Copy link
Contributor

MoOx commented Mar 28, 2015

The plugin cannot return anything but just edit the CSS AST.
Maybe we can offer a mutable option but I think we should instead just compute JS variables like it's done for others properties.
PRs are welcome for this enhancement.

For the optional --, just check this commit that have a test in it 0691784

@hgl
Copy link
Author

hgl commented Mar 28, 2015

but I think we should instead just compute JS variables like it's done for others properties.

I'm sorry but not quite sure what do you mean by this. Could you elaborate a bit?

For the optional --, just check this commit that have a test in it 0691784

I was running an older version of this plugin. Now it's working. Thanks for the heads up.

@MoOx
Copy link
Contributor

MoOx commented Mar 28, 2015 via email

@hgl
Copy link
Author

hgl commented Mar 28, 2015

OK, do you mean we should resolve the passed in variables option directly without looking at the actual css source code?

This can fail if users do:

variables option

{ "border-color": "var(color)" }

css

:root {
  "color": "#aaa";
}
div {
  border: 1px solid var(--border-color);
}

Without the css source code, the passed variables option might be incomplete.

I think the mutable option is a viable approach. What if we modify the variables option in place?

var opts = {};
postcss()
  .use(customProperties(opts))
  .process(css);
opts.variables // computed values

and then cssnext could use this property to output the result.

I can starting working on a PR if you think this approach is good.

@MoOx
Copy link
Contributor

MoOx commented Mar 29, 2015

This plugin works with 2 passes on the complete AST. If we try to resolve JS variables between the first resolution and the variables replacement, I think we should be good. Just here

map[variable] = variables[variable]
. Should take a second.
I am not a fan of mutability :)

@hgl
Copy link
Author

hgl commented Mar 30, 2015

So after the JS variables are resolved, how do we output them?

Are you saying we let cssnext operate directly on json files?

variables.json

{ "--border-color": "var(--color)" }

style.css

:root {
  "--color": "#aaa";
}
div {
  border: 1px solid var(--border-color);
}
$ cssnext --compute-js-variables variables.json style.css

{ "--border-color": "#aaa" }

@MoOx
Copy link
Contributor

MoOx commented Mar 31, 2015

We are not in cssnext. This plugin will never handle file.

I just pushed a modification that should make you happy ;)
I won't do anything more. Your first example in #22 with color won't be resolved by this plugin.
If you want to play with color, you can do that before offering JS variables... using JS ! :p

@MoOx
Copy link
Contributor

MoOx commented Mar 31, 2015

Closed by a5aa2bf

@MoOx MoOx closed this as completed Mar 31, 2015
@hgl
Copy link
Author

hgl commented Mar 31, 2015

OK. I see where our misunderstanding lies.

The bug you fixed actually doesn't exist.

{
  "--border-color": "#aaa",
  "--background-color": "var(--border-color)"
}

Using this variables option works fine before your fix. Because at the second resolution, the js variable will be recursively resolved.

My feature request was to output the resolved values of all custom properties. I created a commit at my branch. It currently only works with custom properties specified directly in the source code and whose values don't contain var(). To make it work with custom properties specified with variables option and those containing var(), this plugin needs to offer an extra feature (we can discuss it once you have seen my commit).

Let me know if you want such feature in cssnext.

(And if I'm not wrong, this fix should be reverted.)

@MoOx
Copy link
Contributor

MoOx commented Mar 31, 2015

I will see if what I did was stupid.
I don't know what you are trying to do, but you have weird needs. You should defined JS vars and use them as is... Don't make things complicated...

var color = "#aaa"
var props = {
  "--border-color": color,
  "--background-color": "color(" + color + " alpha(0.2))"
}

FYI Qix-/color#55

@hgl
Copy link
Author

hgl commented Mar 31, 2015

but you have weird needs

I need to use these variables in my app, for doing UI animations with web-animations for example:

var startColor = variables["start-color"];
var endColor = variables["end-color"];
elem.animate([{
  color: startColor
}, {
  color: endColor
}], 3000);
{
  "--start-color": "#aaa",
  "--end-color" : "color(var(--start-color) alpha(0.2))"
}
a {
  color: var(--start-color);
}
a:hover {
  color: var(--end-color);
}

The same custom properties will be used both in css and js. So I need the resolved value in order to make it work in most browsers.

@hgl
Copy link
Author

hgl commented Mar 31, 2015

So what do you think? Could cssnext offer a list of resolved custom properties?

@MoOx
Copy link
Contributor

MoOx commented Apr 1, 2015

As I said before, for me you can do that in JS first, without adding an extra step.
If you want to share same variables from JS & CSS, just prepare your variables in JS first.

In your case you could do

var color = require("color")
var mainColor = color("#aaa")

module.exports = {
  "start-color": mainColor.rgbString(),
  "end-color" : mainColor.alpha(0.2).rgbString()
}

cssnext is in javascript so everything that is implemented come from JS. You can use JS directly.

@MoOx MoOx reopened this Apr 1, 2015
@MoOx MoOx closed this as completed Apr 1, 2015
@MoOx
Copy link
Contributor

MoOx commented Apr 1, 2015

Note: that what me and other people are doing.

@hgl
Copy link
Author

hgl commented Apr 1, 2015

Thanks for showing me the tip. Didn't realize this.

However, not super excited about this procedural approach. I'm worried the amount of boilerplate code it entails:

var color = require("color")
var mainColor = color("#aaa")
var pageWidth = 960
var sidebarWidth = pageWidth / 2

module.exports = {
  "start-color": mainColor.rgbString(),
  "end-color" : mainColor.alpha(0.2).rgbString(),
  "page-width": pageWidth + "px",
  "sidebar-width": sidebarWidth + "px",
  "search-width": sidebarWidth * 0.8 + "px"
}

Comparing it with

{
  "start-color": "#aaa",
  "end-color" : "color(var(--start-color) alpha(0.2))",
  "page-width": "960px",
  "sidebar-width": "calc(var(--page-width) / 2)",
  "search-width": "calc(var(--sidebar-width) * 0.8)"
}

I personally find the later less painful to write.

But I understand if you don't want the extraction feature in cssnext, and I'm thinking about writing an separate project to provide this feature, but I'd still like to leverage this project to resolve variables. Will you accept an extra option where custom-properties are preserved but their values are replaced with resolved ones?

@MoOx
Copy link
Contributor

MoOx commented Apr 1, 2015

If you do a correct PR with tests & docs, why not.
But what you are trying to looks like a dangerous game.

This es6 code looks better to me, and more flexible:

var color = require("color")

const mainColor = color("#aaa")
const pageWidth = 960

exports default {
  "start-color": mainColor.rgbString(),
  "end-color" : mainColor.alpha(0.2).rgbString(),
  "page-width": `${pageWidth}px`,
  "sidebar-width": `${pageWidth / 2}px`,
  "search-width": `${pageWidth * 0.8}px`
}

It's explicit.

@hgl
Copy link
Author

hgl commented Apr 1, 2015

search-width depends on sidebar-width, so it's actually

"search-width": `${pageWidth / 2 * 0.8}px`

Will send a PR soon.

@hgl
Copy link
Author

hgl commented Apr 1, 2015

Ideally this extra option should extends the existing preserve option:

preserve: "specified"
preserve: "computed"
preserve: "both"

Since the feature I'm looking overlaps with the existing preserve. true can be aliased to both to provide backwards compatibility. What do you think? Does preserve: "specified" make sense?

@MoOx
Copy link
Contributor

MoOx commented Apr 1, 2015

preserve is just a way to keep original output.
What you are trying to do is a complete hack since it's rely on the fact that all plugins are chained and resolve each of their responsibility (postcss-color-*, postcss-calc, etc..).

I think we can't accept such an option in this plugin since it's beyond its responsibility.

@hgl
Copy link
Author

hgl commented Apr 1, 2015

since it's rely on the fact that all plugins are chained

That's the reason I implement the extraction feature in cssnext. From cssnext point of view, relying on plugins being chained is not hack, and the separate project will mostly copy cssnext, but using only postcss plugins that can affect css properties values.

The feature I'm proposing to this plugin is that, comparing to the current preserve behavior:

:root {
  --width: 100px;
  --height: var(--width);
}
/* compiles to */
:root {
  --width: 100px;
  --height: 100px;
  --height: var(--width);
}

It will do:

:root {
  --width: 100px;
  --height: var(--width);
}
/* compiles to */
:root {
  --width: 100px;
  --height: 100px;
}

That is, it will not leave the specified value. Instead, it replaces it with the resolved value. So at cssnext level, I can retrieve all custom properties without getting duplicate values.

I don't think this proposal is a hack. It doesn't rely on other plugins. It simply removes the specified declaration without the conditional as the code base currently does.

Hope that can change your mind.

@MoOx
Copy link
Contributor

MoOx commented Apr 1, 2015

So what about a simple other option preserveComputedOnly ?
Or maybe:
preserve: true || "all" (current behavior)
preserve: "original" // only preserve original authored values
preserve: "computed" // only preserve computed values

@hgl
Copy link
Author

hgl commented Apr 1, 2015

I'm incline towards the later, otherwise we need to make a decision if preserve overrides preserveComputedOnly or the other way around.

I was going to suggest "original", but in css spec, they use "specified". So I thought it might be nice to follow suit.

Let me know if you insist.

@MoOx
Copy link
Contributor

MoOx commented Apr 1, 2015

if specs say specified go for this

preserve: true || "all" (current behavior)
preserve: "specified" // only preserve original authored values
preserve: "computed" // only preserve computed values

@hgl
Copy link
Author

hgl commented Apr 1, 2015

Cool. Glad we have an agreement. :)

@hgl
Copy link
Author

hgl commented Apr 1, 2015

Should I start working from the 3.2.0 commit?

@MoOx
Copy link
Contributor

MoOx commented Apr 1, 2015

Please just remove useless lines in the module BUT keep the tests to ensure we are not going back :)
Don't drop any commit, just iterate.

@hgl
Copy link
Author

hgl commented Apr 2, 2015

Writing it makes me realize what preserve: "specified" does is to simply bypass this plugin. (Users only want specified value, so no resolution is needed at all. And since this plugin is bypassed, custom properties in :root is preserved. Does exactly what the option says.)

Are you ok with this option value?

@hgl
Copy link
Author

hgl commented Apr 2, 2015

I think a better option name would be

computed: "add" // add the computed value before the specified value
computed: "replace" // replace the specified value with computed value

@MoOx
Copy link
Contributor

MoOx commented Apr 2, 2015

The thing is you think about declaration. preserve affect declaration + var() usage.
So indeed this should be moved to another option.
preserve will affect the "computed" option. Keep that in mind.

@hgl
Copy link
Author

hgl commented Apr 2, 2015

source

:root {
    --color: #aaa;
    --bg-color: var(--color);
}
body {
    background: var(--bg-color);
}

{preserve: true, computed: "add"}

:root {
    --color: #aaa;
    --bg-color: #aaa;
    --bg-color: var(--color);
}
body {
    background: #aaa;
    background: var(--bg-color);
}

{preserve: true, computed: "replace"}

:root {
    --color: #aaa;
    --bg-color: #aaa;
}
body {
    background: #aaa;
}

{preserve: false, computed: "add"}

body {
    background: #aaa;
    background: var(--bg-color);
}

{preserve: false, computed: "replace"}

body {
    background: #aaa;
}

I think computed should be dominant over preserve, because as can be seen from the examples, {preserve: false, computed: "add"} doesn't make much sense.

What about we default computed to replace, preserve to false, when computed is add, preserve is simply ignored and works like true? This makes the plugin backward compatible and won't generate incorrect css code.

@MoOx
Copy link
Contributor

MoOx commented Apr 2, 2015

I was thinking computed was only going to affect custom props definition.
Imo, this option should just not be enabled by default, so no issue for backward compatibility.
Maybe you can simply add an option like computedProperties that will only handle --* definition ?

@hgl
Copy link
Author

hgl commented Apr 2, 2015

If we introduce an option only affects custom props definition:

{preserve: true, computedProperties: true}

:root {
    --color: #aaa;
    --bg-color: #aaa;
    --bg-color: var(--color);
}
body {
    background: #aaa;
    background: var(--bg-color);
}

{preserve: true, computedProperties: false}

:root {
    --color: #aaa;
    --bg-color: #aaa;
}
body {
    background: #aaa;
    background: var(--bg-color);
}

This feels asymmetrical (also the name computedProperties doesn't feel right in these examples.). If we keep background: var(--bg-color); so it can override background: #aaa; in supporting browsers, why don't we keep --bg-color: var(--color); so it can override --bg-color: #aaa;?

OK. I was drunk.

Do need a better name though.

@hgl
Copy link
Author

hgl commented Apr 2, 2015

What about preserveSpecifiedDefinition? Doesn't work when preserve is false, defaults to true when preserve is true.

@hgl
Copy link
Author

hgl commented Apr 3, 2015

I just realized that at cssnext level, i'm actually able to dedup the custom properties definitions (when there are multiple custom properties definitions with the same name, use the last one doesn't contain var()). So a preserve: true offer by this plugin is enough to achieve the goal.

But I also realized I need the same preserve feature from the custom media plugin (want to extract custom media queries also.)

Will write a doc to clearly explain the problem and my plan of solving it. Sorry for all the noises I generated.

@hgl
Copy link
Author

hgl commented Apr 3, 2015

Please check this out when you got the time. https://gist.github.com/hgl/77f0141eea04817ee0af

I realized a problem the current preserve option could introduce while writing it.

Let me know what you think. :)

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

No branches or pull requests

2 participants