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

Incorrect TS type/inference for compose function in CVA beta #256

Open
kaelansmith opened this issue Jan 14, 2024 · 4 comments
Open

Incorrect TS type/inference for compose function in CVA beta #256

kaelansmith opened this issue Jan 14, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@kaelansmith
Copy link

Describe the bug
First off, thank you very much for your work on cva. I'm using compose from the beta version of cva @1.0.0-beta.1 to merge two cva components; functionally it works as expected, but the resulting Type appears to be incorrect, thereby breaking VSCode autocomplete for the composed/merged cva component. I wrote my own Compose type that appears to fix it (see further below).

To reproduce

const A = cva({
  base: "A",
  variants: {
    style: {
      primary: "primary-A"
    }
  }
});

const B = cva({
  base: "B",
  variants: {
    style: {
      primary: "primary-B",
      secondary: "secondary-B",
    }
  }
});

const C = cva({
  base: "C",
  variants: {
    style: {
      tertiary: "tertiary-C",
    }
  }
});

const merged = compose(A, B, C);
merged({ style:  }) // auto-complete doesn't show any options for `style`; should show `primary`, `secondary`, `tertiary`

I'm really not a TS expert, but with the help of good ol' ChatGPT I came up with this alternative Typing for compose that seems to fix the issue:

type MergeVariantProps<Types extends any[]> = Types extends [infer First, ...infer Rest]
  ? First extends object
    ? Rest extends any[]
      ? {
          [K in keyof First | keyof MergeVariantProps<Rest>]: K extends keyof First
            ? K extends keyof MergeVariantProps<Rest>
              ? First[K] | Exclude<MergeVariantProps<Rest>[K], First[K]>
              : First[K]
            : K extends keyof MergeVariantProps<Rest>
            ? MergeVariantProps<Rest>[K]
            : never
        }
      : never
    : never
  : {};

export interface Compose {
  <T extends any[]>(...components: T): (
    props?: Partial<MergeVariantProps<{ [K in keyof T]: VariantProps<T[K]> }>> & CVAClassProp
  ) => string;
}

const myCompose = compose as Compose;
const merged = myCompose(A, B, C);
merged({ style:  }) // auto-complete now shows `style` options `primary`, `secondary`, `tertiary`

Happy to get a PR going with these changes, but wanted to get your take on this and/or figure out if I'm missing something.

@kaelansmith kaelansmith added the bug Something isn't working label Jan 14, 2024
@joe-bell
Copy link
Owner

Thanks for this @kaelansmith! Honestly I need to do a bit more thinking/digging here as I'm not even sure if compose() overrides components in this way

@kaelansmith
Copy link
Author

@joe-bell for sure -- I did some of my own testing and was pleasantly surprised to find it worked how I hoped it would; here's an example:

const A = cva({
  base: 'base-A',
  variants: {
    style: {
      primary: 'primary-A',
    },
  },
});

const B = cva({
  base: 'base-B',
  variants: {
    style: {
      primary: 'primary-B',
      secondary: 'secondary-B',
    },
    newBStyle: {
      fancy: 'fancy-B',
    },
  },
});

const C = cva({
  base: 'base-C',
  variants: {
    style: {
      tertiary: 'tertiary-C',
    },
    newBStyle: {
      fancy: 'fancy-C',
    },
  },
});

const merged = compose(A, B, C);
const result = merged({ style: 'primary', newBStyle: 'fancy' });
// result => "base-A primary-A base-B primary-B fancy-B base-C fancy-C"

This result is exactly what I'd expect; I like that style.primary from B didn't override style.primary from A -- it only extended it (concatenation). My use-case (like most I presume) is Tailwind styling, and when you incorporate tailwind-merge via the defineConfig onComplete hook, that takes care of removing conflicting classes from the compose concatenation (with the later components provided to compose taking precedence); I love this because it allows UI library/package developers (like me) to provide default Tailwind styling via CVA components, and package users can easily and elegantly extend/override small pieces of that styling via compose.

One interesting behavior I came across in my testing, which I'm not sure about, is how defaultVariants works when composing multiple components; example:

// same example code as before, but added defaultVariants to `B`:
const A = cva({
  base: 'base-A',
  variants: {
    style: {
      primary: 'primary-A',
    },
  },
});

const B = cva({
  base: 'base-B',
  variants: {
    style: {
      primary: 'primary-B',
      secondary: 'secondary-B',
    },
    newBStyle: {
      fancy: 'fancy-B',
    },
  },
  defaultVariants: { // added
    newBStyle: 'fancy',
  },
});

const C = cva({
  base: 'base-C',
  variants: {
    style: {
      tertiary: 'tertiary-C',
    },
    newBStyle: {
      fancy: 'fancy-C',
    },
  },
});

const merged = compose(A, B, C);
const result = merged({ style: 'primary' }); // removed `newBStyle: 'fancy'` because it's set as default in B
// result => "base-A primary-A base-B primary-B fancy-B base-C"

This result excludes fancy-C as defined in component C; I think I would've expected it to be included because B already set newBStyle: 'fancy' as a defaultVariant, and C didn't override that. Not a big deal though, I don't have strong opinions around that either way.. thoughts?

@413n
Copy link

413n commented Apr 3, 2024

Would the possibility to specify a merger function as param be something relevant? In my case I wanted to use tailwind-merge to merge the classes based on composition order, but since the classed are simply concatenated, the browser picks the classes in the order I don't want

@lunelson
Copy link

lunelson commented Apr 3, 2024

Would the possibility to specify a merger function as param be something relevant?

You can always pass the whole result in to tailwind-merge; what I'd expect from this function is that the concatenated order of the classes is the same as the order passed to compose, so you'd get ${/* classes from A */} ${/* classes from B */} ${/* classes from C */}, then tailwind-merge can do it's thing on that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants