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

Fixing types for props with ref #1213

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

mzabriskie
Copy link
Contributor

The props for components are incorrectly typed in most cases. Since all components use forwardRef they should be using ComponentPropsWithRef (see note in @types/react).

Without this change type errors are given when passing a ref to a component. A case where this shows up is in Storybook:

// This will give a type error because `args` will include `ref`, but it is incompatible with `LabelProps.ref`
const Template: Story<LabelProps> = (args) => <Label {...args} />

// Workaround
const Template: Story<LabelProps> = ({ ref, ...args }) => <Label {...args} />

With this change type errors are resolved and the above workaround is no longer needed.

Fixes #881

@hasparus
Copy link
Member

Thanks for the PR, @mzabriskie!

@mzabriskie
Copy link
Contributor Author

@hasparus my pleasure. I am wondering about how @types/theme-ui__components comes into the picture. Since you have moved the type definitions to @theme-ui/components I believe the @types package ought to be removed. Otherwise, it is also incorrectly defined and would need a PR as well.

@hasparus
Copy link
Member

hasparus commented Oct 16, 2020

That's correct. Fixes to components types should be ported to DefinitelyTyped to update @types/theme-ui__components for Theme UI 0.3 users. We can remove @types/theme-ui__components (i.e. publish new version which is a stub) after stable 0.4 release.

@hasparus hasparus merged commit 9a7745b into system-ui:master Oct 16, 2020
Copy link
Member

@hasparus hasparus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested it locally. Let's release it fast.

@hasparus
Copy link
Member

@mzabriskie Do you want to PR DefinitelyTyped, or should I take care of it?

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

Successfully merging this pull request may close these issues.

[QUESTION] How do you implement custom components with Typescript + forwardRef?
2 participants