jaredh159 / tailwind-react-native-classnames

simple, expressive API for tailwindcss + react-native
2.08k stars 84 forks source link

Strange priority bug #245

Closed JamesHemery closed 8 months ago

JamesHemery commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 8 months ago

fix available in v4.0.1