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

fix: refactor styled-base Flow types to work again #1570

Merged
merged 8 commits into from Oct 27, 2019

Conversation

FezVrasta
Copy link
Member

@FezVrasta FezVrasta commented Oct 25, 2019

What:
work needed to fix #1515

Why:
right now styled components are typed as any

How:
refactored the Flow types of styled-base to make sure they return the proper types

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2019

🦋 Changeset is good to go

Latest commit: ff63490

We got this.

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

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link

codecov bot commented Oct 25, 2019

Codecov Report

Merging #1570 into master will not change coverage.
The diff coverage is 100%.

Impacted Files Coverage Δ
packages/styled/src/tags.js 100% <ø> (ø) ⬆️
packages/styled-base/src/utils.js 100% <ø> (ø) ⬆️
packages/styled-base/src/index.js 100% <100%> (ø) ⬆️

@FezVrasta FezVrasta added the WIP label Oct 25, 2019
@Andarist
Copy link
Member

What do you mean by "part of the work needed to fix #1515"? What has to be done to consider that ticket done other than this PR?

@FezVrasta
Copy link
Member Author

I still got “any” out from the styled package, but maybe it’s just some local issue with the monorepo

@Andarist
Copy link
Member

I still got “any” out from the styled package, but maybe it’s just some local issue with the monorepo

Could you try building & linking packages to your repro case? That way you could check how this would behave if this would get published

@FezVrasta
Copy link
Member Author

I updated the types, now StyledComponent is a polymorphic type (like React$StatelessFunctionalComponent) so that you can use it as follows:

type Props = { color: string };
const Foo = styled<Props>`
  color: ${props => props.color};
`;

This has the advantage that it makes it easy to catch errors inside your styled components, but has the disadvantage of requiring the type in all cases.

I would like to make it optional, so that it fallbacks to any, but I can't seem to figure out how to do it. Ideas?

}

export type StyledComponent<P> = React$StatelessFunctionalComponent<P> & {
defaultProps: P,
Copy link
Member

Choose a reason for hiding this comment

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

is this correct? shouldnt Partial<P> equivalent from flow be used here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah probably this should be $Shape<P>

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't unable to use $Shape<P> because that would then get assigned to P and throw a type error.

It looks like Flow doesn't support defaultProps, I typed it as any for now :-(

facebook/flow#1660

@Andarist
Copy link
Member

I would like to make it optional, so that it fallbacks to any, but I can't seem to figure out how to do it. Ideas?

Isn't requiring type at all times (as its hard to infer it) better than falling back to any?

@FezVrasta
Copy link
Member Author

FezVrasta commented Oct 26, 2019

Isn't requiring type at all times (as its hard to infer it) better than falling back to any?

Yes it's absolutely better, but it's kind of a breaking change, because now all the untyped styled components will require the annotation.

Also, if I don't expect to assign any property to my styled component and I plan to use it only internally of some larger component I may not want to define a type for each of them, like it happens when I define a internal component props => <div {...props} /> and Flow simply infers its type rather than asking me to define it.

I should have probably said that I'd like it to fallback to inference, rather than to any.

@FezVrasta
Copy link
Member Author

FezVrasta commented Oct 26, 2019

I guess I hit a road blocker... Flow doesn't seem to support type parameters on tagged template strings 😢

So that means we aren't able to do:

const Foo = styled.div<Props>`
  color: red;
`;

facebook/flow#7527

We could still merge this PR if we at least found a way to make styled('div')({}) work without type annotations (fallbacking to inference, or any).
If we made it work, then we could just say that in order to define explicit props types to the styled components users can't use template literals because of this Flow limitation.

But without this, the tagged template usage is completely unusable.

@Andarist
Copy link
Member

@FezVrasta let me know when you are done with this PR - I'll do a code review then and we'll be able to merge this in

@FezVrasta
Copy link
Member Author

I don't understand why if I write the following code in styled-base/src/index.js Flow is happy about the missing function type annotation, but if I do the same in a different file it complains :-(

const styled = createStyled('div')

// OK
const Ok = styled<{ color: string }>({ color: props => props.color })

const Ko = styled({ color: props => props.color })

@FezVrasta
Copy link
Member Author

Oh damn it, Flow is right...

The test fails because I'm trying to export styled({ color: props => props.color }), if I just assign it without exporting it everything works fine.

@FezVrasta FezVrasta removed the WIP label Oct 27, 2019
@FezVrasta
Copy link
Member Author

FezVrasta commented Oct 27, 2019

Ok @Andarist I think we are ready for a final review.

Notable unresolved/unaddressed issues:

  1. Flow doesn't support generics on tagged templates
  2. I haven't added type checking on the ${props => props.color} calls inside CSS declarations (I don't know how they are called)

Breaking changes (?):

  1. You need to define the styled component type if you want to export it, previously (being any) this wasn't needed*;

*: I'm not sure we should consider this a breaking change, is more like a fix for something that wasn't working properly but should never have been allowed in first place.

@Andarist Andarist merged commit 9767309 into emotion-js:master Oct 27, 2019
@Andarist
Copy link
Member

@FezVrasta thanks!

@github-actions github-actions bot mentioned this pull request Oct 27, 2019
@bradringel
Copy link

So, this change has caused our flow error count to jump up to around 800 after getting this version bump in our project. Any suggestions for how to adopt this type of change in a large project. We're also using emotion-theming, so a lot of the times the props for our styled components come from the theme context, and many of our styled components are created with the tagged template literal version of styled. This was a rough change to introduce in a patch version bump

@Andarist
Copy link
Member

@bradringel could you reduce your failing call sites to small repro cases? when fixing those we could include them as "flow tests".

@bradringel
Copy link

@Andarist sure. A super basic one like this fails:

//@flow

import styled from '@emotion/styled';

export const UserName = styled.div`
  color: ${props => props.theme.colors.primary};
`;

Fails with the error
image

typing the component with any fixes it, but that's a lot of components to go add any to:

//@flow

import styled from '@emotion/styled';

export const UserName:any = styled.div`
  color: ${props => props.theme.colors.primary};
`;

doesn't have an error

@Andarist
Copy link
Member

Isn't the problem here that previously such component had "props" typed to any implicitly and it was type-unsafe to use "props"? The situation here, even if it causes more flow errors, gets improved in this regard. This could be treated as a bug fix.

@bradringel
Copy link

That's true, it was implicitly unsafe, but now we need to go and fix 800 of these types of errors. And because flow doesn't support generics for tagged template literals, which is how a lot of those components are written, they're all just going to get typed to any.

This is technically a safer way to write those components, but is a big change to the way we need to interact with this library, and was introduced in a patch version.

@armount
Copy link

armount commented Nov 20, 2019

Let me preface this by saying we are fans of Emotion - we use it all over our project. So, please don't take this as an attack, I'm just looking for advice for suggested usage in Flow situations. I could be missing something really easy (it wouldn't be the first or last time), but it feels like Flow users are in for a wild time if no version of tagged templates is supported.

Is your position that we should be doing this instead?

// some utility file having these types
export const Div = styled('div');
export const Span = styled('span');

// some other file
export const MyComponent = Div<Props>({ ... props based object literal here... });

Our whole project uses both of the tagged template forms below (I realize, unsafe), and both do not pass flow checks if I try to type the 'styled' call with a generic. We export them because we like to separate all of our styled components which form our 'building blocks' from the complex components that use them instead of lumping them together in the same file. Using the tagged template versions of these calls let us write css-like implementations instead of JSON-like structures, which was also really appealing:

export const MyComponent  = styled('div')` ...css style props here...`;
export const MyComponent = styled.div` ...css style props here...`;

I feel like the answer, "yes, you can use tagged template forms of the calls but you will be forced to type them all to keep Flow from complaining" is a lot different than "you can't use this supported feature even if you type it unless you want to live with Flow complaining until they support tagged templates". The former is a lot less work than the latter, even if both are time consuming to some degree in a large project. That's where I agree with @bradringel that this feels like a major change, not a minor version bump. I think I'll wait to hear what you suggest and pin to the previous version in the meantime. Based on the answer, we'll plan to make these changes as part of tech debt after our release deadline is met.

Thanks for taking the time to read and respond. I do not imagine being a maintainer of a popular OSS project is easy - much respect.

@FezVrasta
Copy link
Member Author

I made some further improvements to the Flow types in this PR #1588

It’s currently in the next branch.

I’m from mobile but once I get back on my computer I’ll try to provide a more complete answer

@Andarist
Copy link
Member

I see that on the next branch this is supported:

const root: typeof Foo = styled<Props>('div')`
colors: red;
`

So you should be able to work around Flow not accepting generics on tagged template calls. I'm not sure if this supported with types on master though, could you check this for me?

I understand the inconvenience caused by this and I'm truly sorry for that. I believe that it's not practical to tie types to semver closely though (when it makes sense to use we try to do that though, in example we've pushed TS types refactoring to next so it doesn't affect latest releases). It's just super hard to do - with 2 type systems that evolve over time and make breaking changes between their versions.

That's true, it was implicitly unsafe, but now we need to go and fix 800 of these types of errors.

If those were previously unsafe and typed to any I could help you writing a codemod that would just make those explicit in your code.

louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
* fix: refactor styled-base Flow types to work again

* fix: improve Styled type to accept optional props type

* fix: make styled package return proper types

* fix: missing flow header

* test: test untyped styled component

* chore: fix site types

* docs: comment typo
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.

styled components typed as "any"
4 participants