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?

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.

Figured it out and updated the PR! :-)

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

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.

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!