marigold-ui / marigold

Design System based on react-aria and Tailwind CSS
https://marigold-ui.io
MIT License
101 stars 7 forks source link

feat: improve `useStyle` types #887

Closed sebald closed 3 years ago

sebald commented 3 years ago

Description

Currently the input type of useStyles is very lax and allows the index type ([key: string]: any;). This allows for some silly mistakes and does not use the full potential of TypeScript.

Context

In order to harden the types we would like to do something like this:

export interface StylesProps extends Omit<CSSObject, 'variant' | 'element'> {
  element?: ElementType;
  variant?: string | string[];
}

Sadly this doesn't work because of the index type of CSSObject.

Property 'element' of type 'ElementType<any> | undefined' is not assignable to string index type 
'StylePropertyValue<string | number> | null | undefined'.

Using any for element also does not work. useStyles({ element: Component }) will result in the following error:

Type 'ComponentClass<any, any>' is not assignable to type 'ThemeUICSSObject'.
          Index signature is missing in type 'ComponentClass<any, any>'.

We basically ran into this https://github.com/microsoft/TypeScript/issues/17867

Use Cases

​useStyles​(​{​ element​,​ css​,​ className, variants ​}​)
type UseStyleInput = {
    element?: ElementType;
    css?: Omit<CSSObject, 'variant' | 'element'>;
    variants?: string | string[];
    className?: string;
}

Consequences

We would have a very type-safe API. But would have to refactor a lot of code.

sebald commented 3 years ago

A solution would be to change the API. E.g.

useStyles({ element, css, className })

or

useStyles(stylesIncludingClassName, element)
viktoria-schwarz commented 3 years ago

Hmmm. If we change the API to useStyles({ element, css, className }) that would mean something like this

export type StylesProps = {
  element?: ElementType;
  css?: Omit<CSSObject, 'css' | 'element'>;
  classNames: string | string[];
};

and we would include the variant in the classNames..? I see the problem with the lax typing but idk what would be best without changing tooo much again

sebald commented 3 years ago

Variants are part of css. But maybe rather have it on the root?

I think is worth the effort since TS already found some wrong css props when I changed it locally 😄

viktoria-schwarz commented 3 years ago

What do you mean by having the variant on the root? Where is the root here :D

sebald commented 3 years ago

​useStyles​(​{​ element​,​ css​,​ className, variants ​}​)

viktoria-schwarz commented 3 years ago

​useStyles​(​{​ element​,​ css​,​ className, variants ​}​)

So do we decide to use this new API?

It would mean to touch every component. When should we start working on this?

sebald commented 3 years ago

IDK what do you think?

viktoria-schwarz commented 3 years ago

@ti10le do you have an opinion on this?

I think it's a topic independent of the documentation, so we can start working on it (rather sooner than later otherwise I think we'll never do it). And concerning the exact API we just need to agree on something and stick to it :) If what you @sebald lastly proposed works fine I would be fine with this as well.

sebald commented 3 years ago

Also: the person who works on this, may change the proposal if she sees fit. This is just a blueprint! 🙂

ti10le commented 3 years ago

Correct what you say @viktoria-schwarz: rather sooner than later useStyles​(​{​ element​,​ css​,​ className, variants ​}​) --> this would be also a good API for me 👍🏼