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.

Sign up for GitHub

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

Jump to bottom

StyledComponent infer type from shorthand #3767

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

StyledComponent infer type from shorthand #3767

wants to merge 1 commit into from

Conversation

penx
Copy link
Contributor

@penx penx commented Mar 6, 2020 •
edited

Fixes #3766

When a shorthand is called as:

import styled from 'styled-components';

export const Red = styled.div`
  border: 1px solid red;
`;

We can infer its type as StyledComponent<{}, {}, HTMLDivElement>.

This is currently a WIP. I'm fairly new to flow so some direction would be appreciated cc @omninonsense @jbrown215 @goodmind @AndrewSouthpaw

  • maybe a more extensive solution is possible?

e.g. this doesn't currently infer a type for:

const RED = 'red';

export const Red = styled.div`
  border: 1px solid ${RED};
`;

or

export const Red = styled(MyComponent)`
  border: 1px solid 'red';
`;

But both could be inferred AFAIK

  • I'm not sure how to test for this, as I can't write a failing test without using export which isn't possible inside a test.

@penx penx changed the title StyledComponent infer type from shorthand #3766 StyledComponent infer type from shorthand Mar 6, 2020
omninonsense
omninonsense suggested changes Mar 7, 2020
View reviewed changes
definitions/npm/styled-components_v5.x.x/flow_v0.104.x-/styled-components_v5.x.x.js
Comment on lines +251 to +253
[[call]]: (
string[]
) => StyledComponent<{ ... }, { ... }, V>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! Thanks for helping with this!

At first glance it seems okay. I would maybe put the more specific overload first, and this one after it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think you could maybe add a special interpolation type that excludes functions, so people can still use interpolation inside the templates. Something like this:

    [[call]]: (
      string[],
      InterpolationBase[]
    ) => StyledComponent<{ ... }, { ... }, V>,

but from the top of my head, I am not sure if that will cause issues with inference, though

P.S: Maybe even accepting functions that don't accept any arguments could be added, too.

Copy link
Contributor Author

@penx penx Apr 9, 2020 •
edited

Choose a reason for hiding this comment

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

@omninonsense sorry for the delay, I've tried to pick this up again but am getting a bit lost

I would maybe put the more specific overload first

if I put lines 251-253 lower down then the overload currently on line 254 is matched first and flow complains about "Missing type annotation for Theme, StyleProps:.

a special interpolation type that excludes functions

I think you meant to add a ...in? Like this?

    [[call]]: (
      string[],
      ...InterpolationBase[]
    ) => StyledComponent<{ ... }, { ... }, V>,

Also, for some reason I've tried various combinations of the above (including my original code) but am still getting this error:

Missing type annotation for V. V is a type parameter declared in function type [1] and was implicitly instantiated at
$ObjMap [2].

     417│     ElementName
     418│   >;
     419│
 [2] 420│   declare type ConvenientShorthands = $ObjMap<
     421│     BuiltinElementInstances,
 [1] 422│     <V>(V) => StyledShorthandFactory<V>
     423│   >;
     424│
     425│   declare interface Styled {
     426│     <Comp: React$ComponentType<P>, Theme, OwnProps = React$ElementConfig<Comp>>(

maybe you have time to check the branch out or for a quick call?

Copy link
Contributor Author

@penx penx Apr 9, 2020 •
edited

Choose a reason for hiding this comment

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

Hopefully this is recreating the error I'm seeing, will try to debug now:

https://flow.org/try/#0PTAEAEDMBsHsHcBQBLAtgB1gJwC6gEoCmAhgMZ6RayqgDkWJ5tA3IojgJ7qGgDKn0QgBNeAC2w5RxAHZCAYmRzYOAHgBqAPlABeUAAoAlDq0yOrTt1ABRAB44sisRKmydodRr1qj2rfw6CIuK4LvKKyh6s7Fw8AEIArsjQOMjSVoKohNI4AJLSAM44MqSE+W4A3oig1aBCyABuAFwEjDgAJOmEmdlEkCq0dfW0GogAvmwWPADCsNL1WchZOE4hMkJlum0A8gBGAFYAssToKlU1CUkpaRlLeYXFpQA0Z9W29o7BkmtnGlGks4VQIUAsJmjM5gslisvrINqBKgBIQbNQzGdyDUDAEbjRCEGyYXCgf4FPAHDgAEQabmBgQAdINWEA

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! On line 20 of the try-flow link, you forgot to call it (also tweaked the function signature to resemble a template literal, so flow doesn't complain about that then):

https://flow.org/try/#0PTAEAEDMBsHsHcBQBLAtgB1gJwC6gEoCmAhgMZ6RayqgDkWJ5tA3IojgJ7qGgDKn0QgBNeAC2w5RxAHZCAYmRzYOAHgBqAPlABeUAAoAzjizJpAcwDaAXQA0oAHSOZHawEodW5607dQAUQAPY0UxCSlZHVB1DT01d20tfg5BEXFccPlFZWjWdi4eACEAV2RoHFM-QVRCaRwASWkjGVJCA0iAb0RQbtAhZAA3AC4CRhwAEkrCatqiSBVaPv7aDUQAXzYfHgBhWGl+muQanFD0mSE23TGAeQAjACsAWWJ0FS6e4tLy6Unp+sacZqtGxvbqBYLkE6SM5vDS5Ui7IygIzJYTDHZ7A5HSEZC6gToASEWwz08S0KkWoGAK3WiEIAUwuFA8P+oAeHAAIgNIsiUvZFgADfmsIA

Copy link
Contributor

Choose a reason for hiding this comment

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

The error is still strange though, lmao.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant to add a ...in? Like this?

Yeah, I forgot the spread

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On line 20 of the try-flow link, you forgot to call it

Cheers, though this solves the issue on this simplified example I still have the same error in my branch. As you say, the error seems strange. I don't think this is intended so I've raised a bug against Flow.

facebook/flow#8344

@TheMightyPenguin TheMightyPenguin mentioned this pull request May 11, 2020
Closed
@techieshark
Copy link
Contributor

Without this, the simplest use of styled-components (that is: a div that takes no props) needs a manual type definition.

e.g.

styled.div`
  border: 1px solid red;
`;

With this patch, it appears to be properly inferred.

This isn't perfect (as noted by the author's comments) but it's an improvement, no? Should this be merged now and improved upon later?

I imagine Flow not being able to infer types for even "simple" styled-components usage is a show-stopper for many considering whether to use Flow or (if they've been using it for some time) whether or not to upgrade past 0.85 or so. (That's just my loose +1'ing for pulling this in if there are no drawbacks to doing so.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omninonsense omninonsense omninonsense requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Auto infer StyledComponent type from shorthand
3 participants
@penx @techieshark @omninonsense