-
Notifications
You must be signed in to change notification settings - Fork 675
Remove @emotion/styled from components #1368
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
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/systemui/theme-ui/jeojhf9md [Deployment for 021546b canceled] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for submitting this :)
Quick aside—we’re also depending on |
I just tried running size-limit on the uncompressed, built versions of the current release vs this branch, & it reduces size by 1KB in this package (17.44KB vs 16.55KB). Less than I thought/hoped, but it’s a solid step nonetheless, & once we remove |
@@ -12,9 +12,7 @@ | |||
}, | |||
"dependencies": { | |||
"@emotion/react": "^11.1.1", | |||
"@emotion/styled": "^11.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index.d.ts imports StyledComponent and we leak it in our public API.
import { StyledComponent } from '@emotion/styled'
export const Box: StyledComponent<BoxOwnProps, BoxProps>
We'll need an update to index.d.ts
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can update it for the sake of it, however it is imported by @emotion/react
as a devDependency anyway:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a devDependency anyway
It's not installed in user's node_modules then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what to do about it - all types are dev dependencies and they are not part of the production bundles.
- We can add the type and pretty much duplicate
StyledComponent
, with all the risks duplicating code entails. - We can add
@emotion/styled
as a devDependency (different from using it at run-time as previously). - We can leave as is.
I took #3 but open to different opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add @emotion/styled as a devDependency (different from using it at run-time as previously).
Not sure what to do about it - all types are dev dependencies and they are not part of the production bundles.
"dependencies"
and "devDependencies"
have no inherent meaning in the context of bundling. If I remember correctly, brunch bundler used to bundle all "dependencies"
, but webpack doesn't care about them at all (It traverses your module tree.)
They have meaning for package managers however. If a package A has package B in "dependencies"
, both will appear in user's node_modules when they do npm install A
. This is not the case for "devDependencies"
. In case of "peerDependencies"
, the user will get a warning in the terminal prompting them to install the peerDependency.
If we use types from a devDependency, users won't know to install the package, but the types will collapse to any
or fail their build when their tsconfig option skipLibCheck
is set to false.
You can read more about it in npm docs:
There are also bundledDependencies
and optionalDependencies
, but I haven't seen anybody use them, so I'm not sure what's the support like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What part of the type do we need from Emotion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its just ‘StyledComponent’
import { StyledComponent } from '@emotion/styled' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, I think it should be a devDependency.
So if somebody is depending just on theme-ui
, and doesn't have @emotion/styled
in their own dependencies or devDependencies, this change will remove @emotion/styled
from their node_modules.
If they use TypeScript with skipLibCheck: false
, we'll crash their build.
If they use JavaScript or skip lib check, we'll de facto disable editor support on Box and Flex, what is still an regression.
If we remove @emotion/styled from runtime, we need to stop depending on their types, especially since the types for Box and Flex is this PR are now incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hasparus Will your PR to move @emotion/styled
to user/peer deps fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lachlanjc I started working on removing @emotion/styled on remove-emotion-styled-2
branch — take a look at develop...remove-emotion-styled-2.
I think we'll release peer-deps branch and @emotion/styled removal in the same version — both might be breaking changes.
great! Co-authored-by: Lachlan Campbell <[email protected]>
Closing in favor of #2043! |
a few questions we can discuss