Skip to content

Conversation

@caugner
Copy link
Contributor

@caugner caugner commented Mar 26, 2025

Summary

Migrates to lefthook, replacing lint-staged and husky.

Also adds pre-push and post-merge hooks:

  • The post-merge hook, limited to the main branch, runs npm install.
  • The pre-push hook runs npm install, followed by lint commands (ESLint, prettier, tsc).

Test results and supporting details

Related issues

@caugner caugner requested review from Elchi3 and queengooborg March 26, 2025 19:20
@github-actions github-actions bot added infra Infrastructure issues (npm, GitHub Actions, releases) of this project dependencies Pull requests that update a dependency package or file. scripts Issues or pull requests regarding the scripts in scripts/. size:l [PR only] 101-1000 LoC changed labels Mar 26, 2025
@caugner caugner force-pushed the lefthook branch 12 times, most recently from 1835727 to adb7f02 Compare March 26, 2025 19:59
@ddbeck
Copy link
Contributor

ddbeck commented Mar 26, 2025

Does lefthook have something like Husky's HUSKY=0 envvar, to opt out of this? I rather dislike pre-commit hooks and don't wish to run them, if you apply this change (they're quite slow and disruptive, as somebody who commits a LOT, making heavy use of fixup and rebase).

@queengooborg
Copy link
Contributor

Git itself has a way to disable hooks when committing -- --no-verify

@ddbeck
Copy link
Contributor

ddbeck commented Mar 27, 2025

OK, it turns out there is an environment variable option LEFTHOOK=0—I don't know why I couldn't find it last night—so I'm +0 on this PR.

@queengooborg it's not a good alternative. Setting aside having to remember to set --no-verify, other tools won't necessarily pass such an option through to git-commit.

@caugner
Copy link
Contributor Author

caugner commented Mar 27, 2025

I rather dislike pre-commit hooks and don't wish to run them, if you apply this change (they're quite slow and disruptive, as somebody who commits a LOT, making heavy use of fixup and rebase).

As someone who commits a lot too, I understand your concern about hooks very well.

I have now tweaked the pre-push and post-merge hook:

  1. The pre-commit hook with lefthook now takes 1.02 seconds for me (M1 Macbook) on your draft PR, versus 2.94s with lint-staged.
  2. The pre-push hook (without npm run lint) now takes 3.18s for me.
  3. The post-merge hook (without npm run build) now takes 3.12s for me.

I would like to argue that these hooks are no longer slow and disruptive, and avoid you from running npm run lint:fix (on all files) manually.

@ddbeck Are these runtimes sufficiently low for you to reconsider keeping these hooks enabled in your BCD checkout?

@ddbeck
Copy link
Contributor

ddbeck commented Mar 27, 2025

@caugner ah yeah, that's really promising. I'm willing to give these a try. 👍

(also thank you for the nudge on the other PR—I completely forgot about it)

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot added the merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. label Mar 28, 2025
@github-actions github-actions bot removed the merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. label Apr 1, 2025
@caugner
Copy link
Contributor Author

caugner commented Apr 1, 2025

@queengooborg Are you okay with merging this?

@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2025

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot added merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. and removed merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. labels Apr 1, 2025
@caugner
Copy link
Contributor Author

caugner commented Apr 3, 2025

If nobody objects, I will merge this on Monday.

@github-actions github-actions bot added the merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. label Apr 4, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2025

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. label Apr 8, 2025
@caugner caugner enabled auto-merge (squash) April 8, 2025 08:52
@caugner caugner merged commit d3183ea into main Apr 8, 2025
11 checks passed
@caugner caugner deleted the lefthook branch April 8, 2025 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency package or file. infra Infrastructure issues (npm, GitHub Actions, releases) of this project scripts Issues or pull requests regarding the scripts in scripts/. size:l [PR only] 101-1000 LoC changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants