Fix non-deterministic variant sorting#14835
Conversation
We were using `peer-*/parent-name:*` instead of `peer-*/sibling-name:*`
| if (aValue === null) return -1 | ||
| if (zValue === null) return 1 |
There was a problem hiding this comment.
Same thing here — none of the tests in variants.test.ts fail without this code so I don't think this path is actually tested at the moment.
There was a problem hiding this comment.
Yeah this won't ever happen in pratice because:
- In core all our functional variants return null if given no value
matchVariant— which does support a "default" value has a dedicated compare function
Verifying this did reveal that sorting broken there though so I'm fixing that
There was a problem hiding this comment.
Removed and added a note explaining why this isn't actually necessary.
There was a problem hiding this comment.
Removing these do result in failure of the postcss tests. I think we should figure out why they are failing there and add them to the variants.test.ts if possible.
Maybe as a dedicated sorting test at the bottom of that file?
| let aValue = options?.values?.[a.value.value] ?? a.value.value | ||
| let zValue = options?.values?.[z.value.value] ?? z.value.value | ||
| let aValueKey = a.value ? a.value.value : 'DEFAULT' | ||
| let zValueKey = z.value ? z.value.value : 'DEFAULT' |
There was a problem hiding this comment.
@RobinMalfait I'm not 100% sold that this is the "right" fix. Thoughts?
|
The |
|
Fixed the changelog conflict, which in turn triggered the tests again |
| ) | ||
| } | ||
|
|
||
| let classLists = permute(['is-data:flex', 'is-data-foo:flex', 'is-data-bar:flex']) |
There was a problem hiding this comment.
Should we add a couple of arbitrary values here as well?
3 participants
Right now our variant sorting is sensitive to the authoring order of classes or the order in which we scan files because we weren't comparing variant values when sorting.
This PR addresses this by: