Skip to content

Commit

Permalink
bugfix: enables CSS minification via environment variable
Browse files Browse the repository at this point in the history
Setting `CLAYCLI_COMPILE_MINIFED` (or any of the variants) did *not*
minify CSS or Fonts. This was due to our use of a package called
`gulp-if`, which has a usage like this:

```js
gulpIf(condition, trueChild, falseChild, minmatchOptions);
```

Where `condition` is expected to either be:

1. A `boolean`.
2. A `function` that will return a `boolean`.

When setting `CLAYCLI_COMPILE_MINIFIED` from an environment variable we
end up with a `string` value for `minify`, which causes `gulpIf` to
evaluate to `false`, skipping minification.

This commit simply casts `minify` to a `bool` when calling `gulpIf`.

A more robust approach would be to fix typing across the board for
environment variable defaults, providing some sort of `strToBool`
method to cast `['1', 't', 'true', 'y', 'yes']` to `true` and
everything else to `false`.

Taking the more robust approach would require a larger refactor and is
a good follow-up candidate for this bug fix.

:warning: Note: There is not an existing test suite to validate this
change, all tests were performed manually by running `clay compile` on
an existing `clay` project with `export CLAYCLI_COMPILE_MINIFIED=true`
before the commit & then checking the format of the CSS output files
(they were not minified).

After adding the commit, re-running with `CLAYCLI_COMPILE_MINIFIED=true`
resulted in minified CSS output.
  • Loading branch information
mattoberle committed May 28, 2020
1 parent 91d6f2f commit 442d205
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 3 deletions.
4 changes: 2 additions & 2 deletions lib/cmd/compile/fonts.js
Expand Up @@ -149,7 +149,7 @@ function compile(options = {}) {
.pipe(newer({ dest: path.join(destPath, 'css', `_inlined-fonts.${styleguide}.css`), ctime: true }))
.pipe(es.mapSync((file) => getFontCSS(file, styleguide, true)))
.pipe(concat(`_inlined-fonts.${styleguide}.css`))
.pipe(gulpIf(minify, cssmin()))
.pipe(gulpIf(Boolean(minify), cssmin()))
.pipe(gulp.dest(path.join(destPath, 'css')))
.pipe(es.mapSync((file) => ({ type: 'success', message: file.path })));
streams.push(inlinedFontsTask);
Expand All @@ -165,7 +165,7 @@ function compile(options = {}) {
.pipe(gulp.dest(path.join(destPath, 'fonts')))
.pipe(es.mapSync((file) => getFontCSS(file, styleguide, false)))
.pipe(concat(`_linked-fonts.${styleguide}.css`))
.pipe(gulpIf(minify, cssmin()))
.pipe(gulpIf(Boolean(minify), cssmin()))
.pipe(gulp.dest(path.join(destPath, 'css')))
.pipe(es.mapSync((file) => ({ type: 'success', message: file.path })));
streams.push(linkedFontsTask);
Expand Down
2 changes: 1 addition & 1 deletion lib/cmd/compile/styles.js
Expand Up @@ -126,7 +126,7 @@ function compile(options = {}) {
simpleVars({ variables }),
nested()
].concat(plugins)))
.pipe(gulpIf(minify, cssmin()))
.pipe(gulpIf(Boolean(minify), cssmin()))
.pipe(gulp.dest(destPath))
.pipe(es.mapSync((file) => ({ type: 'success', message: path.basename(file.path) })));
}
Expand Down

0 comments on commit 442d205

Please sign in to comment.