Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parallel variant ordering clash #9282

Merged
merged 23 commits into from Sep 9, 2022

Conversation

thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Sep 7, 2022

Our ordering for parallel variants could result in some non-deterministic behavior depending on which ones you used and how many utilities were registered before that utility that was using the parallel variant.

This was a side effect of the single bigint sorting scheme we used. We've replaced it with Objects that have a better breakdown of what each section represents. This also means that the bigint numbers we're working with are now MUCH smaller. And, generally, only have to use large numbers for the bitfield used for variants.

This should also result in a minor speedup for the implementation of getClassOrder

@@ -107,7 +107,7 @@ test('variants without & or an at-rule are ignored', () => {

test('arbitrary variants are sorted after other variants', () => {
let config = {
content: [{ raw: html`<div class="[&>*]:underline underline lg:underline"></div>` }],
content: [{ raw: html`<div class="underline lg:underline [&>*]:underline"></div>` }],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had an error in my terminal and fixed it and now GitHub Actions is complaining and I'm pretty sure the new code is the correct code. 🤔

Symlink and build instead of adding a recursive dev dependency

It breaks node < 16
@thecrypticace thecrypticace force-pushed the fix/parallel-variant-ordering-clash branch from 4215841 to 7c121a5 Compare September 9, 2022 16:50
@thecrypticace thecrypticace merged commit d6bec49 into master Sep 9, 2022
@thecrypticace thecrypticace deleted the fix/parallel-variant-ordering-clash branch September 9, 2022 17:12
@AndyOGo
Copy link

AndyOGo commented Mar 30, 2023

Changing the order of generated classes affects precedence.

Why is this not a breaking change?

@RobinMalfait
Copy link
Contributor

@AndyOGo are you running into actual issues? The ordering changes we did shouldn't result in any breaking change unless you were already relying on behaviour that was non-deterministic before and could break the moment you introduced a new content file. A more detailed comment can be found here: #10603 (comment)

If you are running into issues, then could you open an issue with a minimal reproduction repo attached so that we can take a look?

@AndyOGo
Copy link

AndyOGo commented Mar 31, 2023

Thanks for your quick reply and sharing that comment @RobinMalfait
I understand your point of view.

I look at it from a CSS spec and semantic versioning viewpoint.
Changing the order of classes, changes the precedence of applying those style rules.
For that reason it's a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants