Add type-defs for styled-components v4 #2933

omninonsense · 2018-11-01T17:01:19Z

I'd like someone to thoroughly review this!

To-do:

Add types for styled-components/native (help wanted)
Add KeyFrames to Interpolation
Add CSSRules(Interpolation[]) to Interpolation
Improve tests for css (and maybed keyframes?); add comments too.
Add tests for refs
Add tests for stuff that doesn't exist (ie styled.derp/styled('derp'))
Set lower bound to flow 0.75!
Add support for attrs API

These are currently impossible (to the best of my knowledge) to express these safely and without breaking the types for the "main" functionality of the library, due to Flow's lack of support for some core React features (React.forwardRef) this library relies on:

Add support for withCompnent API
Add support for as prop API
Add support for custom props on built-in components (ie <MySpan myProp=... />)

Please let me know if you can think of anything else to add to the list.

Also, not sure if it's wise to simply merge this (if the PR is good enough), and then hope someone else contributes with whatever I missed?

omninonsense · 2018-11-01T17:04:35Z

This should hopefully address #2880

omninonsense · 2018-11-01T17:05:02Z

Also, the justification for the horribly misleading StyledElementType<T> = T type is to make it seem like it's built-in elements, and to hide the fact (to IDEs) that we're lying about the returned type being a string. Yikes! I feel really bad about this. I hope someone knows of a better way.

Also, it's added so it works with forward refs (v4 doesn't use innerRef anymore). I forgot to add tests for refs; I'll add them later or tomorrow

omninonsense · 2018-11-01T17:24:47Z

Damn you, Travis! Doesn't seem to be related to this PR, since master is also failing in the same ways. 🤷‍♂️

omninonsense · 2018-11-02T18:13:58Z

Travis is back! 🎉

AndrewSouthpaw · 2018-11-02T21:00:01Z

I suggest you tag a few people who have worked on this libdef already. :-)

omninonsense · 2018-11-05T12:14:25Z

I suggest you tag a few people who have worked on this libdef already.

@AndrewSouthpaw Good idea!

Hi @dashed, and @shrynx! Would you two like to take a look at this too, please? 🐈

zaguiini · 2018-11-06T19:30:06Z

Hi! I'm trying to use it but with no success. Successfully installed as custom definitions but I'm getting errors.

How should I use it, @omninonsense?

omninonsense · 2018-11-07T15:09:09Z

Hi @zaguiini!

So, the way you're trying to call the function (EDIT: only talking about the type arguments in the call expression) won't work with this libdef (I don't remember if this works for v3). The main reason is that we lie about the type that's returned by styled in order to compensate for the fact Flow currently doesn't have a good way of correctly typing React.forwardRef()s (see facebook/flow#6103), which is something styled-components started using in v4 instead of innerRef.

What we essentially do is:

for built-ins like styled.div, we say that it returns a string ("div"), but we hide/wrap this lie in StyledElementType<T>
for components we say that we're returning that same component type

This is not ideal, but it means that we have working refs, and prop validation is preserved for the non built-in components.

Once forward ref support lands in flow, these can be improved a lot, to even add custom props accepted by the styled component (which the wrapped component ignores, etc), and maybe even properly typing the functions inside the template literals. Although, I think flow doesn't really understand template literals expand to fn(strings: string[], ...interpolations : Interpolation[]), so I didn't bother too much with having Interpolation accept a prop parameter in there.

omninonsense · 2018-11-08T18:01:27Z

@zaguiini I never answered you other question. In the example you provided, I'd just do:

type Props = {
  a: boolean;
}

const CardContainer = styled.div`
  /* ... */
  background-color: ${(props: WithTheme<Props>) => props.theme.light.white};
  /* ... */
`

I usually have a theme.js file with something like this (this is where the WithTheme<> mentioned above comes from):

export const theme = {
  /* ... */
}

export type Theme = typeof theme
export type WithTheme<P : {} = {}> = P & {| theme: Theme |}

dashed · 2018-11-08T19:11:10Z

Hi @dashed, and @shrynx! Would you two like to take a look at this too, please? 🐈

@omninonsense At the moment, I don't have a project currently using styled-components v4 + flow types; and unfortunately, I won't have the bandwidth to review this PR anytime soon.

Awesome job though! 👍

maamounapprise · 2018-11-12T17:52:29Z

you would need to add for macro too

AndrewSouthpaw · 2018-11-14T19:43:22Z

If we don't get a PR from people with more context within a week, I'm okay with merging it in (after reviewing with little context) and letting to be "better than nothing," and people can continue adding fixes as needed. @omninonsense if this stagnates for too long just ping again and I'll merge it in.

iduuck · 2018-11-16T10:32:26Z

I have a problem when using the as prop for the new styled-components version. I have a case, where I style the react-router-dom's own Link. After declaring it, I want to have a link to Instagram, but not with Link, but with a a tag.

<ListLink as="a" target="_blank" href="https://instagram.com/fintory">
  Instagram
</ListLink>

But now, I am getting an error, that I am not supplying the right Props:

Cannot create ListLink element because property to is missing in props [1] but exists in object type [2].

     client/components/Footer/index.js
     128│                   <ListLink as="a" target="_blank" href="https://www.facebook.com/fintory.agency/">
     129│                     Facebook
     130│                   </ListLink>
 [1] 131│                   <ListLink as="a" target="_blank" href="https://instagram.com/fintory">
     132│                     Instagram
     133│                   </ListLink>
     134│                   <ListLink as="a" target="_blank" href="https://twitter.com/fintory">
     135│                     Twitter
     136│                   </ListLink>

     flow-typed/npm/react-router-dom_v4.x.x.js
 [2]  20│   declare export class Link extends React$Component<{
      21│     className?: string,
      22│     to: string | LocationShape,
      23│     replace?: boolean,
      24│     children?: React$Node,
      25│   }> {}

zaguiini · 2018-11-16T11:50:43Z

I have a problem when using the as prop for the new styled-components version. I have a case, where I style the react-router-dom's own Link. After declaring it, I want to have a link to Instagram, but not with Link, but with a a tag.

<ListLink as="a" target="_blank" href="https://instagram.com/fintory">
  Instagram
</ListLink>

But now, I am getting an error, that I am not supplying the right Props:

Cannot create ListLink element because property to is missing in props [1] but exists in object type [2].

     client/components/Footer/index.js
     128│                   <ListLink as="a" target="_blank" href="https://www.facebook.com/fintory.agency/">
     129│                     Facebook
     130│                   </ListLink>
 [1] 131│                   <ListLink as="a" target="_blank" href="https://instagram.com/fintory">
     132│                     Instagram
     133│                   </ListLink>
     134│                   <ListLink as="a" target="_blank" href="https://twitter.com/fintory">
     135│                     Twitter
     136│                   </ListLink>

     flow-typed/npm/react-router-dom_v4.x.x.js
 [2]  20│   declare export class Link extends React$Component<{
      21│     className?: string,
      22│     to: string | LocationShape,
      23│     replace?: boolean,
      24│     children?: React$Node,
      25│   }> {}

You forgot to add the to prop. Make it optional if you don't want to pass it.

omninonsense · 2018-11-16T15:16:55Z

@iduuck Try doing what @zaguiini suggested. I don't think this is related to styled-components typings directly (could be wrong). I wasn't aware of the as prop mechanism in styled-components.

zaguiini · 2018-11-16T15:19:35Z

@iduuck Try doing what @zaguiini suggested. I don't think this is related to styled-components typings directly (could be wrong). I wasn't aware of the as prop mechanism in styled-components.

There is indeed one "as" mechanism: https://www.styled-components.com/docs/basics#extending-styles

omninonsense · 2018-11-16T15:22:57Z

Hi @AndrewSouthpaw

If we don't get a PR from people with more context within a week, I'm okay with merging it in (after reviewing with little context) and letting to be "better than nothing," and people can continue adding fixes as needed. @omninonsense if this stagnates for too long just ping again and I'll merge it in.

I have some updated typings that I need to push, but got busy/distracted by work. I will probably get back to adding some updates early next week after cleaning them up and adding tests to try and add some of the missing APIs (if possible without breaking the typings for the "primary functionality" of the library).

omninonsense · 2018-11-16T15:25:15Z

@iduuck Try doing what @zaguiini suggested. I don't think this is related to styled-components typings directly (could be wrong). I wasn't aware of the as prop mechanism in styled-components.

There is indeed one "as" mechanism: https://www.styled-components.com/docs/basics#extending-styles

Added to the checklist! :)

omninonsense · 2018-11-19T12:41:31Z

I decided to give up on withComponent, attrs APIs and the as prop API for now; there is no way to add them without breaking more important stuff (or adding them at all).

I'll look into adding the react native stuff later today or tomorrow.

omninonsense · 2018-11-22T17:37:48Z

I think this is ready-ish for review, @AndrewSouthpaw. At least until we get better support for better typing from @facebook/flow

SpainTrain · 2018-11-28T23:07:49Z

For tracking React feature support in flow:

AndrewSouthpaw

Generally looks good. Unless there's a strong need, I'd collapse the separate test files into one and scope with describe/it blocks.

omninonsense · 2018-12-04T13:59:36Z

Generally looks good. Unless there's a strong need, I'd collapse the separate test files into one and scope with describe/it blocks.

Will do! I only split them up because I assumed it would make them easier to read/understand. 😃

omninonsense · 2018-12-04T14:45:07Z

@AndrewSouthpaw I used a few (random) other libdefs to see how to structure it. Is it okay like this?

omninonsense · 2018-12-05T16:47:14Z

Had a random epiphany that attrs can be supported.

omninonsense · 2018-12-05T16:49:10Z

Forgot about styled-components/native. 👀

Sorry about the noise!

omninonsense · 2019-01-22T16:49:42Z

@egeriis Yes, but see #2933 (comment) about flow not fully supporting template literals.

egeriis · 2019-01-22T16:54:04Z

@omninonsense Yeah, I read it. Mind slip that I didn't understand it would affect being able to do this.

omninonsense · 2019-01-22T16:57:17Z

I think TypeScript supports something along these lines:

const MyDiv: StyledComponentType<'div', Props> = styled.div``;

It would be nice if we could support this too someday.

egeriis · 2019-01-23T14:50:21Z

@omninonsense Idea of effort required to fix the CI errors?

This allows HOCs can be chained without losing type information

Co-Authored-By: theshortcut <cferris@gmail.com>

omninonsense · 2019-01-23T15:49:36Z

I haven't actually looked at fixing it.

I rebased and pushed. Hopefully that fixes it. 🤷‍♂️

(Didn't mean to close, I misclicked, I am stupid)

egeriis · 2019-01-25T09:50:25Z

@omninonsense It didn't work ;)

pascalduez · 2019-01-26T12:36:20Z

Tests are failing for older versions of the typdef, mostly due to recent Flow changes.
Given it's been tested by some users here and all the work already been done, I would merge this. Opening the path for a follow-up on Flow > 0.75, okay for you @AndrewSouthpaw?

egeriis · 2019-01-28T10:00:57Z

Which Flow versions is this libdef compatible with?

goodmind · 2019-01-28T19:18:52Z

facebook/flow#3936

AndrewSouthpaw · 2019-01-29T22:18:52Z

Seems reasonable to me!

AndrewSouthpaw · 2019-01-29T22:19:18Z

Also, would just like to say, way to go @omninonsense on your commitment on this PR.

kogai mentioned this pull request Nov 2, 2018

[react-loadable] Make type application of ReactLoadable.Map more precisely #2934

Merged

gantoine added the work in progress label Nov 11, 2018

retyui mentioned this pull request Nov 18, 2018

Flow doesn't return error with styled-components #2963

Closed

guyfedwards mentioned this pull request Nov 26, 2018

chore: styled-components@3.4.10 weaveworks/ui-components#426

Merged

AndrewSouthpaw requested changes Dec 1, 2018

View reviewed changes

Nino Miletich and others added 13 commits January 23, 2019 15:48

add type-defs for styled-components v4

23fdfb1

styled-components: add CSSRules and KeyFrames to interpolations.

7ac174f

styled-components: test for missing elements

25f28ee

styled-components: forgot a few @flow comments

88ae226

styled-components: fix tests

cb4d7c8

styled-components: lower required flow version to 0.75

8df94e7

styled-components: add ref tests

eff49e1

styled-components: add styled-components/native libdef

42355cb

styled-components: add (fickle) tests for styled-components/native

6a9024c

styled-components: collapse tests into one file

a47de2e

styled-components: add support for attrs API

74641de

styled-components: add $Supertype

3992601

This allows HOCs can be chained without losing type information

styled-components: attrs API also has a function form

2ab3495

Co-Authored-By: theshortcut <cferris@gmail.com>

omninonsense closed this Jan 23, 2019

omninonsense reopened this Jan 23, 2019

AndrewSouthpaw approved these changes Jan 29, 2019

View reviewed changes

AndrewSouthpaw merged commit 1a7d5ca into flow-typed:master Jan 29, 2019

TomPridham mentioned this pull request Feb 5, 2019

added failing test #3126

Closed

georgelima mentioned this pull request Feb 7, 2019

Feature/flow coverage ZcashFoundation/zepio#66

Merged

Add type-defs for styled-components v4 #2933

Add type-defs for styled-components v4 #2933

Conversation

omninonsense commented Nov 1, 2018 • edited

To-do:

omninonsense commented Nov 1, 2018

omninonsense commented Nov 1, 2018 • edited

omninonsense commented Nov 1, 2018

omninonsense commented Nov 2, 2018

AndrewSouthpaw commented Nov 2, 2018

omninonsense commented Nov 5, 2018

zaguiini commented Nov 6, 2018

omninonsense commented Nov 7, 2018 • edited

omninonsense commented Nov 8, 2018 • edited

dashed commented Nov 8, 2018

maamounapprise commented Nov 12, 2018

AndrewSouthpaw commented Nov 14, 2018

iduuck commented Nov 16, 2018

zaguiini commented Nov 16, 2018

omninonsense commented Nov 16, 2018

zaguiini commented Nov 16, 2018

omninonsense commented Nov 16, 2018 • edited

omninonsense commented Nov 16, 2018

omninonsense commented Nov 19, 2018

omninonsense commented Nov 22, 2018 • edited

SpainTrain commented Nov 28, 2018

AndrewSouthpaw left a comment

Choose a reason for hiding this comment

omninonsense commented Dec 4, 2018

omninonsense commented Dec 4, 2018

omninonsense commented Dec 5, 2018

omninonsense commented Dec 5, 2018

omninonsense commented Jan 22, 2019

egeriis commented Jan 22, 2019

omninonsense commented Jan 22, 2019 • edited

egeriis commented Jan 23, 2019

omninonsense commented Jan 23, 2019 • edited

egeriis commented Jan 25, 2019

pascalduez commented Jan 26, 2019

egeriis commented Jan 28, 2019

goodmind commented Jan 28, 2019

AndrewSouthpaw commented Jan 29, 2019

AndrewSouthpaw commented Jan 29, 2019

omninonsense commented Nov 1, 2018 •

edited

omninonsense commented Nov 1, 2018 •

edited

omninonsense commented Nov 7, 2018 •

edited

omninonsense commented Nov 8, 2018 •

edited

omninonsense commented Nov 16, 2018 •

edited

omninonsense commented Nov 22, 2018 •

edited

omninonsense commented Jan 22, 2019 •

edited

omninonsense commented Jan 23, 2019 •

edited