charge icon indicating copy to clipboard operation
charge copied to clipboard

feat: Minify CSS output

Open delucis opened this issue 4 years ago • 5 comments

This adds cssnano at the end of the PostCSS pipeline to minify CSS output.

cssnano is used without any options and will use its default preset:

with the default preset, we offer safe transforms only (Source)

Closes #782

delucis avatar Apr 30 '20 14:04 delucis

This looks pretty good!

I think maybe the minification should only happen in production (when building) and not in development (when serving)? 🤔 What do you think?

brandonweiss avatar Apr 30 '20 15:04 brandonweiss

maybe the minification should only happen in production (when building) and not in development

I thought that too, but the environment flag didn’t seem to be passed to the CSS builder and none of the other tasks seemed to be making that distinction (HTML is minified on serve for example), so just settled for the easy route 😄 .

If you can point me to the best way to disable in development, I’m happy to make the required changes.

delucis avatar Apr 30 '20 16:04 delucis

Figured it out and updated the PR! :-)

delucis avatar Apr 30 '20 19:04 delucis

Hi — checking in to see if you think this needs any additional changes?

delucis avatar May 29 '20 23:05 delucis

I’m so sorry—I completely blanked on this.

This is great! Just one more thing—and sorry, I should have pointed this out in advance. Because we’re conditionally minifying depending on the environment, we should have a test for that. So a test that when building for production it minifies the output of a CSS file.

The reason you had to update the snapshots is because the environment argument to the build function defaults to production and we weren’t previously setting the environment in the filesystem helper, so all the tests were assuming the environment is production.

https://github.com/brandonweiss/charge/blob/e2ce0cab7696d94c58b94fb9440f94ecb2859b8f/test/helpers/filesystem.js#L112-L121

I think we’ll probably want to tweak the helper. It probably makes sense to explicitly set the environment to “development”, and then the helper can take an optional third argument that’s an object that gets spread into the build function, that way arguments like environment can be overridden in some tests:

await buildAndSnapshotFilesystem(t, async () => {
  // ...
}, { environment: "production" })

That should keep the existing snapshots more readable and make it possible to add a test for the CSS minification. Let me know if that doesn’t make any sense or the code is confusing!

brandonweiss avatar May 31 '20 21:05 brandonweiss