Conversation

edited

This fixes a number of errors in the language service parser for CSS/Less (that are not covered in microsoft/vscode#43087).

In Less, functions can be called anywhere to create a node, including any stylesheet or ruleset root. Fixed.

Custom functions can accept mixin-like arguments (such as accepting rulesets), and arguments can be separated with a semi-colon. Fixed.

Adds support for map lookups @ruleset[@lookup] or #ns.mixin(@arg)[prop]

Adds support for property references ($prop and interpolated ${prop})

Less is more forgiving of property and variable names than the CSS parser. This is particularly important after Less 3.5 which allowed the use of rulesets as maps, so declarations may be values like 100: 100 or even 10px: true if someone wanted to get crazy*. The CSS parser parses them as any number of token types (number or dimension), so there's a regex parsing function added in the Less parser to be more forgiving of property names.

Adds support for anonymous mixins, introduced with the each() function

Added if(), boolean, range(), and each() functions to Less completions

Fixes nested at-rule children of nested at-rules not being marked as nested... (Note: in Less, there are more at-rules than @supports and @media that bubble, but I notice the core CSS parser only supports those two types as being nested. Not sure if that's worth addressing or not.)

Fixes spelling for nestedProperties

Fixes detached ruleset declarations (assignment to a variable) no longer needing to terminate in a semi-colon (can end in } like any other block)

Fixes the disparity between mixin body and detached ruleset body. They support the same node types. Not sure why that was originally parsed differently here.

Fixes @supports ( as being parsed as a VariableCall. Checks for whitespace.

This comment has been minimized.

One thing that's still kind of outstanding is that because rulesets can be maps in Less, the warning and squiggly lines about "unknown property"s is somewhat useless. Is there a way for those unknown property flags to not be set in a Less document? I think just coloring the keyword differently would be enough, but the squiggly line is quite obnoxious. (That would be true for CSS in general IMO. It's okay to visually distinguish known and unknown, but I wouldn't hit the user over the head.)

Note: in Less, there are more at-rules than @supports and @media that bubble, but I notice the core CSS parser only supports those two types as being nested. Not sure if that's worth addressing or not.

That's fine for now.

Overall looks good, just a few questions to understand the less features being added.

This comment has been minimized.

Less supports variables and (like PHP) variable variables. Meaning, the name (identifier) of a variable being referenced can itself be variable. So: @@var means "return the value of a variable with the id that @var resolves to. If @var is foo then @foo is looked up/returned.

In a 2.x release, support for property referencing ($prop) was added. Basically it just means that properties are (more or less) variables.

Well it's not explicitly documented, you can technically then combine these features of variable variables, where the property identifier being looked up is variable, or the variable id being looked up is variable based on a property name.

Why someone would ever do this, I don't know, it's just a normal side-effect of combining these features.

This comment has been minimized.

You're thinking of mixins. Mixins start with . or #, and those are defined in your stylesheet. Functions are defined in JavaScript. They accept a node (or nodes), and return a node. lighten() and darken() are examples of Less functions. After Less made it easier for users to define functions using @plugin, the restriction on functions being only in a property's value was dropped. This way, authors can call a function that returns a ruleset, for example. Less also later added functions that are meant to be called around rules, such as each().

This comment has been minimized.

Similar to what's mentioned above, this is just a normal side-effect of treating properties as variables. That is, you can do interpolation in other values. Would this particular syntax in the above assertion ever make sense? Not really. But it's valid Less, which should be what the parser is checking for.

It should probably be added to Less docs, but it's valid. In a lot of cases for these, I pulled code directly from Less unit tests and added them to the language service tests. In fact, I opened Less unit tests in VSCode to see what was failing first, before adding language service tests.

This comment has been minimized.

@matthew-dean Thanks a lot for your help! We're happy to have your expertise!

Most of the changes are good. To (hopefully) make it simpler to work on this, I already pushed some of the uncontroversial changes, see 5e7a590

What I don't like so much are changes related to the map lookups. The lookup is modelled as a child to variables and mixins. I think it is cleaner if we do it in the term, like an an operator (e.g. the % operator).

This comment has been minimized.

What I don't like so much are changes related to the map lookups. The lookup is modelled as a child to variables and mixins. I think it is cleaner if we do it in the term, like an an operator (e.g. the % operator).

This comment has been minimized.

@kevinramharak I apologize. As someone who coordinates open source work, I know it's frustrating when a PR seems so close but languishes in changes. I just haven't had the extra cycles for any development on the side. Hopefully @radium-v can get to it? On a long enough timeline, I should be able to return to a pile of Less maintenance stuff, which includes this, but I don't yet have a slot in my schedule.

This comment has been minimized.

I've merged master and moved methods according to feedback. However, I found that with even the base-level merge I was getting unrelated errors on SCSS (which was logging console errors but not reporting as errors to the test runner?) and an error with a property in the CSS actions test.

This comment has been minimized.

edited

@octref Less 3.10 was a minor version bump just because I converted the Less.js project to ES2015 modules, and as a result, was more than just a bugfix? (Not really sure how that fits into Semver lol.) But to answer your question, no, there have been no syntax changes and there are none currently planned. The last major ones were the "rulesets-as-maps" feature, and the ability to call functions on any node, which this should cover.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.