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

Strange priority bug #245

Closed
JamesHemery opened this issue Aug 22, 2023 · 7 comments · Fixed by #279
Closed

Strange priority bug #245

JamesHemery opened this issue Aug 22, 2023 · 7 comments · Fixed by #279

Comments

@JamesHemery
Copy link
Contributor

JamesHemery commented Aug 22, 2023

Hi,

There is a strange priority issue :

<View style={tw`bg-blue-500 bg-red-500`} /> // background should be red-500 and he's red-500
<View style={tw`bg-red-500 bg-blue-500 bg-red-500`} /> // background should also be red-500 but he's blue-500
@jaredh159
Copy link
Owner

jaredh159 commented Aug 25, 2023

that to me seems like a programmer error. i've never attempted to guarantee that "last utility wins". do you think we should?

in stock web tailwind, i consider it a programmer error if i supply two competing utilities, as these can produce non-deterministic behavior if the order of css rules emitted ever changes. i've run into these issues in production. i don't think they guarantee (or can) that the last utility wins. which is why some tailwindcss IDE plugins warn you if you create duplicates like this.

am i off base here?

@JamesHemery
Copy link
Contributor Author

Hi @jaredh159,

Thanks for your answer!

On the web it is necessary to be careful not to have utilities that contradict each other, because CSS precedence overrides the order of classes in HTML. This is why tailwind-merge exists, and is mainly used on the web to avoid this problem, which often arises with composition (e.g.: building a system design).

However, as twrnc doesn't rely on CSS but on React Native's Stylesheet (i.e. objects), I think it would be interesting to consider that the latter utility wins by simply constructing the StyleSheet object in the same order as the class declaration? If not, the problem can be solved by using tailwind-merge, but it's a pity because here it's not a constraint linked to an external element like CSS.

What do you think?

@jaredh159
Copy link
Owner

yeah, we probably have the ability to do this, i'm just wondering if it's worth it. the examples you provided are clearly programmer error. are those just over-simplifications to show the issue? what's a real-world use case where it would be helpful to ensure that last utility wins?

i sort of lean towards the idea that i'd rather get some unexpected behavior to show me i did something wrong, rather than letting the library gloss over the issue, and i end up with unused utilities that i should probably just delete. but i may be missing a use case having to do with composing components, for sure.

@JamesHemery
Copy link
Contributor Author

Yes, this is a simplified case; here's a more telling example that's solved using tailwind-merge on the web:

function Button({ className, label }){
    return <TouchableOpacity style={tw.style('px-3 h-11 bg-red-500', className)}>{label}</TouchableOpacity>
}

<Button className="bg-blue-500"/> 

In this case, ensuring that the last utility wins seems coherent, which is why tailwind-merge exists in CSS (because of CSS precedence problems).

Of course there are workarounds, such as avoiding passing a tailwind string from the outside:

function Button({ style, label }){
    return <TouchableOpacity style={[tw`px-3 h-11 bg-red-500`, style]}>{label}</TouchableOpacity>
}

<Button style={tw`bg-blue-500`}/> 

This can be handled by the library directly, or include some kind of middleware so that tailwind-merge can be automatic :

import { twMerge } from 'tailwind-merge';

const tw = create();
tw.addMiddleware((className) => twMerge(className));

export default tw;

But I think using tailwind-merge would be a mistake, as it's designed to deal with the specific problems of CSS, since it analyzes all utilities to remove conflicting ones (which can be performance-hungry).

In our case, Javascript itself solves the problem if we generate the object in the same order as the utilities are declared in the string, it will produce :

{
    backgroundColor: '#ff0000',
    backgroundColor: '#00ff00',
}

// Same as
{ backgroundColor: '#00ff00'}

These are just suggestions, the idea being to produce the best package with collective intelligence :) !

@jaredh159
Copy link
Owner

Sorry for the delay getting back to you, been pretty busy with other projects...

I think the way I feel about this after thinking a little more, is that if it's straightforward to ensure that the last utility wins by the order in which we spread, or set props on the style object, it would be a desirable property of the library. but if it ends up being complicated because of performance or caching, then i would be more comfortable saying the onus is on the developer to not pass us duplicates.

do you know where in the codebase the problem is created? or maybe rather, do you have a sense of if there's a spot to drop in a simple fix for this? i haven't looked yet myself, but i wouldn't be surprised if there was a simple alteration that could give us this behavior.

@JamesHemery
Copy link
Contributor Author

I am working on an RFC that I will submit to you this week, if all or part of the suggestions suit you I will work on an implementation :)

@jaredh159
Copy link
Owner

fix available in v4.0.1

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 a pull request may close this issue.

2 participants