Issues with `ClassNameValue` types
cschroeter opened this issue · 15 comments
Given this Button component in Solid.js I receive the following type error.
import { splitProps, type JSX } from 'solid-js'
import { tv, type VariantProps } from 'tailwind-variants'
export interface ButtonProps
extends ButtonVariantProps,
JSX.ButtonHTMLAttributes<HTMLButtonElement> {}
export const Button = (props: ButtonProps) => {
const [variantProps, buttonProps] = splitProps(props, ['class', 'size'])
return <button class={styles(variantProps)} {...buttonProps} />
}
type ButtonVariantProps = VariantProps<typeof styles>
const styles = tv({
base: 'button',
defaultVariants: { size: 'md' },
variants: {
size: {
sm: 'button--size_sm',
md: 'button--size_md',
lg: 'button--size_lg',
},
},
})Argument of type 'Pick<ButtonProps, "class" | "size">' is not assignable to parameter of type '({ size?: "md" | "sm" | "lg" | undefined; } & ClassProp<ClassNameValue>) | undefined'.
Type 'Pick<ButtonProps, "class" | "size">' is not assignable to type '{ size?: "md" | "sm" | "lg" | undefined; } & { class: ClassNameValue; className?: undefined; }'.
Type 'Pick<ButtonProps, "class" | "size">' is not assignable to type '{ class: ClassNameValue; className?: undefined; }'.
Property 'class' is optional in type 'Pick<ButtonProps, "class" | "size">' but required in type '{ class: ClassNameValue; className?: undefined; }'.ts(2345)
const variantProps: Pick<ButtonProps, "class" | "size">
To simply the issue even further:
interface Foo {
class?: string
}
export const App = () => {
const variantProps: Foo = {}
styles(variantProps) // error
}This gives the same error, but I think the type is perfectly fine?!
@cschroeter I wasn't able to reproduce this locally. Could you create a reproduction repo or code sandbox that shows the error?
@jrgarciadev Looks like this issue is related to the ClassProp type.
export type ClassProp<V extends unknown = ClassValue> =
| {
class: V;
className?: never;
}
| {class?: never; className: V}
| {class?: never; className?: never};Technically the ClassProp type is not accurate based on how the runtime works. To fix this issue we could relax the type to support passing both a class and className at the same time. I understand why that's not ideal given that in most cases, it would be confusing to pass both, but would simplify it a bit and also fix bugs like this one.
export type ClassProp<V extends unknown = ClassValue> = {
class: V;
className?: V;
};Not tested but couldn't that work?
export type Without<T, U> = { [P in Exclude<keyof T, keyof U>]?: never }
export type XOR<T, U> = T | U extends object ? (Without<T, U> & U) | (Without<U, T> & T) : T | U
export type ClassProp<V extends unknown = ClassValue> = XOR<{ class: V }, { className: V }>@cschroeter Even with that code, the same issue occurs. The issue is that Foo is specifying both class and className, so there isn't a way to support that and also enforce that you only pass one of the two props.
@mskelton got it. Yeah so I guess relaxing the types is a good solution after all.
I had an issue with this in cva library and did provide a fix for the typing. The issue is the ambiguity of union when both elements of the union can be undefined.
Here is the direct link from the issue to the solution with explanation: TS Playground
@akomm The type of ClassProp in tailwind-variants already supports the 4 use cases that you showed in the TS Playground link. The issue is when you define both class and className.
@cschroeter My recommendation would be to update your component to always use className for your Button component as right now your type supports both it would seem.
Are you sure? Here reproduction with only using className: TS Playground added import * as React from "react"
I've also added a VBoxAlternative component in the example, which shows how it is possible that you maybe did not notice the problem. Because it only becomes apparent if you pass through the className in a specific way. Or did I miss something?
Here is using the ClassProp from tailwind-variants: TS Playground
Here fixing the ClassProp from tailwind-variants: TS Playground
Does it miss some cases maybe?
Another shot: if you have exactOptionalProperties enabled, you won't notice the issue. The issue derives from the fact that | undefined is added to class or className property resulting in more than one element in the union being able to have the value undefined, so TS can not depict one unique element from the union. Instead it reduces the full union to a smaller union of multiple elements that both fit the constraint and then applies the constraint on the user site to satisfies both cases at once, not just one.
Explanation and demo why: TS Playground
Thank you for your efforts in addressing this issue. I appreciate the time you've invested. However, the solution you proposed doesn't seem to align with Solid.js's approach, as demonstrated in the example I've shared below. This example showcases a basic button implementation in Solid.js using the Solid JSX namespace.
In Solid.js you do not use className, but either classList or class https://www.solidjs.com/tutorial/bindings_classlist
import { splitProps, type JSX } from 'solid-js'
import { tv, type VariantProps } from 'tailwind-variants'
export interface ButtonProps
extends ButtonVariantProps,
JSX.ButtonHTMLAttributes<HTMLButtonElement> {}
export const Button = (props: ButtonProps) => {
const [variantProps, buttonProps] = splitProps(props, ['class', 'size'])
return <button class={styles(variantProps)} {...buttonProps} />
}
type ButtonVariantProps = VariantProps<typeof styles>
const styles = tv({
base: 'button',
})@akomm I had a mistake in my testing. After more testing, the solution from CVA does indeed work for tailwind-variants as well, so I've put up a PR with this fix.
@mskelton anything I can help to push this over the finish line?
@cschroeter @akomm Just waiting on a review from @jrgarciadev. The project is definitely not dead, but Junior and the NextUI team have had a lot on their plate lately with the launch of NextUI Pro.