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

Minify CSS before writing the bundles. #348

Closed
wants to merge 4 commits into from
Closed

Conversation

m-mujica
Copy link
Contributor

@m-mujica m-mujica commented Jan 6, 2016

Closes #88.

Instead of minifying each css node individually, this PR changes steal-tools to minify the concatenated bundle before writing. I tested this branch in my current app and got some interesting numbers:

  • before:
    652K dist/bundles/bundle-a.css
    268K dist/bundles/bundle-b.css
    192K dist/bundles/bundle-a-b.css
  • after
    440K dist/bundles/bundle-a.css
    196K dist/bundles/bundle-b.css
    192K dist/bundles/bundle-a-b.css

There is some noise in the write_bundles file b/c I made some changes to the indentation, here is the interesting diff 8c3da0e.

@m-mujica
Copy link
Contributor Author

m-mujica commented Jan 6, 2016

@matthewp should I send this PR against minor instead? Not sure why the build failed.

@m-mujica
Copy link
Contributor Author

m-mujica commented Jan 7, 2016

The build was failing due to a change in socket.io that socketio/socket.io#2368 broke feathers/testee, a new version of testee fixed the problem.

I also noticed that my main css bundle was quite big, I was able to reduce its size by using less reference imports bringing it down to 276K minified, with this branch the same bundle minified is 272K; which is less of a dramatic improvement, but :itissomething:

@matthewp
Copy link
Member

Went over it, looks good. Trying to think of when this should go it. Is it a breaking change? Trying to think of a scenario where your app would behave differently because of the non-duplicate selectors.

@matthewp
Copy link
Member

Putting it under the 1.0.0 milestone for now, could go in sooner.

@matthewp matthewp added this to the 1.0.0 milestone Jan 19, 2016
@matthewp matthewp removed this from the 1.0.0 milestone Feb 3, 2016
@matthewp
Copy link
Member

matthewp commented Feb 3, 2016

This will go on 0.14.0

@matthewp matthewp modified the milestones: 1.0.0, 0.14.0 Feb 3, 2016
I incorrectly merged the minify module which caused tests to break, this fixes it.
@matthewp
Copy link
Member

matthewp commented Feb 3, 2016

This is merged into minor and part of 0.14.0-pre.0. Closing.

@matthewp matthewp closed this Feb 3, 2016
@m-mujica m-mujica deleted the minify-bundled-css branch August 11, 2016 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants