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

bugfix: enables CSS minification via environment variable #172

Merged
merged 1 commit into from
May 28, 2020

Conversation

mattoberle
Copy link
Contributor

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:

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.

⚠️ 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.

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.
@mattoberle mattoberle merged commit 225e157 into master May 28, 2020
@mattoberle mattoberle deleted the moberle/fix-minify-envvar branch May 28, 2020 17:06
phyllisstein added a commit that referenced this pull request Jan 20, 2021
* master:
  3.13.0
  Allow multiple leading underscores in field names (#179)
  Introduces release workflow via CircleCI (#180)
  3.12.2
  import the command you actually use (#175)
  3.12.1
  bugfix: enables CSS minification via environment variable (#172)
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.

3 participants