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

Add way to extract props & variants types #264

Merged
merged 4 commits into from
Oct 13, 2020
Merged

Conversation

hadihallak
Copy link
Member

@hadihallak hadihallak commented Oct 7, 2020

Adds ExtractVariantProps generic util that allows extracting the variant props out of stitches components
Allows React.ComponentProps to work with stitches components - Fixes #248

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 7, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c26cf18:

Sandbox Source
Stitches CI: Next.js Configuration
Stitches CI: CRA Configuration
stitches-ts-issue Issue #248

@peduarte
Copy link
Contributor

peduarte commented Oct 7, 2020

@hadihallak cool, nice workaround. A couple of questions:

  • If you manage to solve this with React.ComponentProps would you still keep these? Asking so I can manage breaking change expectation
  • Is it worth exposing a StitchesComponent type that does this for us? eg:
// type helper
type StitchesComponent<C> = TStyledComponentProps<C> & TExtractVariantProps<C>;

// usage
function PageContainer(props: StitchesComponent<typeof Container>) {}

We could sill expose them individually, but I imagine most people would need both types, most of the time, no?

@jjenzz
Copy link
Contributor

jjenzz commented Oct 7, 2020

@hadihallak Something like this should get React.ComponentProps working I believe (I extended React.FC):

export interface IStyledComponent<ComponentOrTag extends React.ElementType, Variants, Config extends TConfig> extends React.FC<MergeElementProps<React.ElementType, VariantASProps<Config, Variants>> & {
  as?: React.ElementType;
  css?: TCssWithBreakpoints<Config>;
  className?: string;
  children?: any;
}> {
  /**
   * Props of a styled component
   */
  <As extends React.ElementType = ComponentOrTag>(
    // Merge native props with variant props to prevent props clashing.
    // e.g. some HTML elements have `size` attribute. And when you combine
    // both types (native and variant props) the common props become
    // unusable (in typing-wise)
    props: MergeElementProps<As, VariantASProps<Config, Variants>> & {
      as?: As;
      css?: TCssWithBreakpoints<Config>;
      className?: string;
      children?: any;
    }
  ): any;
  /**
   * Compound Variant typing:
   */
  compoundVariant: (
    compoundVariants: VariantASProps<Config, Variants>,
    possibleValues: TCssWithBreakpoints<Config>
  ) => IStyledComponent<ComponentOrTag, Variants, Config>;

  /**
   * @deprecated
   */
  defaultProps?: never;
}

I deprecated the defaultProps there too because you'll most likely get TS errors and they're probably not worth solving, given that they're deprecated.

@peduarte peduarte requested a review from jonathantneal October 8, 2020 14:50
@hadihallak
Copy link
Member Author

That's Awesome @jjenzz 💯

I did two changes:
1- Updated extends React.FC<MergeElementProps<React.ElementType, VariantASProps<Config, Variants>> so that it's using ComponentOrTag instead of React.ElementType as this was causing the component to lose its original non-variants props
2- Removed the as prop from the type of React.FC as it wouldn't have an effect on the props one React.ComponentProps is called

After playing with this more I found out that the thing with the components getting a more broad typing happened also in styled-components and they have type util that allows the user to get the proper types. not sure why they haven't used something similar to what you suggested or if there are gotchas we're not aware of 🤔

I removed the old utils but part of me is thinking that TExtractVariantProps might be helpful if you only care about variants and nothing more. let me know what you guys think

@peduarte
Copy link
Contributor

Thanks @hadihallak

Some thoughts on the naming, how about:

  • StitchesComponent
  • StitchesVariants

Usage examples:

// extract all props of `Button`, inc `as` and `variants`
function MyButton(props: StitchesComponent<Button>) {}

// only extract `variants`
function MyComponent(props: StitchesVariants<Button>) {}

@jonathantneal @jjenzz what do you think?

@jonathantneal
Copy link
Contributor

For exposing types, I like these a lot. They look immediately clear in purpose, quickly writable, and structured well for IntelliSense completion.

@jjenzz
Copy link
Contributor

jjenzz commented Oct 12, 2020

@peduarte As far as I understand it, we don't need the StitchesComponent type because consumers can do React.ComponentProps<typeof Button>. Or do we think it's worth exporting something like the following as well regardless?

export type StitchesComponentProps<C = React.ElementType> = React.ComponentProps<C>

@peduarte
Copy link
Contributor

@jjenzz I think its a nicer API, a nice to have. But yeah, that should work too 👍

@hadihallak
Copy link
Member Author

Updated the PR with a new name for the variantsprops util and a StitchesComponentProps util that deals directly with the internal types instead of React.ComponentProps

@peduarte peduarte merged commit 2486366 into canary Oct 13, 2020
@peduarte peduarte deleted the fix/types/component-prpos branch October 13, 2020 09:12
@peduarte
Copy link
Contributor

Oops, merged too soon and ended up committing to canary, my bad 🤦

Commit here:
e64c73e

Renamed StitchesComponentProps to StitchesProps

@pronevich
Copy link

@jjenzz After defaultProps deprecated, how default variant should be applied?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React.ComponentProps not working as expected
5 participants