Fix bad developer experience due to Husky pre-commit hooks

[HonkingGoose]

Describe the proposed change(s).

Currently

Right now we use husky to run this script before each git commit:

#!/bin/sh
. "$(dirname "$0")/_/husky.sh"


yarn lint-staged
yarn ls-lint


BRANCH_NAME=$(git branch --show-current)


if [ "$BRANCH_NAME" = "main" ]; then
  echo
  echo "Aborting commit."
  echo "You're committing to the main branch, usually this is not what you want to do."
  echo "Did you forget to make a new feature branch to put your work on?"
  echo
  echo "Use git checkout -b <new_branch> to create a new feature branch."
  echo
  echo "If you're sure you want to commit to the main branch, run git commit with the --no-verify flag."
  echo


  exit 1
fi

The pre-commmit hook running the linters adds friction, for example:

1. Write new code for problem
2. Try to create commit -> Husky runs linters -> Prettier error
3. Run Prettier to fix formatting
4. Create in-between commit
5. Try another approach -> try to commit -> husky complains again -> fix code -> commit
6. And so on

This friction can stop people from becoming regular contributors, or just plain be annoying for the maintainers.

So here are some options to fix the problem, let's discuss! 😄

Option 1: drop yarn lint-staged and yarn ls-lint commands

Maybe we want to drop the yarn lint-staged and yarn ls-lint commands, and let our CI tests on the PR branch catch those errors. We keep the check to prevent wrong pushes to main.

Option 2: use pre-push commit instead

We keep the current script, but change when it runs. When you run the script before git push you can commit "badly" as often as you want. But when it's time to push you need to clean up your work before pushing it to the PR branch.

If we do this option, we should change the log message for the script so it uses "pushes to main" instead of "committing to main". 😉

Option 3: drop husky fully

I don't think this is a good idea, most every commit should land into main via a PR, so that all the tests are run first. Only in rare cases do we want a maintainer to push to main directly.

[viceice]

🤔 prettier shouldn't complain, it should normally just fix the issue.

developers should also use a good idea, which shows lint Errors while you type. like free vscode.

for a maintainer it's more work to review and request changes because of failed prettier.

[HonkingGoose]

Some more background information. We tried fixing bad stuff on the contributors' PR branches automatically, for more context read the issue and PR:

• lint-fix bot #7575
• ci: add lint fix workflow #7635

Summary: we rejected the PR to auto-fix stuff. This is because of security concerns with using pull_request_target in the GitHub Action workflow.

[rarkins]

I prefer 2 or 3.

The commit linting takes too long today.

[HonkingGoose]

I prefer 2 or 3.

You were happy with my PR to "block accidental pushes to main" to the Husky checks:

This is a great idea. I shouldn't commit to main very often anyway so having to do it with a manual flag is fine. Let's make sure the error message is useful and wouldn't discourage any first time contributor to give up. ¹

Related PR:

• chore: soft-block commits to main branch with husky pre-commit check #11094

If you go with option 3 we also drop the check for commits/pushes to main, do you really want that? Did something change in the meantime, where you would maybe rather have no husky checks at all? Or do you no longer care about preventing accidental pushes to main? I'm trying to get a feel for the reasoning behind the choices. 😉

Footnotes

1. https://github.com/renovatebot/renovate/issues/11091#issuecomment-892557775 ↩

[rarkins]

Not pushing to main is good. Perhaps it's more about the time it takes rather than whether it's on or off?

[HonkingGoose]

I think the big thing is that we need to decide who to bother with the time loss:

• Linters run locally: contributor gets the time loss
• Linter run on GitHub: contributor gets to do something else, but maintainers get bothered by failed lints

[rarkins]

I think unfortunately the linting is too slow right now. If it were maybe <5s, it might be manageable

[viceice]

it's probably the markdown lint which always scans all files. 🤔

maybe it's eslint because of the circular dep check i added some time ago.

[HonkingGoose]

Not pushing to main is good.

GitHub can now block direct pushes when you have required workflows: ¹

Blocking direct push: Direct pushes are now blocked on branches of the repositories where required workflows are enforced. To push to a branch where required workflows are enforced at the organizational level, create a pull request to make the necessary changes. If you want to allow direct pushes for a particular repository, you must remove the repository as a target from respective required workflows.

Does this mean that we can drop the part of the Husky script that blockes pushes to main? The feature is still in beta, and I don't know if we enabled it yet.

Footnotes

1. GitHub Blog about GitHub Actions – Required workflows improvements ↩

[rarkins]

GitHub can now block direct pushes when you have required workflows

I don't think that will stop users from committing to their own main branch

[HonkingGoose]

Oh yeah. Let's keep that check then. 🙂

[rarkins]

See #22224 (comment)

lint-staged is ESM-only and now causing problems

[renovate-release]

🎉 This issue has been resolved in version 35.95.0 🎉

The release is available on:

• GitHub release
• 35.95.0

Your semantic-release bot 📦🚀