postcss / postcss-custom-properties

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

Consider merging strict, preserve and appendVariables option as `strict` #26

Closed MoOx closed 9 years ago

MoOx commented 9 years ago

I think we can simplify the usage of those 3 options as one: strict.

strict set to true will

strict will be default to false in order to reduce output (and because I am sure some will whine :p)

poke watchers + @simonsmith @hgl what do you think about that ?

hgl commented 9 years ago
  • preserve original author code as future proof code
  • append user variables at the end

Doing these two will make the custom properties harder to be extracted, for we have to remove author code when collecting computed values.

What about merging strict, appendVariables, preserve, but add fallback: false to remove fallbacks and author code?

MoOx commented 9 years ago

I didn't think about your issue indeed. Maybe we can just merge appendVariables with preserve ?

MoOx commented 9 years ago

What about having only strict: bool || "computed" false: reduced output to the minimum (default) true: all the fucking possible output (computed + fallbacks)

Passing a variables object might affect strict: true to behave like "computed". Or we can just accept "computed" in strict option to behave like preserve: computed + appendVariables.

I am thinking about that to add a strict option to cssnext to enable/disable reduced output or output closer to the spec.

hgl commented 9 years ago

strict: "computed" sounds awkward.

What about this?

source

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

preserve: false

body {
  color: #aaa;
}

preserve:"all"

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

preserve: "computed"

:root {
  --color: #aaa;
  --color2: #aaa;
}
body {
  color: #aaa;
}

I am thinking about that to add a strict option to cssnext

Good idea, i mentioned a similar unsafe option in #14. (IMO, cssnext should default to strict, better safe than sorry).

hgl commented 9 years ago

Maybe also a preserve: "specified"


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

Basically, non-false preserve preserves the definitions.

MoOx commented 9 years ago

I think I will transform preserve the only option available with the following possibilities

appendVariables will be automatic if you use a truthy preserve.

hgl commented 9 years ago

The current computed behavior also removes specified values in rules. So "computed-definition" not only affects definitions.

MoOx commented 9 years ago

Yeah but it preserve computed definition only, right ? So preserve: computed-definition seems clear to me.

hgl commented 9 years ago

What about turning preserve into an array?

false // only computed value, no definitions
[ // preserve definitions and
  "fallback", // strict behavior
  "specified", // specified value
]

So the current computed behavior is to pass an empty array, or true

MoOx commented 9 years ago

I though about that but found an issue with this method. I would like a simple "true" value that enable maximal safe output.

MoOx commented 9 years ago

If you think about it you will find an issue with the array thing

false: minimal true: maximal array ["specified", "fallback", "computed-definition"]

computed-definition overlap specified since specified are after computed-definition. So passing an array looks weird to me. + An empty array has never been a good solution. We need a verbose option name.

That why I came up with the 4 values false | true | fallback | computed.

hgl commented 9 years ago

With multiple parts being able to be preserve, a simple preserve: true feel ambiguous. preserveAll: true sounds more reasonable.

So what about we ditching true, but use "all" instead.

false // only computed value, no definitions
[ // preserve definitions and
  "fallback", // strict behavior
  "specified", // specified value
]
"all" // maximum output

Passing true is undocumented thus undefined.

hgl commented 9 years ago

Or we special case true, and treat it the same as "all"

MoOx commented 9 years ago

An empty array to enable a behavior looks like bad design to me :/

hgl commented 9 years ago

So what about a subtraction option

preserve: true | false (max or min output) exclude: ["fallback", "specified"]

MoOx commented 9 years ago

an option that depend on another is weird. What about this preserve: bool (min,max) preserve: {specified: bool, fallback: bool, whatever we can add: bool } ?

hgl commented 9 years ago

This means empty object enables a behavior.

hgl commented 9 years ago

an option that depends on another is pretty common. You see "only works under option x" fairly often in man pages.

MoOx commented 9 years ago

This means empty object enables a behavior. {} !== true We can handle that. If a user pass an empty object he is clearly trying something :D

I like this quote "Simplicity is the ultimate sophistication" - Leonardo Da Vinci.

So I am always trying to find the ultimate simple thing I can offer to people :)

If I quickly read the doc, I won't understand why there is an "exclude" option.

hgl commented 9 years ago

So I am always trying to find the ultimate simple thing I can offer to people :)

I deeply respect that. :)

If a user pass an empty object he is clearly trying something

The object option approach means this: passing an object option means enabling a major option but tuning sub-options, and passing an empty object means turning all sub-options off, but leave the major option enabled.

I feel enabling the major option and tuning sub-options should be in separate options, but I can live with the previous premise.

MoOx commented 9 years ago

We can consider an empty object is just like false ? Well this case, object can be an array indeed if it's just to enable some part.

With a second pass, my brain seems to accept this proposal as a simple one

preserve: bool (min,max) preserve: [ // preserve definitions and "fallback", // + strict behavior "specified", // + specified value ]

because I think not that much people will use the empty array scenario. But the doc will need to be clear about that case.

hgl commented 9 years ago

What if we just do this:

preserve: {
  definition: bool // keep definition
  fallback: bool // keep fallback
  specified: bool // keep specified
  variables: bool // keep variables
}

Then we can treat an empty object the same as false.

hgl commented 9 years ago

preserve: true enables all four.

hgl commented 9 years ago
preserve: {
  definition: false,
  variables: true,
}

means append variables, but not keep definitions specified directly in css.

Maybe we are giving too much customization this way?

MoOx commented 9 years ago

We should just think about the possible scenario

Edge case: I only want computed definition (not sure how to place this case in the list above :D)

hgl commented 9 years ago

I now think your four values for preserve is pretty good.

Only that I think "computed-definition" should just be "computed":

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

body {
    color: var(--color);
}

preserve: true

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

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

preserve: "computed-definition"

:root {
    --color: #aaa;
    --color2: #aaa;
}

body {
    color: #aaa;
}

It also removes specified value in rules, not only in definitions.

hgl commented 9 years ago

And internally, we map the four values for preserve to bools for these variables

definition
fallback
specified
variables

So the code should be easier to read, and when we add a new value to preserve, it won't mess with the existing code.

For example:

preserve: "fallback"

definition = true
fallback = true
specified = false
variables = true

preserve: true

definition = true
fallback = true
specified = true
variables = true
MoOx commented 9 years ago

Not sure the predefined value make sense, but I agree with the four values. Will try to handle that tomorrow + release 4.0 :)

MoOx commented 9 years ago

After some dev on that I came up with

preserve: true || {specified, fallback} preserveDefinition

preserve variables only doesn't make sense because you end up with a file that might not contains original specified values, but only one :root{} at the end. It's just weird.

and preserve: definitions was just conflicting specified everywhere, so since it's an edge case, I prefer to keep that as a separate option that overlap preserve.specified. This will need to be clear in the README that preserveDefinition will make preserve.specified ignored.

hgl commented 9 years ago

but only one :root{} at the end. It's just weird.

Do you mean this is weird?

body {
  color: #aaa;
}
:root {
  --js-defined: "no one uses this"
}

It's not weird. The custom properties in :root can be queried in supporting browsers.

document.documentElement.style.getPropertyValue("--js-defined"); // "no one uses this"

So leaving them there has a use case (at least in supporting browsers).

MoOx commented 9 years ago

No I mean

:root {--a:b}
body{}

with variables and preserve.variables will give you

body{}
:root{--js:*}

Which is weird because it seems you lost a part. So I think you should better use just preserveComputedDefinitions (better than preserveDefinitions ?) which will preserve js (computed) variables as well.

MoOx commented 9 years ago

variables + preserve.specified will gives you this

:root {--a:b}
body{}
:root{--js:*}
hgl commented 9 years ago

I thought we agreed that truthy preserve preserves definitions?

MoOx commented 9 years ago

It just doesn't make sense because we have tests like this

      if (!preserve.specified && !preserve.definitions) {
//...
      if (!preserve.specified || preserve.definitions) {
//...
    if (preserve.specified || preserve.definitions) {

So I prefer to get another option to avoid brain mess

      if (!preserve.specified && !preserveDefinitions) {
//...
      if (!preserve.specified || preserveDefinitions) {
//...
    if (preserve.specified || preserveDefinitions) {
hgl commented 9 years ago

This is what I had in mind

source

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

variables.json

{ "foo": "bar"}

preserve: false

body {
  color: #aaa;
}

preserve: "fallback"

:root {
  --color: #aaa;
  --color2: #bbb;
  --color2: #aaa;
}
body {
  color: #aaa;
}
:root {
  --foo: "bar"
}

preserve:"all" | true

:root {
  --color: #aaa;
  --color2: #bbb;
  --color2: #aaa;
}
body {
  color: #aaa;
  color: var(--color);
}
:root {
  --foo: "bar"
}

preserve: "computed"

:root {
  --color: #aaa;
  --color2: #aaa;
}
body {
  color: #aaa;
}
:root {
  --foo: "bar"
}
hgl commented 9 years ago

So you decided to go with object option?

hgl commented 9 years ago

When I said i think your four values proposal is pretty good, I mean this:

false (only new computed values (default)) fallback: fallback (strict behavior) + previous true || all: specified + previous "computed-definition": current computed behavior (your hack to be able to extract). Renamed to be clear about what it keep. This option is weird anyway.

Only that "computed-definition" should be "computed".

hgl commented 9 years ago

And when I said "And internally, we map the four values for preserve to bools for these variables"

I mean this

var def = false;
var fallback = false;
var specified = false;
var vars = false;
if (preserve === "fallback") {
  def = true;
  fallback = true;
  vars = true;
} else if (preserve === "all") {
  def = true;
  fallback = true;
  specified = true;
  vars = true;
}

// ..later in resolveValue

if (def) {
  // keep defitions
}
if (fallback) {
  // do fallback
}
// ..etc

This is much easier to reason about.

MoOx commented 9 years ago

preserving only computed definition is clearly an edge case that require different test everywhere near the "classic" preserve option (cf my previous comment).

The example you give for fallback doesn't make sense. It's useless to keep multiple definitions of the custom prop definitions since browser that will read that would only keep the last value. I don't see any smart use case.

default behavior of preserve will not add computed values for --* in :root since browser that might read that are supposed to support custom props.

I want simple things, you want the last use case (computed definition) for your extract things. Am I right ?

Let's make things simple. You edge case will be supported but probably as a separate option to avoid making weird things for end user.

MoOx commented 9 years ago

"fallback" will just add fallback for normal properties.

MoOx commented 9 years ago

Lets consider this

source

:root {
  --a: a;
  --b: var(--a, b);
}
body {
  prop: var(--b, c);
}

variables

{ foo: "bar"}

preserve: false

body {
  prop: a;
}

preserve: "fallback"

body {
  prop: c;
  prop: b;  
  prop: a;
}

preserve: true (all)

:root {
  --a: a;
  --b: var(--a, b);
}
body {
  prop: c;
  prop: b;  
  prop: a;
  prop: var(--b, c);
}

preserve: "computed"

:root {
  --a: a;
  --b: a;
}
body {
  prop: a;
}
:root {
  --foo: "bar"
}

Last use case is completly an edge case since result have nothing to do with previous examples. It's just a simple output which make accessible variables via JS

hgl commented 9 years ago

It's useless to keep multiple definitions of the custom prop definitions

Didn't think about that. Thanks for the heads up. But if I'm not wrong, the current implementation will add the fallbacks in :root when preserve: true, strict: true?

default behavior of preserve will not add computed values for --* in :root since browser that might read that are supposed to support custom props.

Sounds reasonable.

You edge case will be supported but probably as a separate option

What's the name for this separate option? How does it work?

hgl commented 9 years ago

Lets consider this

I actually like the example. Yes, preserve: "computed" is an edge case, designed specifically for the extract option. How about we make this option undocumented? It's useless other than for the extraction feature.

hgl commented 9 years ago

By undocumented, I mean we have the liberty to change the option name. Users are expected to use cssnext to access this feature. They shouldn't use this option directly.

MoOx commented 9 years ago

We might add as well

preserve: specified

:root {
  --a: a;
  --b: var(--a, b);
}
body {  
  prop: a;
  prop: var(--b, c);
}
:root {
  --foo: "bar"
}

but it doesn't make much sense to me.

I want to go foward with that plugin by pushing 4.0

Just tell me what are your use case, I don't want to add un required use cases

Here is what I see:

drop strict option drop appendVariables option (automatic depending on preserve)

preserve: false (minimal output) preserve: "fallback" (safe minimal output) preserve: true (future proof output with fallback) preserve: "computed" (your edge case)

I will document everything, I don't like undocumented options. We never know what people might want to do with the code you publish. If we need to change, we can just make a major bump.

Are you ok with that ?

hgl commented 9 years ago

Yep, lgtm. We can add preserve: specified in the future if a use case pops up.

hgl commented 9 years ago

With the default being no fallbacks, no specified values. I now think a better value for "computed" is "definitions" as you mentioned.

preserve: "definitions" // no fallbacks, no specified values, just definitions

Since there is no preserve: specified, I guess we are fine.

And if we do end up implementing it, we should use internal variables as I mentioned, and not letting each value battles with another.

MoOx commented 9 years ago

After trying to implement that change again, I think we should keep fallback in a strict option. So the things we can do is:

hgl commented 9 years ago

Thank you for continuing working on this.

So the plan is to use two options: strict and preserve?

How do they interact? For example, when preserve is false, how does strict being false/true affect the result?

The whole situation is pretty complex. Let me try to get this straight:

We would like the options to control four things:

  1. Whether author's specified variables should be preserved.
  2. Whether variables used with a fallback should be prepended with a fallback property.
  3. Whether author's specified custom property definitions should be preserved.
  4. Whether custom-property definitions specified in js should be appended.

These four are mutually independent. But we would like to introduce options to only allow a subset of all possible combinations, because some of them aren't very useful.

From our previous discussion, we would like to allow these combinations:

A: min output

  1. No
  2. No
  3. No
  4. No

B: max output

  1. Yes
  2. Yes
  3. Yes
  4. Yes

C: safe min output

  1. No
  2. Yes
  3. No
  4. No

D: output easy to extract custom property definitions

  1. No
  2. No
  3. Yes
  4. Yes

Now, how does the two-option strategy map to these four cases?

MoOx commented 9 years ago

Hum, this issue starts to give me headache.

The custom definitions scenario is not as simple as you show it, it not just no/no/yes/yes.

  1. seems to just be 2. for declarations.

Maybe we are not approching this right way.

Here are some solutions I see (that we can already handle today)

I think none of them is stupid or weird.

My original idea to simplify this was to provide a unique preserve option in order to use it in cssnext and some other plugins if we can. In a way I like the way autoprefixer handle at the same time fallback + modern code. I think I would like to end up with the same approch in cssnext, not sure if it's a good idea or a tricky one. But maybe 2 options (strict + preserve) is better in order to easily control output.

The last scenario is clearly and edge case and I am not considering it in my idea of simplification because it's just something that looks like a hack to me.