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

[rfc] use @emotion/stylis #2640

Merged
merged 2 commits into from Jun 21, 2019
Merged

[rfc] use @emotion/stylis #2640

merged 2 commits into from Jun 21, 2019

Conversation

@probablyup
Copy link
Contributor

probablyup commented Jun 21, 2019

Drops the bundle size by a fair amount (12.1kB vs 13.62kB)... the downside is you
can't customize stylis options anymore but still can supply plugins.

I think it's an acceptable compromise for the bundle size win?

Drops the bundle size by a fair amount... the downside is you
can't customize stylis options anymore but still can supply plugins.

I think it's an acceptable compromise for the bundle size win?
@probablyup probablyup added the 5.0 label Jun 21, 2019
@probablyup probablyup requested review from kitten and mxstbr Jun 21, 2019
keyframe: false,
prefix: true,
compress: false,
semicolon: false, // NOTE: This means "autocomplete missing semicolons"

This comment has been minimized.

Copy link
@probablyup

probablyup Jun 21, 2019

Author Contributor

the only change from this current set of options is semicolon is turned on

This comment has been minimized.

Copy link
@kitten

kitten Jun 21, 2019

Member

I think that's technically a breaking change that might affect plenty of apps without reasonable errors/warnings in place 😅

@probablyup

This comment has been minimized.

Copy link
Contributor Author

probablyup commented Jun 21, 2019

AFAIK the only setting I've heard someone want to customize was "prefix", and that actually is still customizable in this custom stylis build... instead of the stylisOptions prop we started out with for the beta we could instead switch it to something like disableVendorPrefixes for that one use case.

@mxstbr
mxstbr approved these changes Jun 21, 2019
Copy link
Member

mxstbr left a comment

I'm a fan of that, I was always a bit weary of introducing the stylisOptions prop as we really don't want anybody changing those options 😅

@probablyup

This comment has been minimized.

Copy link
Contributor Author

probablyup commented Jun 21, 2019

Updated to use the new prop for StyleSheetManager, bundle size is now 12.18kB still a nice win!

@mxstbr

This comment has been minimized.

Copy link
Member

mxstbr commented Jun 21, 2019

Did that remove support for stylisOptions? I still see it in the code if I'm not mistaken?

@probablyup

This comment has been minimized.

Copy link
Contributor Author

probablyup commented Jun 21, 2019

@mxstbr
mxstbr approved these changes Jun 21, 2019
Copy link
Member

mxstbr left a comment

LGTM let's slim our bundle size!

@probablyup

This comment has been minimized.

Copy link
Contributor Author

probablyup commented Jun 21, 2019

@thysultan

This comment has been minimized.

Copy link

thysultan commented Jun 21, 2019

Yes, please don't directly expose a stylisOptions to the end user that directly maps 1 to 1 with current stylis options in order to make it easier to upgrade to the next major version(breaking change) of stylis.

@probablyup

This comment has been minimized.

Copy link
Contributor Author

probablyup commented Jun 21, 2019

@thysultan Good to know, thanks!

@probablyup probablyup merged commit 7d6308f into canary Jun 21, 2019
6 checks passed
6 checks passed
sailci/build build
Details
sailci/clone clone
Details
sailci/flow flow
Details
sailci/install install
Details
sailci/lint lint
Details
sailci/test test
Details
@probablyup probablyup deleted the canary-switch-stylis branch Jun 21, 2019
probablyup added a commit that referenced this pull request Nov 7, 2019
* use @emotion/stylis

Drops the bundle size by a fair amount... the downside is you
can't customize stylis options anymore but still can supply plugins.

This is desirable according to Sultan though as the config interface
will change with the next stylis major.

* reimplement "stylisOptions" as "disableVendorPrefixes"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.