Use amount of properties when sorting

[RobinMalfait]

Right now we sort the nodes based on a pre-defined sort order based on the properties that are being used. The property sort order is defined in a list we maintain.

We also have to make sure that the property count is taken into account such that if all the "sorts" are the same, that we fallback to the property count. Most amount of properties should be first such that we can override it with more specific utilities that have fewer properties.

However, if a property doesn't exist, then it wouldn't be included in a list of properties therefore the total count was off.

This PR fixes that by counting all the used properties. If a property already exists it is counted twice. E.g.:

.foo {
  color: red;

  &:hover {
    color: blue;
  }
}

In this case, we have 2 properties, not 1 even though it's the same color property.

Test plan:

1. Updated the tests that are now sorted correctly
2. Added an integration test to make sure that prose-invert is defined after the prose-stone classes when using the @tailwindcss/typography plugin where this problem originated from.

Note how in this play (https://play.tailwindcss.com/wt3LYDaljN) the prose-invert comes before the prose-stone which means that you can't apply the prose-invert classes to invert prose-stone.

[RobinMalfait]

We were using a break before once we noticed a --tw-sort, but now we want to make sure we count all properties. So once we notice a --tw-sort we don't want to collect new properties in the order set anymore, but we still want to keep walking to count other properties.

[philipp-spiess]

Could we invert this condition so we reduce the level of nesting here?

Suggested change

	if (!seenTwSort) {
	if (teenTwSort) continue;

[philipp-spiess]

LGTM but I'm confused why two utilities got reordered in the tests even though they set the same number of properties

[philipp-spiess]

Could we invert this condition so we reduce the level of nesting here?

Suggested change

	if (!seenTwSort) {
	if (teenTwSort) continue;

[philipp-spiess]

I'm confused why the order here changed, didn't both classes define the same number of properties before? 🤔

[RobinMalfait]

This is related to the bug we had originally where only known properties are counted, but we have to think if this is what we want though 🤔

So, if you look at the the property-order file, you see this:

  'translate',
  '--tw-translate-x',
  '--tw-translate-y',

and --tw-translate-z doesn't exist in that list (we can add it though).

This means that even though we have 2 properties, in the previous code this would result in a sort order of [53] for the translate-z-* class (53 is the order of the translate property).

And for translate-y-* it would result in [53, 55] where 55 is the order of the --tw-translate-y property.

In the old code, that also means that the translate-z-* class counted 1 property, and the translate-y-* counted 2 properties, therefore it is sorted this way.

In the new code, this still results in [53, 55] and [53] but now we track the count which is 2 in both cases.

Another thing that already existed is that we only compare those numbers up to where they have the same length. So in both scenarios that would be comparing 53. Then we compare the property count, and then we compare alphabetically.

So previously we would compare the property count which was 1 vs 2, now they are both 2 and therefore we fallback to the alphabetical sort where the negative -translate-z is sorted before the other one.

The current code handling all of that:

astNodes.sort((a, z) => {
  // SAFETY: At this point it is safe to use TypeScript's non-null assertion
  // operator because if the ast nodes didn't exist, we introduced a bug
  // above, but there is no need to re-check just to be sure. If this relied
  // on pure user input, then we would need to check for its existence.
  let aSorting = nodeSorting.get(a)!
  let zSorting = nodeSorting.get(z)!

  // Sort by variant order first
  if (aSorting.variants - zSorting.variants !== 0n) {
    return Number(aSorting.variants - zSorting.variants)
  }

  // Find the first property that is different between the two rules
  let offset = 0
  while (
    aSorting.properties.order.length < offset &&
    zSorting.properties.order.length < offset &&
    aSorting.properties.order[offset] === zSorting.properties.order[offset]
  ) {
    offset += 1
  }

  return (
    // Sort by lowest property index first
    (aSorting.properties.order[offset] ?? Infinity) -
      (zSorting.properties.order[offset] ?? Infinity) ||
    // Sort by most properties first, then by least properties
    zSorting.properties.count - aSorting.properties.count ||
    // Sort alphabetically
    compare(aSorting.candidate, zSorting.candidate)
  )
})

Looking at the count/order result, if I add the --tw-translate-z to the property-order list, then this would be the result:

--tw-translate-x: 1px;
 2 [ 53, 54 ]

--tw-translate-y: 1px;
 2 [ 53, 55 ]

--tw-translate-z: calc(var(--value) * -1);
 2 [ 53, 56 ]

Where 2 is the count of properties, and the array is the sorted list based on the property order.

So uhm, I think this code is completely wrong:

  // Find the first property that is different between the two rules
  let offset = 0
  while (
    aSorting.properties.order.length < offset &&
    zSorting.properties.order.length < offset &&
    aSorting.properties.order[offset] === zSorting.properties.order[offset]
  ) {
    offset += 1
  }

Because the offset will start at 0, so the aSorting.properties.order.length and zSorting.properties.order.length will never be less than 0.

Thanks for being my rubber ducky! 😅

---

Complete sidenote: this test is wrong because this is the translate-z-* test and we use translate-y-* 😅

[philipp-spiess]

Also wouldn't expect this moving.

[RobinMalfait]

Aha! I accidentally counted properties that were set to undefined | null. So we counted 3 for this guy (font-family, font-feature-settings, font-variation-settings)

[RobinMalfait]

Use the correct translate-z-px instead. (previous commits also show the fix for the order change)

[RobinMalfait]

This was missing, but we do have it for --tw-scale-z and --tw-rotate-z so I figured why don't we just add this.

[philipp-spiess]

Unrelated to this PR but we found translate-z-px emits two css classes right now. Needs a follow-up fix

[RobinMalfait]

PR: #16718

[philipp-spiess]

Ah nice so this previously counted container and container-type as the same but isn't doing that anymore. Neat!