Convert components package to TS

[PaulieScanlon]

Hey all, here's a first "incomplete" pass at converting the components package to TypeScript.

I'm afraid i've run out of "talent" in a number of places so have left TODO comments where i'm not able to resolve the TypeScript errors.

Could someone perhaps give me some pointers and i'd be happy to give it another shot!

Thanks

[hasparus]

Hey @PaulieScanlon, I'll take a look at your errors. Can I open a PR targeted at your fork? Or do you have any questions / problems I could help you with?

[PaulieScanlon]

@hasparus hey!

Most of the issues I’m struggling with relate to the ref.

You’ll probably see the TODO comments next to the ones I couldn’t work out.

You can open PR on my fork no worries!

Thanks for stepping in, I appreciate it!

[hasparus]

Hey, sorry it took so long. I started working on today and I know what's wrong.

I usually don't encounter these problems, because I don't use refs much. ComponentPropsWithoutRef would save us some of the trouble, but this isn't a plausible decision for a library. (I'm using @theme-ui/components with react-hook-form in one app :D)

Okay, if Box was declared as function, we could make it generic, make the props generic and infer what the ref should be. The Box is a styled component and it leads to few problems:

We can't make it generic easily. We'd have to build on top of StyledComponent type to add another generic parameter.

I see you've added forwardRef in SVG. I think we should try to add TS to existing code without adding new runtime features. Especially as this introduces complexity at type level.

---

I believe we have a regression in comparison with @types/theme-ui__components.
I've copied the test from DT and manually tested autocomplete and inference.
DX would need to be tested. Component types are public API, and may crash user's CI on breaking change.

I mentioned in #121 that few packages here would be very easy to convert.
This is not one of them :D JSX handling is a tricky part of TypeScript and styled API types are insanely complex (explicit composition with css and sx prop FTW).

A small example what happened when I tried to fix one of the errors:

I'm working around it with a small hack:

---

I wonder where do we stand between prioritizing ease of development vs user DX.
I'm not sure how mature is theme-ui considered to be by the team at Gatsby.
(cc @mxstbr @jxnblk)

I'll send you a PR today @PaulieScanlon, but I think this will need some more work from us, especially in tests.

[hasparus]

I'll leave these here as a note:

• https://github.com/typescript-cheatsheets/react-typescript-cheatsheet/blob/master/ADVANCED.md#typing-a-component-that-accepts-different-props
• https://blog.andrewbran.ch/polymorphic-react-components/

[hasparus]

Isn't this a bug? css is passed to styled component without touching theme. Shouldn't this be __css?

cc @jxnblk

https://github.com/system-ui/theme-ui/blob/master/packages/components/src/Box.js#L13-L30

Edit: Yeah. It gets emitted to css.

a snapshot

.emotion-0 {
  box-sizing: border-box;
  margin: 0;
  min-width: 0;
  font-family: heading;
  font-weight: heading;
  line-height: heading;
  font-size: 32px;
}

<h2
  className="emotion-0"
/>

[PaulieScanlon]

Hey @hasparus thanks for looking into this... what would you say is next step is? I'd like to help out but you lost me at the forward ref generics.

Is there anything i can be investigating or is there a bigger conversations that needs to happen about this package?

[hasparus]

The ForwardRef generic could be probably dropped tbh. TS would infer proper types if we annotate component props and ref. The problem is, components with "as" prop are tricky to type. We should write some tests to check their public api on type level (inference of onClick event would be perfect, not sure if we can achieve it with Box).

We could change "as" prop usages to withComponent, to get rid of the errors, but I’m not sure if we want to, as this would have a small runtime cost. We’d probably be fine with few as any assertions.

[jxnblk]

Good catch! This does look like a bug, e.g. css prop should be __css

[mrmartineau]

Just checking if there is any movement on this PR? what needs to happen, apart from the conflict, to get this merged?

[hasparus]

@mrmartineau I'm adding types "the easy way" in this commit d06b4c2. I expect this might get merged before we tackle problems in this PR the hard way.

I suppose current state of our work on TS adoption in @theme-ui/components is in my PR to Paullie
PaulieScanlon#1. The blocker is -- @emotion/styled doesn't support as prop in TypeScript.

I drafted an idea for further work in #703 (comment). I'll copy it here for convenience.

This blocks TS adoption in @theme-ui/components (Paulie's #692 and my PaulieScanlon#1), and I'd really like to remove @types/theme-ui__components from DT, because it will be bad for my email inbox in the long term :D

Loose idea:

How to fix the as prop problem in @emotion/styled-base

1. Fork emotion
   - TypeScript typedefs for @emotion/styled-base are in emotion/packages/styled-base/types/index.d.ts. Last commit to the file was 8 months ago :D
   - Change required TS version to 3.5
   - The logic for as prop is in emotion/packages/styled-base/src/index.js#L86
2. Hack a proof of concept
3. Contact Andarist or someone else from Emotion team to speak about it. This would be polite, because we require higher TypeScript version. I have a bad feeling that all changes to emotion styled typings may be breaking for TS users (like all my apps at work ouch).
4. @emotion/styled-base is currently tested with dtslint. I am not sure if it's sufficient. Do we need tsd or even ts-snippet? We'd like to assert that the props are inferred properly, because we kinda want to test "developer experience".
5. Create a PR for Emotion

I would like to work on this, because TS Emotion support for as prop bugs me also outside of theme-ui, but I'm a bit occupied lately so the status is IMHO "need help".

[jxnblk]

@hasparus would removing the @emotion/styled dependency and creating the components in this library using "plain old" React help out with the issue you're running into? If so, I'd say it's something we should consider

[mrmartineau]

@hasparus thanks for the clarification. There's a lot to consider.

@jxnblk your suggestion makes sense. Would removing@emotion/styled reduce the package bundle size? or would it still be needed elsewhere?

[hasparus]

@hasparus would removing the @emotion/styled dependency and creating the components in this library using "plain old" React help out with the issue you're running into? If so, I'd say it's something we should consider

yes

If we define Box as a function and instead of using styled HOC to build Grid and Form we just spread props onto Box in their definitions, we'll certainly work around some complexity.

I'll give a shot this @emotion/styled-base fix today after work to at least estimate how much time it would take.

[hasparus]

Okay, I did some research. New info:

There is a closed PR from a week ago emotion-js/emotion#1796 adding the as prop support. It introduced a regression. This is basically what I've been afraid of. @emotion/styled-base types are humongous (styled-base/types/index.d.ts) and I've had problems with them reaching TypeScript limits previously.

They're simplified on the next branch for new major version of emotion, but they'll still be big. styled just has a pretty big public API: template strings, interpolation, component selectors.

This leads to a choice. We can either

1. Internally use styled and withComponent instead.
   This is what I've had TS limits problems at work. Whenever it happens, we rewrite components using these HOCs to functions spreading props to fix it.
    
   We could then use assertions to enforce our will on TypeScript and type public API by hand.
   We know that works, we can unit test it, we can argue with TS a little.

I assume we would like to support as prop in TypeScript for theme-ui users.
It's useful for Box, but it is pretty much required for Field.

BTW It already works for Field in DefinitelyTyped. Not sure if I mentioned that.

2. Don't use styled HOC as @jxnblk mentioned

• as, sx and variant props seem like a nice squad for conventional UI composition in JSX.
  This is a really nice public API.
• as prop is a little heavy on TypeScript as I previously mentioned, but there is some weight in styled we can drop. This is also true for other complex HOCs, but they are getting less popular nowadays.
  • I experienced that the worst thing happens when one combines as with styled like this styled(Field).
  • This would need to be thoroughly tested and benchmarked regularly, maybe with a bot in PRs.

[ivoreis]

Hi, @hasparus any luck solving that emotion-styled issue? Does this need any technical implementation on the emotion-js side of is just a matter of having the types up-to-date?

The emotion-js/emotion#1796 PR got closed for now so I don't think we'll be getting it anytime soon.

Have we considered the option of porting over https://github.com/emotion-js/emotion/blob/master/packages/styled-base/types/index.d.ts into theme-ui and tweak it? The downside would be that the versions would diverge but considering that styled-base types haven't changed that much over the last 2 years this could be a low-risk situation unless v11 brings major ts improvements or making it a first-class citizen.

Considering that emotion and styled-components have quite similar API, could we extract the missing features from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/styled-components/index.d.ts#L89 and bring in-house?

Any thoughts?

[hasparus]

Hey @ivoreis

Have we considered the option of porting over https://github.com/emotion-js/emotion/blob/master/packages/styled-base/types/index.d.ts into theme-ui and tweak it?

@jxnblk mentioned it in #692 (comment)

would removing the @emotion/styled dependency and creating the components in this library using "plain old" React help out with the issue you're running into? If so, I'd say it's something we should consider

@dburles entirely got rid of all of our problems with styled and withComponent in the New Variant API PR https://github.com/system-ui/theme-ui/pull/823/files#diff-32c732f0a1fc783cfd11a724d53fc091R4. ❤

This will greatly simplify any TS related work here, so I suppose we just wait until #823 is merged and the only thing we need to type is as prop similarly to how it's implemented in styled-components, react-hook-form and Reakit.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/cptpjsndv
✅ Preview: Failed

[atanasster]

Great PR, looking forward to having native typescript implementation, but can I suggest we start converting components to ts after merging #1368 as there will be significant changes

[lachlanjc]

This would still be amazing to do, but since we let this PR go stale there’s quite a lot of merge conflicts now. So sorry for this sucky experience @PaulieScanlon—if you’re still interested we’d still love this work to happen. Really appreciate your time.