feat(preset-attributify): improve ternary support #2271
Conversation
✅ Deploy Preview for unocss ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
I am not so sure about this - ideally, UnoCSS should be language/framework agnostic. In attrubutify mode we do specialize the HTML support. But having ternary support could be a framework specific thing. I think does not scale in the long term. |
Every framework out there that has HTML-like interface also supports ternaries and their users use them. Since most of our users use UnoCSS through a framework, we should also specialize in that part too.
We couldn't be language agnostic without knowing what language we're dealing with. In its current form, Initial ternary support was aiming to reduce false positives. You can see false positives in this deploy preview of an older PR. Initial ternary support wasn't perfect. When ternaries are nested inside a string literal, it ignores everything the string literal has, and only extracts the ternary's left and right sides. You can see that in main branch right now: Fixing that was not as easy as today back then. It's required proper extraction and syntax highligting. In recent weeks, I adressed a bunch of edge cases in extraction and highlighting part. And we're here today: Removing this feature was the easiest thing to do for me. I wanted to give it a try to see if this can work. While working on it I asked myself a few times; "Why do we even support that?".
I definitely agree with that. But this PR significantly improves an existing feature and it may not need to be scaled. It could stop there. Let's give it a try and see how far it goes 🤞 |
There was a problem hiding this comment.
What I am worrying about is that - I do not understand the new regex and logic you introduced. I definitely trust you, but the problem is the complexity here. To demonstrate my concern, I have pushed a few negative test cases. I imagine our implementation will be more complex and more specific if we want to fix them all.
The fact is that JavaScript is so dynamic, and it's not possible to cover all the cases using RegExp.
I see "false-positive" as unavoidable as we can't possibly know the precise intention of users' code, or properly parse every language. The cost of "false-positive" is to ship a few extra unused CSS (that we try to minimize as possible). While on the other hand, "false-negtive" will be much more harmful as it might break users' app or their expectations.
I feel that I made a hasty decision to have the initial ternary support - I actually considering we'd better remove the ternary handle as a whole to use the simple split way.
| /* layer: default */ | ||
| [text~="hover\:red-500"]:hover, | ||
| [text~="red-500"]{--un-text-opacity:1;color:rgba(239,68,68,var(--un-text-opacity));} | ||
| [text~="red-400"]{--un-text-opacity:1;color:rgba(248,113,113,var(--un-text-opacity));} |
There was a problem hiding this comment.
When the value of the attribute is "looks like a ternary" it will cause a false negative. red is missing here.
| /* layer: default */ | ||
| [text~="blue-1"]{--un-text-opacity:1;color:rgba(219,234,254,var(--un-text-opacity));} | ||
| [text~="green-1"]{--un-text-opacity:1;color:rgba(220,252,231,var(--un-text-opacity));} | ||
| [text~="red-1"]{--un-text-opacity:1;color:rgba(254,226,226,var(--un-text-opacity));} |
There was a problem hiding this comment.
It failed to extract the nested string template, yellow-1 yellow-2 is missing here.
| [text~="blue-1"]{--un-text-opacity:1;color:rgba(219,234,254,var(--un-text-opacity));} | ||
| [text~="blue-2"]{--un-text-opacity:1;color:rgba(191,219,254,var(--un-text-opacity));} | ||
| [text~="green-1"]{--un-text-opacity:1;color:rgba(220,252,231,var(--un-text-opacity));} | ||
| [text~="red-1"]{--un-text-opacity:1;color:rgba(254,226,226,var(--un-text-opacity));} |
There was a problem hiding this comment.
yellow-1 yellow-2 red-2 is missing
No description provided.