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

Upgrade to Emotion v11 #1261

Merged
merged 24 commits into from
Nov 20, 2020
Merged

Upgrade to Emotion v11 #1261

merged 24 commits into from
Nov 20, 2020

Conversation

dcastil
Copy link
Contributor

@dcastil dcastil commented Nov 14, 2020 •
edited
Loading

Closes #1258

To dos:

  • Test whether Theme UI docs are working with Emotion v11
  • Fix TypeScript issues in Jest tests

@dcastil
Copy link
Contributor Author

dcastil commented Nov 14, 2020 •
edited
Loading

The last remaining TypeScript issue is that Emotion's styled components using the withComponent modifier don't accept a css prop.

The Box component in packages/components/test/types.tsx:89 accepts it, but the SectionBox in packages/components/test/types.tsx:84 doesn't.

@hasparus do you know how this could be resolved? (fixed it in 60d493a, Emotion removed their css types from the global scope in emotion-js/emotion#1941.)

@deckchairlabs
Copy link
Contributor

@dcastil I suppose box would need to specify that it does take css prop? As the props only seem to be inferred when passing an actual component, and not a string tag?

https://github.com/emotion-js/emotion/blob/master/packages/styled/types/base.d.ts#L36

@lachlanjc
Copy link
Member

Thank you so much for your work here @dcastil!!

@hasparus hasparus added this to the v0.4 milestone Nov 18, 2020
dcastil added 18 commits November 18, 2020 20:00
@dcastil
upgrade emotion-related packages to v11
4984f49
@dcastil
update imports of emotion packages
5c525ca
@dcastil
upgrade emotion packages in @theme-ui/mdx
17409b8
@dcastil
fix incorrectly specified devDependencies
00b3b61 
reduces bundle of @theme-ui/mdx from 40 kB to 1 kB
@dcastil
fix TypeScript types
ebbdf47
@dcastil
add yarn.lock of changes
f409fd1
@dcastil
fix @emotion/react being included in @theme-ui/css bundle
c4816c0
@dcastil
deduplicate yarn.lock
615c932
@dcastil
fix missing TypeScript types
8ced010
@dcastil
fix missing peer dependencies of @emotion/jest
ecdd7e8
@dcastil
fix some jest tests
7f0ba4a
@dcastil
update snapshots
31bc896
@dcastil
fix undefined theme causing issues with emotion v11
9d7bf53
@dcastil
fix type issue in color mode TpyeScript test due to updated csstype lib
57f2aef
@dcastil
fix css prop not working
3ff14aa
@dcastil
fix incorrect component types
951f3e8
@dcastil
upgrade emotion libs and remove unnecessary dependencies
1de9348
@dcastil
fix type issue in Font component
e41d1b8
@hasparus
Copy link
Member

Hey @dcastil, is there a way I can DM you? Twitter, email or sth like that?

I'm trying to set up some means of communication between frequent contributors (AKA the place to yell "@hasparus, you promised me that code review a week ago"), and I'd like to send you an invite link :)

@dcastil
Copy link
Contributor Author

dcastil commented Nov 18, 2020

@hasparus That's cool! I DMed you on Twitter.

hasparus
hasparus reviewed Nov 18, 2020
View reviewed changes
packages/core/src/index.ts
import { css, Theme } from '@theme-ui/css'
import * as React from 'react'
import deepmerge from 'deepmerge'
import packageInfo from '@emotion/core/package.json'
import packageInfo from '@emotion/react/package.json'
import {} from '@emotion/react/types/css-prop'
Copy link
Member

@hasparus hasparus Nov 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Let's leave this a side note here. I think we should also make sx prop types opt in (for v0.5.0? v0.6.0?), as we have users who may prefer to use @theme-ui/components instead of JSX pragma.

hasparus
hasparus approved these changes Nov 18, 2020
View reviewed changes
Copy link
Member

@hasparus hasparus left a comment

Choose a reason for hiding this comment

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

LGTM.
This would be a small breaking change, as users will have to update their emotion deps, and fix a bunch of TS errors. Do you think we should bump to v0.5.0 while merging it?

I'm aware we didn't release 0.4.0, but the rc versions are "stable enough" and a bunch of projects are using them.

@hasparus
Copy link
Member

I've added a test, replaced emotion/core with emotion/react in the keyframes guide in the docs, and drafted a bit for the changelog on test-components-ref-type branch.

@fcisio
Copy link
Collaborator

fcisio commented Nov 19, 2020

Hey @dcastil, is there a way I can DM you? Twitter, email, or sth like that?

I'm trying to set up some means of communication between frequent contributors (AKA the place to yell "@hasparus, you promised me that code review a week ago"), and I'd like to send you an invite link :)

You should look into creating a Discord Server!
It's free and it's as complete as Slack. Others like Gatsby and Storybook are on there, but you can make it as small and contained or as big as you want.

Hope you find this useful 🎉

@hasparus
Copy link
Member

Nice idea, @fcisio!
Here's the invite https://discord.gg/2bJVCbuKwA

@pkieltyka
Copy link

pkieltyka commented Nov 19, 2020 •
edited
Loading

thanks all for putting this together, this will be a big improvement to theme-ui. Just my 2 cents -- I think either a release as 0.4.0-rcX or 0.5.0 is fine. 0.5.0 is probably safer, but I also think any user on a "rc" release understands it beta-ware and can also pin to a specific "0.4.0-rcX" version for their needs, so it might feel more correct to stick to 0.4.0-rcX line. Either approach is fine IMHO. As well, the migration path from emotion 10 to emotion 11 for an app is quite simple -- to give an example, my app built with emotion 11 of ~300 components upgraded without any changes to the components going from emotion 10 to 11. The only changes I had to make was updating my import statements and the Theme type: https://emotion.sh/docs/emotion-11

@hasparus
Copy link
Member

Don't fix conflicts, I'll merge it in a bit :)

hasparus added a commit that referenced this pull request Nov 20, 2020
@hasparus
chore: update changelog
735f9b3 
BREAKING CHANGE: Upgrade to Emotion 11, and `csstype` 3. PR #1261.
  - We are now depending on `@emotion/react@11` instead of `@emotion/core@10`
  - `sx` prop types are still global, and we opt in for Emotion `css` prop types (This will change in the future.)
  - Refer to [Emotion 11 release notes](https://emotion.sh/docs/emotion-11) for more information.
@hasparus hasparus merged commit 948583d into system-ui:master Nov 20, 2020
@dcastil dcastil deleted the upgrade-to-emotion-11 branch November 20, 2020 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasparus hasparus hasparus approved these changes

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

Successfully merging this pull request may close these issues.

Upgrade to emotion v11
6 participants
@dcastil @deckchairlabs @lachlanjc @hasparus @fcisio @pkieltyka