search

ChromaticHQ / calliope

An opinionated yet extensible toolset to handle common front-end tasks
3 stars 0 forks source link

Make JS/SCSS linting opt-out, instead of opt-in #27

Closed agarzola closed 2 years ago

agarzola commented 2 years ago

Description

This PR updates the default configuration of Calliope to make JS/SCSS linting during development opt-out (that is: enabled by default). It also updates the README to reflect this new behavior, as well as the boilerplate README, .env, and calliope.config.js files. Finally, tests were adjusted to skip linting while processing stylesheets for tests and to add a test that validates that linting is carried out by default.

While running tests, I noticed a deprecation warning for the fs.rmdirSync method, so I took this opportunity to switch a helper file to use fs.rmSync as recommended.

Motivation / Context

Wanting JS/SCSS to be linted during development is a more preferable behavior than the alternative (not wanting to lint these files during development), so we should enable linting during development by default, whilst preserving developers’ ability to disable it if desired.

Testing Instructions / How This Has Been Tested

Tested as part of chromatichq/chromatichq-eleventy#274.

Screenshots

This screenshot shows the chromatichq-eleventy project as it currently exists in chromatichq/chromatichq-eleventy#274 with no .env file (ls .env fails) and yarn start showing errors for offending code in a JS file:

image

Deprecation warning before the rmdirSync-to-rmSync change in this PR: image

No deprecation warning after changes in this PR: image

Documentation

The main README, sample README, sample .env, and sample calliope.config.js files have all been updated to reflect the new behavior.

agarzola commented 2 years ago

Tests were failing on Node.js v12 due to rmSync not being available. That method was introduced in v14. Since v12 is end of life and we have no projects on v12 that use Calliope, I have pushed a commit that:

  1. Removes v12 from our testing matrix.
  2. Adds v18 to our testing matrix (since that is the next LTS; currently in active development).
  3. Sets the engines object in our package.json to make sure installing the package with a Node.js version <14 throws an error.

image

agarzola commented 2 years ago

Tests for the styles task were failing in Windows due to my ill-advised use of echo (via child_process.execSync) to create a contrived .env file in the tmp directory where tests are run. I just pushed a commit that switches to use fs.writeFileSync, which is —naturally— platform-independent.

agarzola commented 2 years ago

Checks pass! I love that our tests surfaced two issues I could not have easily foreseen with only local testing. Merging this now and publishing a new major version.