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

PropsOf<C> TypeScript utilities - Support defaultProps #1405

Conversation

ryanswanson
Copy link
Contributor

@ryanswanson ryanswanson commented Jun 18, 2019

What:
Upgrade the PropsOf<C> TypeScript utilities to support defaultProps by delegating to new JSX and React types.

Why:
The PropsOf<C> utilities in @emotion/styled-base and emotion/theming currently result in a TypeScript error when using styled or withTheme with a component that has a defaultProp that is made optional by defaulting the prop value, yet is required on the Props interface and no value is passed to the component instance.

Error: /src/emotion/packages/styled-base/types/tests.tsx:288:24
ERROR: 288:24  expect  TypeScript@next compile error:
Property 'prop' is missing in type '{}' but required in type 'Props'.
ERROR: 297:21  expect  TypeScript@next compile error:
Property 'prop' is missing in type '{}' but required in type 'Props'.

See tests in this PR for both class-based and function-based component scenarios.

How:
The solution provided here uses some newer typings for JSX and React (requiring some TypeScript and @types/react upgrades).

Complexity is reduced by adapting a library-provided interface that is parameterized by both a component and its props (JSX.LibraryManagedAttributes<C, P>) to our utility interface parameterized by only a component (PropsOf<C>).

The React.ComponentProps<C> type utility is the key that allows us to extract Props (P) from the component (C).

The JSX.LibraryManagedAttributes<C, P> type utility ensures props with associated defaultProps are made optional (also see ReactManagedAttributes and Defaultize utilities in @types/react).

The type parameter C is constrained to match that of React.ComponentProps<C>: keyof JSX.IntrinsicElements | React.JSXElementConstructor<any>.

A primary benefit of adapting to standard interfaces is reduced type maintenance over time. I haven't tested it, but also noticed that Issue #991 may be resolved by this change as well since union prop types should be supported by these standard interfaces.

Note: Upon running yarn test:typescript with a clean build from master, I encountered TypeScript errors related to React.ReactNode's use of a union with both null and undefined included. I have suppressed the error by turning off the no-null-undefined-union flag in tslint; however, you may prefer a different approach.

Checklist:

  • Documentation N/A
  • Tests
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Jun 18, 2019

🦋 Changeset is good to go

Latest commit: 30707a0

We got this.

Not sure what this means? Click here to learn what changesets are.

Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

Superb work! Love such PRs ❤️

@emmatown emmatown merged commit c673e20 into emotion-js:master Jun 20, 2019
@ryanswanson
Copy link
Contributor Author

Thank you @Andarist and @mitchellhamilton for the quick turn-around on this. @ahutchings and I spent some extra time in making the PR comprehensive hoping it would be easier to review and accept.

Superb work on emotion as well! Love such libraries ❤️

@ryanswanson
Copy link
Contributor Author

@Andarist - Do you mind sharing when you expect this to make it into a release and what semver level it will likely be?

@Andarist
Copy link
Member

Not sure, @mitchellhamilton is doing releases - usually, it doesn't take too much time for a new release to be prepared though.

I think it should qualify as patch/minor - we definitely don't consider typing changes to affect semver in the same way as runtime changes affect it.

@ryanswanson
Copy link
Contributor Author

Great! Thank you

louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
* PropsOf<C> TypeScript utilities - Support defaultProps by delegating to new JSX and React types.

* changeset

* Regenerated yarn.lock without internal urls

* Align versions of @types/react across project by upgrading @emotion/core and root package versions.
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.

None yet

3 participants