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

Remove traces of glamor #1286

Merged
merged 4 commits into from
Feb 26, 2017
Merged

Remove traces of glamor #1286

merged 4 commits into from
Feb 26, 2017

Conversation

timneutkens
Copy link
Member

@timneutkens timneutkens commented Feb 25, 2017

As talked about with @rauchg. Glamor takes up around 60KB of the bundle (pre-gzip). Since styled-jsx is the way to go now and we support adding glamor by the user we should remove it as dependency cause it is bundled even when not used.

Added rehydration to the example, since we did that in our code.

There is only one thing I'm not sure about and want to discuss:
what should we do with next/css. Right now I added a throw for when it is imported. I'm not sure if we should do that / some other way to notify the user it has been removed. The reasoning behind the throw is that when we would do a console.warn the user would see css.default.<X> not found because we don't have the glamor dependency anymore.

Fixes #989
Fixes #1108

As talked about with @rauchg. Glamor takes up around 60KB of the bundle (pre-gzip). Since styled-jsx is the way to go now and we support adding glamor by the user we should remove it as dependency cause it is bundled even when not used.

Added rehydration to the example, since we did that in our code.

There is only one thing I'm not sure about and want to discuss:
what should we do with next/css. Right now I added a throw for when it is imported. I'm not sure if we should do that / some other way to notify the user it has been removed. The reasoning behind the throw is that when we would do a console.warn the user would see 'css.default.<X>' not found because we don't have the glamor dependency anymore.
@timneutkens timneutkens changed the title Remove traces of glamor [WIP] Remove traces of glamor Feb 26, 2017
@albinekb
Copy link
Contributor

I thought we were gonna release 2.0 with the deprecation then soon after 3.0 without glamor? :thinking_face:

@rauchg
Copy link
Member

rauchg commented Feb 26, 2017

@albinekb yeah but we've done enough betas to let people migrate at this point, and we didn't realize that the glamor bundle was just as big as the next.js bundle before :(

@albinekb
Copy link
Contributor

LGTM @rauchg, yeah sounds right. I think most users are on beta now 😄

@timneutkens
Copy link
Member Author

Ready for merge.

@timneutkens timneutkens changed the title [WIP] Remove traces of glamor Remove traces of glamor Feb 26, 2017
@arunoda
Copy link
Contributor

arunoda commented Feb 26, 2017

Great work @timneutkens as always.

@arunoda arunoda merged commit 408633c into vercel:master Feb 26, 2017
@timneutkens timneutkens deleted the remove-glamor branch February 26, 2017 12:34
@timneutkens
Copy link
Member Author

@arunoda ❤️ ❤️ ❤️

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants