expressjs.com
expressjs.com copied to clipboard
expressjs
Add husky and configure pre-push hook
Currently, it's easy for new contributors to accidentally miss the
eslint configuration and incorrectly format their code examples when
first creating a pull request. Adding git hooks gives contributors
immediate feedback of mismatched styling before they are able to
create the PR minimizing "re-work" by contributors and back and forth
for maintainers.
@dougwilson I've updated the PR title and commit message to reflect that this is actually a pre-push hook. Let me know if you want any other changes.
Hi @SimplyComplexable apologies, I have just been working to prep a new Express release, so this fell to the wayside for me.
Personally, I am against these types of git hooks, so was seeing if perhaps someone else had an opinion. If I were to have an opinion about git hooks, though, I believe that something like this should be a pre commit hook, not a pre push hook, right?
Hmm I see the value, since it is cumbersome for contributors and maintainers to ask folks to fix styling after the fact.
I'm not usually a fan of husky pre-commit, as it can be frustrating to fight the linter when you're just trying to check in code. However, the linting we are doing here is fairly reasonable as compared to linting commit messages which can be super aggressive and cause you to lose your commit message after writing it.
FWIW we removed husky from the node.js redesign effort (twice, it seems) because it was a barrier to entry.
However, that was linting actual code and commit messages, which was much stricter than what we are looking at here.
As far as my thoughts on the commit vs push, it comes from the docs I typically see around what the different hook cycles are meant for. For example on https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks
The pre-commit hook is run first, before you even type in a commit message. It’s used to inspect the snapshot that’s about to be committed, to see if you’ve forgotten something, to make sure tests run, or to examine whatever you need to inspect in the code. Exiting non-zero from this hook aborts the commit, although you can bypass it with git commit --no-verify. You can do things like check for code style (run lint or something equivalent), check for trailing whitespace (the default hook does exactly this), or check for appropriate documentation on new methods.
The description for what that hook is for seems to be exactly what is trying to be done here is all 😄
We use a similar setup at my work. From my experience, the pre-commit hook is just too aggressive. Commits are done regularly, often when work is still in progress, which would make running linting ineffective. On the other hand, the push usually isn't done until the work is in a more complete state. Our hooks also run tests and type checking, which makes it is a time-consuming process, so limiting the amount of time those hooks have to run is beneficial. I would also argue that it doesn't really matter if linting/tests pass on one particular commit, just that they pass when you go to make the PR which would ultimately be the role of the pre-push hook.
Well, we have to squash all your commits into a single commits before we merge, so having a bunch of commits still creates more work, ultimately. So if the desire of this is to reduce the overall work, it would seem like a pre commit would be the way to go here, as we only land a single commit, ultimately. The other issues you highlight I don't think apply, as our lint takes very little time... And of course there it git itself which, as I linked to, says linting should be a pre commit hook specifically.
I don't know if husky supports this, but it would be even better if we could customize the error or help message to let folks know how to get their commit accepted. Specifically, if we also had a lint:fix script, we could just tell folks to run that to resolve what it could.
One last thing though, I do think a lot of our PRs come in to this repo through the Github UI, with people fixing typos or small things. The only way to really guard against ever touching styling as maintainers would be creating a github action to format the code for us. That might be risky with docs though, but it's a thought.
I do think a lot of our PRs come in to this repo through the Github UI
That's right. That's why there are so many from patch-1 branch names :)
Talking about local dev experience, I was just testing it out, and both pre-commit and pre-push seem to lint uncommitted/unstaged changes :( So using git add -p to selectively commit certain things (or push with an unclean workspace) result in failures with either of the hooks around.
FWIW this is the experience for pre-commit in the VS Code editor:
And for pre-push:
Clicking to open the log for pre-commit takes it right to the eslint output, but for the pre-push it takes it to a large string of git errors ending with a npm test failed (but no eslint output anywhere).
BTW @SimplyComplexable we're only commenting on this so much because we're trying to hash things out, not trying to pick you or it apart! Discussion is typically how PRs go w/ Express, don't take any of it personally.
Exactly @jonchurch ! The discussion is in the effort to get something landed 😆 !
Yeah I completely understand! No worries at all. I was going to add that I typically use --amend to avoid having multiple commits, but that comes down to workflow. Happy to implement whatever you need, just let me know what your decision is.
@jonchurch @dougwilson Any update on how you want this to work? I can work on updating this to a pre-commit hook with lint-staged if that's the direction you want to go
We can address this in the 5.x documentation, as it can be skipped at the moment.
OK, I presume that means we should close this? If not, feel free to reopen.
