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

Initial support for variable expansion in at-rules (needs decision/discussion). #1845

Closed
wants to merge 1 commit into from
Closed

Initial support for variable expansion in at-rules (needs decision/discussion). #1845

wants to merge 1 commit into from

Conversation

seven-phases-max
Copy link
Member

In fact I just wanted to provide a variable interpolation for the @keyframes name (i.e. #1264, #1640 etc.). But since @keyframes rule is currently implemented as a generic "directive" ruleset also handling @supports, @document and similiar at-rules, this feature automatically applies to all of those and that makes an interpolation syntax to be used/allowed there not so obvious.
First of all I planned to make it simply @keyframes @name {...} but for @supports that would mean the following code possible: @supports (@var: @var) {...} with both @vars expanded which is barely can be accepted.
So insipired by #1648 (comment) I decided to allow both @var and @{var} expansions possible with certain (optional) restrictions.

To give a basic idea of what is theoretically possible w/o major refactoring of current "directive" implementation, here's 5 basis interpolation variants for the same example input:

@a: X;
@supports @a @{a} (@a: @{a}) and (@{a}: @a) {}

[1] expand only @var everywhere:
@supports X @{a} (X: @{a}) and (@{a}: X) {}

[2] only @var when not before ::
@supports X @{a} (@a: @{a}) and (@{a}: X) {}

[3] only @{var}:
@supports @a X (@a: X) and (X: @a) {}

[4] @{var} and @var everywhere:
@supports X X (X: X) and (X: X) {}

[5] @{var} everywhere and @var when not before ::
@supports X X (@a: X) and (X: X) {}

This PR implements [5], but can be switched to any of above. Additionally the variable expansion can be enabled/disabled for either of affected at-rules, so for example we can change it to the most safe and minimalistic [1] enabled for @keyframes only.
The list of affected at-rules:
@host
@page
@document
@supports
@keyframes

My main concern with these changes is future compatibility, i.e. current "directives" implementation with no doubt will be improved/changed/refactored eventually and then it may be not so easy to maintain too complex variable interpolation (like [5]) as things go further (for instance, @supports will need its bubbling and other @media like stuff, actually most likely @supports and @media implementations are about to be combined since they are pretty much identical feature in fact).

Other important notes, known issues etc.:

Variable reference support: @@var expansion is also supported (anywhere the @var expansion is allowed).

Comments & strings (issue). The block where the vars are expanded (i.e. @directive <here> {...}) is implemented just as a plain string (not really parsed) so the expansion works like plain text-replace stuff thus it also applies to any strings and comments happened to appear there, e.g.:

@a: X;
@supports "@a pple" /* banan @a*/ {}

expands to:

@supports "X pple" /* banan X*/ {}

Escaping. Escaping is not supported:
@supports ~"@{a}pple" {} -> @supports ~"Xpple" {}

Concatenated vars expansion. There's difference between expansion of concatenated @vars and @{var}s. Also notice the difference between how @var is expanded inside at-rules and within standard Less code:

@foo:      foo;
@baz:      baz;
@foo-bar-: mambo;

@keyframes @{foo}-bar-@{baz} {}
@keyframes @foo-bar-@baz     {}

.standard-var-expansion {
    value: @foo-bar-@baz;
}

result:

@keyframes foo-bar-baz {
}
@keyframes mambobaz {
}
.standard-var-expansion {
  value: mambo baz;
}

@lukeapage
Copy link
Member

Hi,

thanks for fixing those tests I broke (when not running on my computer) ;)

I'm thinking we should make the logic a bit simpler and follow what we do elsewhere (outside of selectors and property interpolation). e.g.

if (hasIdentifier) {
                   identifier = $variable || ($re(/^[^{]+/) || '').trim(); // excuse syntax can't remember $variable syntax

and then identifier is simply either a string or a variable node to be evaluated.

the way it is currently evaluated, the value inside the variable is "hidden" from the ast (being essentially sugar for a ~"" string and then it is turned into a string.

I know it doesn't save that much complexity, but I am keen to stop the spread of interpolated values from being everywhere. this allows us to say the format has to simply just be

@keyframe @variable {

or

@keyframe identifiers {

and if we (in the future) need to be able to parse the identifier more, we are free to do it.

Otherwise I think it looks great and uncontraversial.

@seven-phases-max
Copy link
Member Author

if (hasIdentifier) identifier = $variable

Indeed. If we decide to support only @keyframes (and @page maybe?) name and give up interpolation for other directives (e.g. @supports and @document) I think it's better to rewrite this (All that regex magic looks like one big dirty hack to be honest). I guess I'll try to craft an alternative PR soon.

@lukeapage
Copy link
Member

If we decide to support only @Keyframes (and @page maybe?) name and give up interpolation for other directives (e.g. @supports and @document) I think it's better to rewrite this (All that regex magic looks like one big dirty hack to be honest). I guess I'll try to craft an alternative PR soon.

If we want to allow variables in support I would add a new directive type for property value definitions and then use the same (or similar) parsing mechanism as media queries to actually understand the supports directive.

I think it would be fine to do this in 2 parts.

The supports spec does differ a little from media queries
http://dev.w3.org/csswg/css-conditional/
the grammar looks like it supports nested brackets, "or" (as opposed to ",") a NOT keyword and potentiolly some other logic.

stepping back - are people likely to need variable for supports? It seems less likely

@matthew-dean
Copy link
Member

Is it too expensive to allow referencing variables whereever the heck people want, except in cases of ambiguity / conflict? That is, is there a design approach where we don't have to keep "whitelisting" certain use cases?

@lukeapage
Copy link
Member

Is it too expensive to allow referencing variables whereever the heck people want, except in cases of ambiguity / conflict? That is, is there a design approach where we don't have to keep "whitelisting" certain use cases?

No, you either have a regex and the ast is non-existant, sourcemaps and compression don't work, syntax checking doesn't work or you have a AST and define what is allowed.

Variables are already allowed multiple places in the AST and any new language feature using the right nodes get variables for free, but this particular place has never had them.

@matthew-dean
Copy link
Member

Variables are already allowed multiple places in the AST and any new language feature using the right nodes get variables for free, but this particular place has never had them.

In that case, since @rule keywords come few and far between, then it does make sense to whitelist.

@seven-phases-max
Copy link
Member Author

The major show-stopper with all this is the unique syntax of those at-rules (in particular @media, @supports and even more alien @document)... Properly parsing all those to the AST (and then properly rendering it back to CSS) is the whole big story on its own. So no wonder we're constantly having to balance quick'n'dirty but working hacks vs. ideal but never implemented strict AST parsing :)

@jonschlinkert
Copy link
Contributor

In that case, since @rule keywords come few and far between, then it does make sense to whitelist.

Maybe we can take a different approach instead of whitelisting or escaping. We could do something similar to how Handlebars allows you to avoid a conflict between a helper and a variable.

To explain, let's say we have a Handlebars helper named {{basename}} AND a variable named {{basename}}. By default, Handlebars will give priority to the helper, and thus will try to evaluate basename as such. To get around this, you can tell Handlebars that you want basename to be processed as a field on the context (e.g. "just a variable"), by adding the this keyword before the variable, e.g {{this.basename}}.

So maybe we consider allowing @this.variable, so this is the only "keyword" that would ever need to be whitelisted. I personally like this, as it seems to give as much flexibility as we need and is as future-proof as we're likely to get - and it's understandable.

IMO, since our syntax for variables currently conflicts with an existing syntax in CSS, we're going to continue facing these decisions. My suggesting is that rather than coming up with different rules for different use cases (not programmatic rules, user-facing rules), we should try to decide on a consistent, understandable convention that can be used as a "tie-breaker" in every scenario.

@seven-phases-max
Copy link
Member Author

@jonschlinkert I would not say we have any major problems with @variable syntax conflicting with CSS @whatever syntax. The main syntax ambiguity is in : actually, i.e. variable assignment vs. property assignment vs. at-rule property query. And that's where @{variable} comes in (in other words we already have the @this.variable equivalent in form of @{variable}).

@jonschlinkert
Copy link
Contributor

ahhh, got it! and good point. That (probably not-so-subtle) nuance was lost on me from the beginning with this issue. thanks

@lukeapage
Copy link
Member

I think its simple, there is no demand for supports with variables so we
don't need to make the decision yet. This keeps code the simplest.

@matthew-dean
Copy link
Member

@lukeapage Agreed, based on the info so far, sounds right. 👍

@seven-phases-max
Copy link
Member Author

Closed in favour of #1860.

@seven-phases-max seven-phases-max deleted the variables-in-atrules branch February 16, 2014 16:14
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.

4 participants