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

Add Git hook to lint JavaScript and Sass file pre-commit #2848

Merged
merged 2 commits into from
Nov 29, 2022

Conversation

romaricpascal
Copy link
Member

@romaricpascal romaricpascal commented Sep 14, 2022

What

This PR adds automatic linting of code being commited before git push.

The relevant linters will run on any JavaScript and/or Scss being commited, and only what's being commited. The rest is stashed temporarily while the linting happens to ensure irrelevant files or hunks don't get in the way. The linters will also try to fix any issue they can fix automatically to make the commit only fail if a human intervention is needed.

The pre-commit hook get installed automatically on npm install (which we'll need to run after this gets merged in main). It can be bypassed if really getting in the way of a commit that really needs to go (though like with disabling linters, best to have a good reason for it... and Github CI will fail anyway so might as well save yourself a round-trip).

As a side bonus, you can also lint your staged files without commiting by running npx lint-staged after staging content.

Why

Linting pre-commit reduces the chance of losing time to only detecting linting issues once Github CI run and is quick enough to not burden the time it takes to commit. Most our editors can likely run ESLint on save, but there's no guarantee everyone has something like that set up so that a little extra safety that gets automatically set up when installing the project dependencies.

Note that it won't run in case of a quick edit-and-commit on Github and we'll have to go fix things manually should the CI pick up a linting issue after such edit.

How

It relies on two tools:

  • lint-staged which handles detecting which files are being commited and running the linting on them. It stashes/unstashes any changes that are not staged so the linting runs on the files in the state they are actually commited, in case you're using git add -p.
    I thought there'd be some caution to have around the package and dist folder to avoid messing with compiled files, but standard is already configured to avoid those.
  • husky which handles the creation and installation of Git hooks, mostly pointing Git to the relevant scripts in .husky (configurable) through its core.hooksPath config.

Possible future developments

With Git hooks available, there's potentially other automations we could look into, like:

  • use post-checkout to detect if things like npm install need to run and warn
  • use pre-push to run tests locally before hitting CI (not sure if that's a great one though)
  • possibly other stuff

As linters get run automatically, we could look into linting other kinds of files. I'm thinking Markdown specifically, both for its structure with something like remark-lint and for its content with something like vale.sh that Claire mentionned recently.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 September 14, 2022 17:30 Inactive
.husky/pre-commit Show resolved Hide resolved
.lintstagedrc.js Show resolved Hide resolved
.lintstagedrc.js Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 September 22, 2022 17:02 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 15:14 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 16:06 Inactive
@romaricpascal romaricpascal changed the title For discussion: Add Git hook to lint .(m)js and .scss file pre-commit Add Git hook to lint JavaScript and Sass file pre-commit Nov 23, 2022
@romaricpascal romaricpascal marked this pull request as ready for review November 23, 2022 16:13
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 16:21 Inactive
.lintstagedrc.js Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 17:02 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 17:16 Inactive
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with two little comments

One's a blocker but I'm sure you'll fix it before merging 😆

Cheers @romaricpascal

package.json Show resolved Hide resolved
.lintstagedrc.js Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 17:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 17:50 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 17:58 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 18:02 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 18:34 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 18:38 Inactive
Install the hooks in the prepare script through a node call rather than just `husky install`
as the package won't be installed on Heroku, which would make the `prepare` script fail.

https://typicode.github.io/husky/#/?id=with-a-custom-script
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2848 November 23, 2022 18:43 Inactive
"lint:js:cli": "eslint --cache --cache-location .cache/eslint --cache-strategy content --color --ignore-path .gitignore --max-warnings 0",
"lint:scss": "npm run lint:scss:cli -- \"{app,src}/**/*.scss\"",
"lint:scss:cli": "stylelint --cache --cache-location .cache/stylelint --cache-strategy content --color --ignore-path .gitignore --max-warnings 0",
"prepare": "node -e \"try { require('husky').install() } catch (e) {if (e.code !== 'MODULE_NOT_FOUND') throw e}\""
Copy link
Member Author

@romaricpascal romaricpascal Nov 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This automatically installs the Git hooks when you npm install. This is the lifecycle script recommended by Husky's documentation (and we already have postinstall anyway).

We cannot run the recommended husky install, though. husky is a devDependency, as it's not needed to run the review app. Because of that, it won't be installed on Heroku, (or when running npm install --omit=dev) which means that prepare would fail if we were to run husky install. Instead we can rely on the documented MODULE_NOT_FOUND exception from Node to ensure a smooth installation, as proposed by Husky's docs.

@colinrotherham colinrotherham self-requested a review November 28, 2022 11:27
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving again

You know what would be amazing? Updating the gulp.watch() lint tasks so they also only lint the "changed files", rather than every single JavaScript and Sass file

@romaricpascal romaricpascal merged commit 07fc653 into main Nov 29, 2022
@romaricpascal romaricpascal deleted the git-hook-linting branch November 29, 2022 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants