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

Incorrectly merged styles #14

Open
gjtorikian opened this issue Mar 16, 2024 · 5 comments
Open

Incorrectly merged styles #14

gjtorikian opened this issue Mar 16, 2024 · 5 comments

Comments

@gjtorikian
Copy link

Hi there! I maintain tailwind_merge, which is just a straight Ruby port of the JS tailwind-merge.

Every so often, I think about writing a Rust crate to back my Ruby gem. Today, I found this project. I was able to quickly incorporate the crate into my own, and was delighted to see that it works!

...for the most part.

When I ran my test suite (which again, is a port of tailwind-merge's), it came back with these errors, suggesting that in some cases tailwind-fuse isn't handling a few merge cases:

❯ be rake test  
Run options: --seed 33297

# Running:

..FFFFFF....SS.....F..FF..FFFF......FF.F..............FFF.

Fabulous run in 0.112988s, 513.3288 runs/s, 2363.0828 assertions/s.

  1) Failure:
TestModifiers#test_conflicts_across_prefix_modifiers [test/test_modifiers.rb:13]:
--- expected
+++ actual
@@ -1 +1 @@
-"hover:block focus:hover:inline"
+"hover:block hover:focus:inline focus:hover:inline"


  2) Failure:
TestModifiers#test_conflicts_across_postfix_modifiers [test/test_modifiers.rb:20]:
Expected: "text-lg/none"
  Actual: "leading-9 text-lg/none"

  3) Failure:
TestArbitraryProperties#test_handles_important_modifier_correctly [test/test_arbitrary_properties.rb:31]:
--- expected
+++ actual
@@ -1 +1 @@
-"[some:one] ![some:another]"
+"![some:prop] [some:other] [some:one] ![some:another]"


  4) Failure:
TestArbitraryProperties#test_handles_arbitrary_property_conflicts_correctly [test/test_arbitrary_properties.rb:11]:
--- expected
+++ actual
@@ -1 +1 @@
-"[paint-order:normal]"
+"[paint-order:markers] [paint-order:normal]"


  5) Failure:
TestArbitraryProperties#test_handles_arbitrary_property_conflicts_with_modifiers_correctly [test/test_arbitrary_properties.rb:18]:
--- expected
+++ actual
@@ -1 +1 @@
-"hover:[paint-order:normal]"
+"hover:[paint-order:markers] hover:[paint-order:normal]"


  6) Failure:
TestArbitraryProperties#test_handles_complex_arbitrary_property_conflicts_correctly [test/test_arbitrary_properties.rb:26]:
--- expected
+++ actual
@@ -1 +1 @@
-"[-unknown-prop:url(https://hi.com)]"
+"[-unknown-prop:::123:::] [-unknown-prop:url(https://hi.com)]"


  7) Failure:
TestArbitraryValues#test_handles_arbitrary_length_conflicts_with_labels_and_modifiers_correctly [test/test_arbitrary_values.rb:31]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus:hover:m-[length:var(--c)]"
+"hover:focus:m-[2px] focus:hover:m-[length:var(--c)]"


  8) Failure:
TestPrefixes#test_prefix_working_corectly [test/test_prefixes.rb:11]:
Expected: "tw-hidden"
  Actual: "tw-block tw-hidden"

  9) Failure:
TestTailwindMerge#test_it_basically_works [test/test_tailwind_merge.rb:18]:
Expected: "stroke-[3]"
  Actual: "stroke-2 stroke-[3]"

 10) Failure:
TestSeparator#test_single_character_separator_working_correctly [test/test_seperator.rb:12]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus_hover_!inset-0"
+"hover_focus_!right-0 focus_hover_!inset-0"


 11) Failure:
TestSeparator#test_multiple_character_separator_working_correctly [test/test_seperator.rb:22]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus__hover__!inset-0"
+"hover__focus__!right-0 focus__hover__!inset-0"


 12) Failure:
TestTailwindCSSVersions#test_tailwind_3_4_features [test/test_tailwind_css_versions.rb:39]:
--- expected
+++ actual
@@ -1 +1 @@
-"has-[[data-potato]]:p-2 group-has-[:checked]:flex"
+"has-[[data-potato]]:p-1 has-[[data-potato]]:p-2 group-has-[:checked]:grid group-has-[:checked]:flex"


 13) Failure:
TestTailwindCSSVersions#test_tailwind_3_3_features [test/test_tailwind_css_versions.rb:14]:
Expected: "text-red text-lg/8"
  Actual: "text-lg/8"

 14) Failure:
TestArbitraryVariants#test_arbitrary_variants_with_modifiers [test/test_arbitrary_variants.rb:18]:
--- expected
+++ actual
@@ -1 +1 @@
-"dark:hover:lg:[&>*]:line-through"
+"dark:lg:hover:[&>*]:underline dark:hover:lg:[&>*]:line-through"


 15) Failure:
TestArbitraryVariants#test_arbitrary_variants_with_arbitrary_properties [test/test_arbitrary_variants.rb:45]:
--- expected
+++ actual
@@ -1 +1 @@
-"[&>*]:[color:blue]"
+"[&>*]:[color:red] [&>*]:[color:blue]"


 16) Failure:
TestArbitraryVariants#test_multiple_arbitrary_variants [test/test_arbitrary_variants.rb:40]:
--- expected
+++ actual
@@ -1 +1 @@
-"dark:hover:[&>*]:disabled:focus:[&_div]:line-through"
+"hover:dark:[&>*]:focus:disabled:[&_div]:underline dark:hover:[&>*]:disabled:focus:[&_div]:line-through"


 17) Failure:
TestNegativeValues#test_handles_conflicts_across_groups_with_negative_values_correctly [test/test_negative_values.rb:22]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus:hover:inset-x-1"
+"hover:focus:-right-1 focus:hover:inset-x-1"


 18) Failure:
TestNegativeValues#test_handles_conflicts_between_positive_and_negative_values_correctly [test/test_negative_values.rb:17]:
Expected: "-top-69"
  Actual: "top-12 -top-69"

 19) Failure:
TestNegativeValues#test_handles_negative_value_conflicts_correctly [test/test_negative_values.rb:12]:
Expected: "-top-2000"
  Actual: "-top-12 -top-2000"

58 runs, 267 assertions, 19 failures

If you'd like to see for yourself, you can clone my repo, switch to the get-rusty branch, and run bundle install && bundle exec rake compile test (assuming you have Ruby installed).

Let me know if I can provide any more info to resolve these mismatches!

@nicoburniske
Copy link
Member

Thanks for reporting this! I can go through these one by one. There are definitely some bugs

1) Failure:
TestModifiers#test_conflicts_across_prefix_modifiers [test/test_modifiers.rb:13]:
--- expected
+++ actual
@@ -1 +1 @@
-"hover:block focus:hover:inline"
+"hover:block hover:focus:inline focus:hover:inline"

These two prefix modifiers actually create different tailwind classes, so I elected to handle them separately. If they were the same, wouldn't they only generate one tailwind class? I am open to hearing why they should be considered the same though.

.focus\:hover\:inline:hover:focus{
  display: inline
}

.hover\:focus\:inline:focus:hover{
  display: inline
}

This is a bug

  2) Failure:
TestModifiers#test_conflicts_across_postfix_modifiers [test/test_modifiers.rb:20]:
Expected: "text-lg/none"
  Actual: "leading-9 text-lg/none"

All the arbitrary values ones are not supported rn and should be fixed.

  8) Failure:
TestPrefixes#test_prefix_working_corectly [test/test_prefixes.rb:11]:
Expected: "tw-hidden"
  Actual: "tw-block tw-hidden"
  
 10) Failure:
TestSeparator#test_single_character_separator_working_correctly [test/test_seperator.rb:12]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus_hover_!inset-0"
+"hover_focus_!right-0 focus_hover_!inset-0"

Prefixes and separators do not work with the macro. You need to configure options with this method https://docs.rs/tailwind_fuse/latest/tailwind_fuse/merge/fn.tw_merge_with_options.html

See #12

  9) Failure:
TestTailwindMerge#test_it_basically_works [test/test_tailwind_merge.rb:18]:
Expected: "stroke-[3]"
Actual: "stroke-2 stroke-[3]"

This is a bug

@nicoburniske
Copy link
Member

I think the rest of them are bugs

@gjtorikian
Copy link
Author

Hi, sorry for the late reply!

These two prefix modifiers actually create different tailwind classes, so I elected to handle them separately. If they were the same, wouldn't they only generate one tailwind class? I am open to hearing why they should be considered the same though.

I dug up the original commit to see if there was any additional context on this test: dcastil/tailwind-merge@68ad612

Since there isn't any additional context, I'll note what I'm observing. I'm not a CSS expert, but here's my logical breakdown:

  • According to this test, the classes hover:block hover:focus:inline focus:hover:inline should yield hover:block focus:hover:inline; that is, hover:focus:* is overwritten by focus:hover:*
  • Here's a Tailwind playground example which defines a similar arrangement: hover:bg-indigo-700 hover:focus:bg-red-700 focus:hover:bg-green-700
    • I think the inline / bg-color change here is irrelevant, I only chose them to more easily see the visual change
  • When all is said and done, only bg-red is activated:
Screenshot 2024-05-01 at 13 34 10

I think the issue here is that focus:hover and hover:focus end up representing the same two pseudoclasses, and so whatever version comes last is the one that should be applied. What do you think?

Prefixes and separators do not work with the macro. You need to configure options with this method

Apologies! I needed to do this in Ruby as well but neglected to port that config logic over to my Rust FFI.

@gjtorikian
Copy link
Author

gjtorikian commented May 1, 2024

Prefixes and separators do not work with the macro. You need to configure options with this method

Apologies! I needed to do this in Ruby as well but neglected to port that config logic over to my Rust FFI.

I can't for the life of me get this to work. Even a simple reproduction of the example from the README, like this

const OPTIONS: MergeOptions = MergeOptions {
    prefix: "",
    separator: "_",
};

fn merge(str1: String, str2: String) -> String {
    assert_eq!(
        "tw-bg-black tw-bg-white",
        tw_merge!("tw-bg-black", "tw-bg-white"),
    );

    set_merge_options(OPTIONS);

    assert_eq!("tw-bg-white", tw_merge!("tw-bg-black", "tw-bg-white"),);
    tw_merge!(str1, str2)
}

fails with

fatal: assertion `left == right` failed
  left: "tw-bg-white"
 right: "tw-bg-black tw-bg-white"

Any idea what could be different?

@gjtorikian
Copy link
Author

Any idea what could be different?

It was a classic PEBCAC—got this working, too.

The remaining test failures I see fall into the camp of new Tailwind 3.3/3.4 features, and this question of focus:hover versus hover:focus:

  1) Failure:
TestArbitraryVariants#test_arbitrary_variants_with_arbitrary_properties [test/test_arbitrary_variants.rb:46]:
--- expected
+++ actual
@@ -1 +1 @@
-"[&[data-foo][data-bar]:not([data-baz])]:noa:nod:[color:blue]"
+"[&[data-foo][data-bar]:not([data-baz])]:nod:noa:[color:red] [&[data-foo][data-bar]:not([data-baz])]:noa:nod:[color:blue]"


  2) Failure:
TestArbitraryVariants#test_arbitrary_variants_with_modifiers [test/test_arbitrary_variants.rb:18]:
--- expected
+++ actual
@@ -1 +1 @@
-"dark:hover:lg:[&>*]:line-through"
+"dark:lg:hover:[&>*]:underline dark:hover:lg:[&>*]:line-through"


  3) Failure:
TestArbitraryVariants#test_multiple_arbitrary_variants [test/test_arbitrary_variants.rb:40]:
--- expected
+++ actual
@@ -1 +1 @@
-"dark:hover:[&>*]:disabled:focus:[&_div]:line-through"
+"hover:dark:[&>*]:focus:disabled:[&_div]:underline dark:hover:[&>*]:disabled:focus:[&_div]:line-through"


  4) Failure:
TestSeparator#test_multiple_character_separator_working_correctly [test/test_seperator.rb:22]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus__hover__!inset-0"
+"hover__focus__!right-0 focus__hover__!inset-0"


  5) Failure:
TestSeparator#test_single_character_separator_working_correctly [test/test_seperator.rb:12]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus_hover_!inset-0"
+"hover_focus_!right-0 focus_hover_!inset-0"


  6) Failure:
TestPrefixes#test_prefix_working_corectly [test/test_prefixes.rb:11]:
Expected: "tw-hidden"
  Actual: "tw-block tw-hidden"

  7) Failure:
TestModifiers#test_conflicts_across_prefix_modifiers [test/test_modifiers.rb:13]:
--- expected
+++ actual
@@ -1 +1 @@
-"hover:block focus:hover:inline"
+"hover:block hover:focus:inline focus:hover:inline"


  8) Failure:
TestArbitraryValues#test_handles_arbitrary_length_conflicts_with_labels_and_modifiers_correctly [test/test_arbitrary_values.rb:31]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus:hover:m-[length:var(--c)]"
+"hover:focus:m-[2px] focus:hover:m-[length:var(--c)]"


  9) Failure:
TestArbitraryProperties#test_handles_arbitrary_property_conflicts_with_modifiers_correctly [test/test_arbitrary_properties.rb:20]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus:hover:[paint-order:normal]"
+"hover:focus:[paint-order:markers] focus:hover:[paint-order:normal]"


 10) Failure:
TestNegativeValues#test_handles_conflicts_across_groups_with_negative_values_correctly [test/test_negative_values.rb:22]:
--- expected
+++ actual
@@ -1 +1 @@
-"focus:hover:inset-x-1"
+"hover:focus:-right-1 focus:hover:inset-x-1"


 11) Failure:
TestTailwindCSSVersions#test_tailwind_3_3_features [test/test_tailwind_css_versions.rb:16]:
--- expected
+++ actual
@@ -1 +1 @@
-"start-1 end-1 ps-1 pe-1 ms-1 me-1 rounded-s-md rounded-e-md rounded-ss-md rounded-ee-md"
+"start-1 end-1 ps-0 ps-1 pe-0 pe-1 ms-1 me-1 rounded-s-md rounded-e-md rounded-ss-md rounded-ee-md"


 12) Failure:
TestTailwindCSSVersions#test_tailwind_3_4_features [test/test_tailwind_css_versions.rb:39]:
--- expected
+++ actual
@@ -1 +1 @@
-"has-[[data-potato]]:p-2 group-has-[:checked]:flex"
+"has-[[data-potato]]:p-1 has-[[data-potato]]:p-2 group-has-[:checked]:grid group-has-[:checked]:flex"

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

No branches or pull requests

2 participants