search

postcss / postcss-custom-properties

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

Variables starting with numbers not substituted correctly #158

Closed randak closed 5 years ago

randak commented 5 years ago

Issue

postcss-custom-properties does not currently support properties that begin with numbers, even though there is nothing in the spec prohibiting this.

Background

We recently upgraded from postcss-cssnext to postcss-preset-env, which uses this project. We had a handful of variables break upon upgrading, and the common thread was that they all started with a number (e.g. --1column).

I dug into the code here to validate the problem. The issue is two-fold. First, the regular expressions used to capture variables exclude numbers (const customPropertyRegExp = /^--[A-z][\w-]*$/;). Second, and more difficult to correct, postcss-values-parser incorrectly interprets the variable --1column as a number node. It ends up generating a node with the value of '' and the units of --1column, so when the substitution process happens, it never matches our value and replaces it.

I was able to update the tests to show the issue, as well as updating the regexes. But I'm not sure what to do about the parser or how to make it distinguish between legitimate number nodes with units and variable names like 1column.

Demo

See my branch here for the tests, some log lines, and the updated regexes: https://github.com/randak/postcss-custom-properties/tree/validate-number-issue

jonathantneal commented 5 years ago

Hi @randak,

Thanks for writing up such a clear issue.

Simplified, a Custom Property is written as 2 hyphens followed by an identifier, and an identifier begins with any letter or underscore followed by any amount of letters, numbers, underscores, or dashes. Unfortunately, this means that --1column is non-spec compliant. Here are the relevant citations:

Of course, I welcome corrections to my understanding of the specification, and I hope you’ll be nice if I have misunderstood them.

In the meantime, I empathize with how convenient your Custom Properties seemed and how troublesome cssnext transitions can be. I would warn against hacking around spec compliance, and I would encourage you to find-and-replace the troublesome Custom Properties.

For instance, could 1-column become cols-1 if it describes the length of some grid item, or could it become col-1 if it describes the ordering of some grid item?

Does this help?

randak commented 5 years ago

I was looking through the documentation yesterday, but I didn't find the railroad diagram—that was what I was missing. I'm definitely in favor of following the spec here, so I'll close this out. We did end up changing the names to have the numbers at the end, so not a big deal. Thanks for the quick response!