Internal refactor, introduce AtRule

[RobinMalfait]

This PR introduces an internal refactor where we introduce the AtRule CSS Node in our AST.

The motivation for this is that in a lot of places we need to differentiate between a Rule and an AtRule. We often do this with code that looks like this:

rule.selector[0] === '@' && rule.selector.startsWith('@media')

Another issue we have is that we often need to check for '@media ' including the space, because we don't want to match @mediafoobar if somebody has this in their CSS. Alternatively, if you CSS is minified then it could be that you have a rule that looks like @media(width>=100px), in this case we also have to check for @media(.

Here is a snippet of code that we have in our codebase:

// Find at-rules rules
if (node.kind === 'rule') {
  if (
    node.selector[0] === '@' &&
    (node.selector.startsWith('@media ') ||
      node.selector.startsWith('@media(') ||
      node.selector.startsWith('@custom-media ') ||
      node.selector.startsWith('@custom-media(') ||
      node.selector.startsWith('@container ') ||
      node.selector.startsWith('@container(') ||
      node.selector.startsWith('@supports ') ||
      node.selector.startsWith('@supports(')) &&
    node.selector.includes(THEME_FUNCTION_INVOCATION)
  ) {
    node.selector = substituteFunctionsInValue(node.selector, resolveThemeValue)
  }
}

Which will now be replaced with a much simpler version:

// Find at-rules rules
if (node.kind === 'at-rule') {
  if (
    (node.name === '@media' ||
      node.name === '@custom-media' ||
      node.name === '@container' ||
      node.name === '@supports') &&
    node.params.includes(THEME_FUNCTION_INVOCATION)
  ) {
    node.params = substituteFunctionsInValue(node.params, resolveThemeValue)
  }
}

Checking for all the cases from the first snippet is not the end of the world, but it is error prone. It's easy to miss a case.

A direct comparison is also faster than comparing via the startsWith(…) function.

---

Note: this is only a refactor without changing other code unless it was required to make the tests pass. The tests themselves are all passing and none of them changed (because the behavior should be the same).
The one exception is the tests where we check the parsed AST, which now includes at-rule nodes instead of rule nodes when we have an at-rule.

[adamwathan]

Nice! Curious to benchmark this change, expecting it to be basically identical but secretly hoping it's a tiny bit faster.

[RobinMalfait]

@adamwathan haha that was my next thing on the list to try. I'm hoping for the same results you mentioned.

[RobinMalfait]

Running some benchmarks on the Tailwind UI codebase.

Before (next):

 ✓ src/index.bench.ts (1) 667ms
     name          hz      min      max     mean      p75      p99     p995     p999     rme  samples
   · compile  54.5384  16.6713  22.1950  18.3357  18.7034  22.1950  22.1950  22.1950  ±2.45%       28

After (pr):

 ✓ src/index.bench.ts (1) 650ms
     name          hz      min      max     mean      p75      p99     p995     p999     rme  samples
   · compile  53.4548  16.5493  28.0242  18.7074  18.9284  28.0242  28.0242  28.0242  ±5.57%       27

It looks like this PR is a tiny bit slower right now. Thinking about it, it makes sense from a CSS parsing perspective because we are doing a bit more when we see at-rules. We also re-parse every time we see an at-rule in the variants used, I think this is where we can do some speedups.

Let me try some things, because it's easier to work with style rules and at rules separately, but not at the cost of perfomance.

[RobinMalfait]

Few improvements later, these are the results:

Before (next):

 ✓ src/index.bench.ts (1) 666ms
     name          hz      min      max     mean      p75      p99     p995     p999     rme  samples
   · compile  52.8106  16.1739  29.1533  18.9356  18.9055  29.1533  29.1533  29.1533  ±6.61%       27

After (pr):

 ✓ src/index.bench.ts (1) 661ms
     name          hz      min      max     mean      p75      p99     p995     p999     rme  samples
   · compile  52.8260  16.2722  29.1343  18.9301  19.7049  29.1343  29.1343  29.1343  ±6.34%       27

[adamwathan]

Nice! I think this is a great improvement.

The only high level thing I'm half wondering about is whether we should include the @ sign as part of AtRule.name for greppability even if it's not necessary? Don't have a strong opinion on it but does feel like something we're losing with this change. Could easily be convinced not to worry about it.

[adamwathan]

I think we should just use Rule for this type and rule for the helper function instead of styleRule, as rules and at-rules are distinct concepts in CSS terminology already:

[RobinMalfait]

I was thinking the same, but @thecrypticace pointed me to the spec where they use Style rules and At rules. I think just Rule is very common because of PostCSS.

• https://www.w3.org/TR/css-syntax-3/#css-stylesheets

I'm fine with either. Biggest benefit for separating is that we have a rule() abstraction that returns a style rule via styleRule() or an at rule via atRule() right now depending on the @ being present.

[thecrypticace]

Yeah the spec explicitly calls them style rules.

[adamwathan]

Yeah I'm fine with style rule then if that's considered more technically correct 👍

[RobinMalfait]

Hmm, I do like the grepability of looking for @media 🤔

Fun fact, during printing, I forgot to add an @ in some spots, so maybe this is not a bad change.

Any strong feelings about this @thecrypticace?

[thecrypticace]

Hmm, I do like the grepability of looking for @media 🤔

Fun fact, during printing, I forgot to add an @ in some spots, so maybe this is not a bad change.

Any strong feelings about this @thecrypticace?

Nah, I say go for it.

[adamwathan]

Glad we decided to include the @ seeing the diff now, I think the code reads a lot better with it being part of the string 👍