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(TS): allow CSSObject on @emotion/styled #1129

Closed
wants to merge 5 commits into from

Conversation

kentcdodds
Copy link
Contributor

What: This allows: styled.div(/* this can be an object /*)

Why: Because that API is supported so the typings should allow for it.

How: Tried it locally and it typed fine (even helped me fix an issue I had), so I just made this change in the github editor.

Checklist:

  • Documentation N/A
  • Tests
  • Code complete

@kentcdodds
Copy link
Contributor Author

Hmmm... Just noticed this test:

const Button1 = styled('button')({
color: 'blue'
})

I'm not sure how that's working because using the object syntax isn't working for me. I'm using styled.button rather than styled('button'), so maybe that's why...

@kentcdodds
Copy link
Contributor Author

The failure is:

Property 'button' does not exist on type 'CreateStyled'.

I'm not sure how to fix that 😬

@emmatown
Copy link
Member

styled.button and stuff should be in the @emotion/styled package

@kentcdodds
Copy link
Contributor Author

Ok, so I fixed the test, but then I added a few more tests for things that I think should be errors but they're not and I'm not sure why...

@Andarist Andarist requested a review from Ailrun December 21, 2018 08:24
@Ailrun
Copy link
Member

Ailrun commented Dec 21, 2018

@kentcdodds We cannot emit an error on "wrong properties", since we should handle something like & > &, & > li, ..., and TS does not support RegExp property type (which they said they will support).

@@ -72,7 +71,7 @@ export interface CreateStyledComponentBase<
ReactClassPropKeys
> = Omit<InnerProps & ExtraProps, ReactClassPropKeys>
>(
template: TemplateStringsArray,
template: TemplateStringsArray | CSSObject,
Copy link
Member

@Ailrun Ailrun Dec 21, 2018

Choose a reason for hiding this comment

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

This case is covered by the previous overloading, so you don't need to add one.
Interpolation<WithTheme<StyleProps, Theme>> is a (structural) supertype of CSSObject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... I wonder why I was seeing issues then. I'll dig a little more. Thank you!

@kentcdodds
Copy link
Contributor Author

Ah, I think this is the issue I was having that lead me to open this PR. Would you like me to make a new issue?


This is a weird issue and I don't know what's going on. Help appreciated!

Here's the code:

import styled from '@emotion/styled'

const WorkingDivider = styled.hr({borderTopStyle: 'solid'})

const baseStyles = {borderTopStyle: 'solid'}
const NotWorkingDivider = styled.hr(baseStyles)

const otherBaseStyles = {margin: 20}
const AnotherWorkingDivider = styled.hr(otherBaseStyles)

export {WorkingDivider, NotWorkingDivider, AnotherWorkingDivider}

Here's the error I'm getting:

$ yarn typecheck
yarn run v1.12.3
$ tsc
index.ts:6:37 - error TS2345: Argument of type '{ borderTopStyle: string; }' is not assignable to parameter of type 'TemplateStringsArray'.
  Type '{ borderTopStyle: string; }' is missing the following properties from type 'TemplateStringsArray': raw, length, concat, join, and 10 more.

6 const NotWorkingDivider = styled.hr(baseStyles)
                                      ~~~~~~~~~~


Found 1 error.

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

So from my observations, when I specify borderTopStyle in an object and pass it to styled, it'll fail typechecking with the above error. But if I inline the object it works. I have not observed this behavior with any other css property.

@Ailrun
Copy link
Member

Ailrun commented Jan 5, 2019

I think the issue you met is #1025. Please check my comments on the issue.
It's about how TS works...

@lychyi
Copy link
Contributor

lychyi commented Jan 8, 2019

@kentcdodds @Ailrun is correct, this is due to the inner-workings of TS (and exacerbated by an error messaging issue). Although, to be fair, I think #1025 makes is hard for some TypeScript users to decipher.

Let me try to explain this phenomenon because when I tripped up on this last time it was kind of hard to find a solution online. Hopefully this helps anyone else out:

Explanation

The difference in the working/non-working examples above is that WorkingDivider is an object literal passed as a function argument, thus its type is inferred correctly:

// parameter type is effectively { borderTopStyle: 'solid' }
// important here: value is a string literal type
const WorkingDivider = styled.hr({borderTopStyle: 'solid'})

vs.

// Notice the difference!
// type is { borderTopStyle: string }
// important here: value is a regular string type
const baseStyles = {borderTopStyle: 'solid'}
const NotWorkingDivider = styled.hr(baseStyles)

while in this case, we have an object literal assigned to a variable and then passed as a function argument. const baseStyles infers its type before it is passed into the styled function and the type is different because by default TS does type-widening on object literals. And because borderTopStyle only takes a specific values of strings (a string union type), it won't just accept string as the type. This makes sense, 'frizzy' or 'asdf' is not a valid borderTypeStyle so you want that type to be strict.

Finally, the last part of your example works because margin is designed to take any number or string (say, '20px'), so type-widening doesn't really affect it.

// type here is { margin: number } which is totally fine
const otherBaseStyles = {margin: 20}
const AnotherWorkingDivider = styled.hr(otherBaseStyles)

Solutions

This is my favorite way to fix it. Do the type inference via annotation:

import styled, { CSSObject } from '@emotion/styled';

// You could annotate the type to prevent widening
const baseStyles: CSSObject = { borderTopStyle: 'solid' }

I've seen this next solution get thrown around but it's difficult to maintain. i.e. What if you need to change from 'solid' to 'dotted'?

// Prevent type-widening with an assertion
// ...but if you had multiple properties that needed this, it could get tedious
const baseStyles = { borderTopStyle: 'solid' as 'solid' }

Other reading

@Andarist
Copy link
Member

Andarist commented Jan 8, 2019

@lychyi thanks for the explanation! I've learned something today because of your comment ❤️

@Ailrun
Copy link
Member

Ailrun commented Jan 8, 2019

@lychyi Thank you for such detailed explanation!
As one additional side note, it's not just about type widening, but also about how TS use context (other statements, other expressions, ...) to infer types (of variables, values, ...). Namely, it (almost) ignores context, and this is a critical difference with Flowtype.

@Ailrun
Copy link
Member

Ailrun commented Jan 8, 2019

@lychyi Oh, and in your solution, Interpolation can make a problem when you use props in your styles.
CSSObject is originally meant to be used in that exact case.

@kentcdodds
Copy link
Contributor Author

This has been super helpful. Thank you friends!

@kentcdodds kentcdodds closed this Jan 8, 2019
@kentcdodds kentcdodds deleted the patch-2 branch January 8, 2019 20:02
@lychyi
Copy link
Contributor

lychyi commented Jan 8, 2019

@Ailrun Good to know! Thanks for the heads up.

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

5 participants