fix: improve type constraints in cva function

ben-laird commented 4 months ago

Description

This PR intends to fine-tune the types surrounding the main cva function following a small problem I encountered when using cva in a different project.

Additional context

My contribution is entirely within packages/class-variance-authority/src/index.ts. The Config<T> type didn't properly constrain the type parameter, instead using a type ternary to evaluate to never when T didn't extend ConfigSchema:

type Config<T> = T extends ConfigSchema ? {
  variants?: T;
  defaultVariants?: ConfigVariants<T>;
  compoundVariants?:
    (T extends ConfigSchema
      ? (ConfigVariants<T> | ConfigVariantsMulti<T>) & ClassProp
      : ClassProp)[];
} : never;

In doing so, the second parameter in cva would often complain that the type of its argument is not assignable to undefined, essentially undefined | never. I didn't know how to fix that because I had to follow a type I couldn't introspect very well in order to introspect/know the type (ie have autocomplete, etc). I simply moved the extension constraint to the type parameter instead of in a type ternary:

type Config<T extends ConfigSchema> = {
  variants?: T;
  defaultVariants?: ConfigVariants<T>;
  compoundVariants?:
    (T extends ConfigSchema
      ? (ConfigVariants<T> | ConfigVariantsMulti<T>) & ClassProp
      : ClassProp)[];
} ;

What is the purpose of this pull request?

[x] Bug fix
[ ] New Feature
[ ] Documentation update
[ ] Other

Before submitting the PR, please make sure you do the following

[x] Read the Contributing Guidelines.
[x] Follow the Style Guide.
[x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
[x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).

vercel[bot] commented 4 months ago

@ben-laird is attempting to deploy a commit to the cva Team on Vercel.

A member of the Team first needs to authorize it.

joe-bell commented 4 months ago

Appreciate the PR, but these types have been completely rewritten already in cva@beta: https://github.com/joe-bell/cva/blob/main/packages/cva/src/index.ts

Feel free to raise your suggestions there, but I don't intend to develop further on the class-variance-authority package

ben-laird commented 4 months ago

Thanks so much for the suggestion, I'll take a look there too. I'd still want to merge this PR as it's just a patch/bug fix that improves types DX, but I don't want to presume. Thanks again for the feedback, I'll contribute to/use the beta version.

joe-bell commented 4 months ago

I'll close this for now, but keep me posted on how you get on with the beta? 🙏

joe-bell / cva