add husky

[mmkal]

This will run the build script before every commit, and add the dist folder to it - no more out-of-date dist/.

Note: husky will install the relevant git hooks after npm install (via the prepare script), as documented here: https://typicode.github.io/husky/getting-started.html

I'm not sure if others have existing hooks - if they'd be beneficial to all they could be added to the pre-commit script, or if not there may be a way to merge husky hooks with user-defined ones.

[pvspain]

We have a recommended pre-commit hook already. Perhaps you could extend that instead of introducing another dependency which does the same job?

[mmkal]

Ah, I knew I'd seen that somewhere. Somehow I missed it when looking in the docs again. Thank you. Ok, I agree there definitely shouldn't be two pre-commit hook methods. However I do think that while dist/ is checked into the repo, it's important that npm run build should run before every commit.

So, a couple of thoughts, each of which I might have mentioned in passing on other PRs:

1. My main concern is that dist/ ends up out of sync with src/. To solve for that, I can add a git diff --exit-code step to the GitHub Actions workflow(s). Since GitHub Actions already runs npm run build, git diff --exit-code will fail if there are any unstaged changes (i.e., if the developer failed to run npm run build before committing).
2. Consolidating pre-commit strategies. As mentioned above, I do think it's important that npm run build is called for every commit (all the more so if CI is going to validate it). The simplest thing to do would be to add a line to the recommended pre-commit hook you linked to. But I do think husky might be a step better, even if more opinionated and a new dependency. husky will install the hook when contributors run npm install, so it doesn't require manual intervention/ educating new contributors. For those who are interested or want to understand how the hook works - husky is extremely widely used (10 million downloads/ week) and well documented. It is an extra dependency, but only a dev dependency so has no impact on the library output. And I think it's a pretty safe one.

So, I'll open a separate PR for 1. and wait for feedback on 2 - that is, do we extend the pre-commit hook docs, or consolidate and move the pre-commit hook to husky. Converted this one to a draft in the meantime.

[pvspain]

Ah, I knew I'd seen that somewhere. Somehow I missed it when looking in the docs again.

I also mentioned it in comments on another of your PRs. Surely you read them :)

Thank you. Ok, I agree there definitely shouldn't be two pre-commit hook methods. However I do think that while dist/ is checked into the repo, it's important that npm run build should run before every commit.

No problems there.

So, a couple of thoughts, each of which I might have mentioned in passing on other PRs:

1. My main concern is that dist/ ends up out of sync with src/. To solve for that, I can add a git diff --exit-code step to the GitHub Actions workflow(s). Since GitHub Actions already runs npm run build, git diff --exit-code will fail if there are any unstaged changes (i.e., if the developer failed to run npm run build before committing).

I wouldn't add that to the GitHub Action. Flag it locally - see my comments for point 2.

2. Consolidating pre-commit strategies. As mentioned above, I do think it's important that npm run build is called for every commit (all the more so if CI is going to validate it). The simplest thing to do would be to add a line to the recommended pre-commit hook you linked to. But I do think husky might be a step better, even if more opinionated and a new dependency. husky will install the hook when contributors run npm install,

The hook is recommended rather than mandated. I personally would like all contributors to use it, but I find persuasion is a better strategy in teams, and in life generally, if you want to keep people onboard willingly. It is an essential skill for good managers.

The assets under dist/ are versioned now and will be updated whenever npm run build is run, either explicitly or implicitly by the amended pre-commit hook. I find there are often additional changes when the pre-commit hook runs locally due to the actions of lint:fix and prettier:fix. It is easily resolved with a subsequent amended commit (git commit --amend or setup an alias for less typing!).

To err is human and inevitable. Thinking you are exempt is hubris. Accepting that and always checking yourself is the life skill. We automate for a living. I have found over time people devolve/deskill to just below the newly lowered barrier to entry! It is a slow race to the bottom.

So, I'll open a separate PR for 1. and wait for feedback on 2 - that is, do we extend the pre-commit hook docs, or consolidate and move the pre-commit hook to husky. Converted this one to a draft in the meantime.

I still vote no to husky. It's two additional lines in the existing pre-commit hook. Increasing people's git IQ is a good thing that will serve them well. You can move the hook out of the documentation if you want it more obviously versioned. I think the current solution is elegantly simple - just a cut'n'paste into a terminal window.

[mmkal]

I also mentioned it in comments on another of your PRs. Surely you read them :)

Lots of comments, lots of docs - I forgot!

I wouldn't add that to the GitHub Action. Flag it locally - see my comments for point 2.

I disagree with this. The repo itself is in a very bad state when src/ and dist/ don't match. The docs will say "you can do X" and a user will be bewildered when they can't. It is absolutely the sort of thing that CI should check. How it gets into that state is up to the developer but it's an important check. Local flags shouldn't be mandated as you say (and, in fact, they can't be).

The hook is recommended rather than mandated

Absolutely. husky is for convenience. To make things "just work" rather than rely on people reading all the docs (which I have proved, some won't!). It is of course not mandated. Just set HUSKY=0 as an env var the hooks won't be installed. Developers are free to install a different hook that sends an email to their grandmother if they like.

It is a slow race to the bottom

Fair enough - maybe I've reached the bottom. My feeling is: when I am working on a crossword library part-time I would like to worry about as few crossword-unrelated repo quirks as possible. If things can ✨just work✨, then they should. I want the contributor workflow to be: checkout, install, change some code, commit, open a PR. But hey, if educating contributors about git is a goal of crosswords-js - so be it. I won't die on this hill!

I'll close this - happy to reopen and consolidate if you change your mind.