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

Types: props.theme should not be optional in Emotion 11 #1707

Closed
fabien0102 opened this issue Jan 8, 2020 · 6 comments
Closed

Types: props.theme should not be optional in Emotion 11 #1707

fabien0102 opened this issue Jan 8, 2020 · 6 comments

Comments

@fabien0102
Copy link
Contributor

Current behavior:

The props.theme is Theme | undefined instead of Theme, so it's very hard to migrate to emotion 11 since we need to make sure that the theme is there on every usage.

This was previously discussed and fixed here -> #1501 (comment)

I also follow the project to migrate to the new way, as describe here -> https://github.com/emotion-js/emotion/blob/next/docs/typescript.mdx

Note: Since the theme is a runtime variable inject by the ThemeProvider, I totally get that Theme is actually in reality optional and the current implementation is more typesafe, but in another hand, this will be very painful to use if we need to check everytime if the theme is there before consuming it…

To reproduce:

https://codesandbox.io/s/brave-tereshkova-lx8sn

image

=> props.theme is Theme | undefined instead of Theme

Expected behavior:

I was expecting props.theme to be Theme, so the migration to emotion 11 is way less painful 😬

Environment information:

  • react version: 16.8.4
  • emotion version: 11.0.0-next.10
@Andarist
Copy link
Member

Andarist commented Jan 8, 2020

https://github.com/emotion-js/emotion/blob/31e610f2385d5a3dfd532b31f743e5f6b9fee43b/docs/typescript.mdx#define-a-theme

If you were previously relying on theme being an any type, you have to restore compatibility with: [...]

@fabien0102
Copy link
Contributor Author

Thanks, but I like my typesafety, this was not any and putting any there will be a HUGE lost…

So I guess you suggest that we need to check if theme is there everytime? 😕

@fabien0102
Copy link
Contributor Author

For the record, this was considered like a bug two months ago and fixed -> #1501 (comment)

@fabien0102
Copy link
Contributor Author

Also, this is not consistant with the object notation, Theme is well defined in this case.

import styled from "@emotion/styled";

export default styled.h2(({ theme }) => ({
  color: theme.color // `theme` is `Theme` here
}));

@Andarist
Copy link
Member

Andarist commented Jan 8, 2020

Ok, so I've actually misunderstood your original report here. I've focused myself on that error reported in french - and for that my advice still holds, you should augment builtin interface. But you have also found an issue with our typings 👍 probably a result of different PRs being merged in a roughly the same time and caused a mergeable conflict.

@Andarist
Copy link
Member

Andarist commented Jan 9, 2020

Has been fixed by #1708 . Still need to resolve @emotion/serialize problem in #1710 , but expect a new release soon-ish.

@Andarist Andarist closed this as completed Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants