build: add pre-commit and pre-push hooks

[easymikey]

Add lefthooks to pre-commit and pre-push git hooks.
Why lefthooks?

• parallel hook execution
• easy configuration
• lightweight and fast
• minimal deps

Fixes #1002

• Tests pass
• Appropriate changes to README are included in PR

[nikelborm]

Why add a git hook? Why not check it from GitHub Action?

Using Github Action gives much better DevX because git hooks make every commit longer or even prevent people from committing and pushing. There's nothing more irritating than when you can't save and sync your own work to the cloud. And there's no reason to prevent people from committing to their own forks. Nobody here pushes directly to the main branch and you don't need to prevent commits before they land.

The only suitable case for git-hooks is to run checks to prevent accidental commit of credentials -- because this is truly dangerous and we consciously want to prevent such commits before they're synced to the cloud because it's impossible to remove them after it happens (yes, even git push --force won't help)

[easymikey]

GitHub Action

Are you suggesting using github action on push or precommit?

CI already has a check on these jobs, but we wants to check the correctness as early as possible.

@antongolub, what do you think?

[nikelborm]

I doubt it's possible to use GitHub action on pre-commit so my suggestion was to use GitHub Action after push with repo policy that forbids merging PRs where actions are failing. If there are already such Github actions, then nothing should be done because of the arguments against hooks, mentioned above

[easymikey]

I doubt it's possible to use GitHub action on pre-commit so my suggestion was to use GitHub Action after push with repo policy that forbids merging PRs where actions are failing. If there are already such Github actions, then nothing should be done because of the arguments against hooks, mentioned above

Now github actions used before merge and launched only by the mantainers. But contributors do not formatted code or starts test. It is doubtful to drive ci every push.

[nikelborm]

Running Github Actions is free and doesn't require any manual clicks to be done, even though Actions can be configured to require approve first from a trusted party before running. Running tests and other checks as well on each PR on every commit should be a norm. And it is the case for thousands of repos on Git Hub. And if they aren't being run automatically in this repo NGL I'm quite shocked.

[easymikey]

Running Github actions is free and doesn't require any manual steps to be run. Running tests and other checks as well on each PR on every commit should be a norm. And it is the case for thousands of repos on Git Hub. But when I looked closer and noticed that they weren't being run in this repo NGL I was quite shocked.

Yes, it is free, but it will take a few minutes until all the jobs are completed and the developer gets the result and will have to run all the jobs locally. But only mentainers can tell the logic of why this path is chosen.

[nikelborm]

Running Github actions is free and doesn't require any manual steps to be run. Running tests and other checks as well on each PR on every commit should be a norm. And it is the case for thousands of repos on Git Hub. But when I looked closer and noticed that they weren't being run in this repo NGL I was quite shocked.

Yes, it is free, but it will take a few minutes until all the jobs are completed and the developer gets the result and will have to run all the jobs locally. But only maintainers can tell the logic of why this path is chosen.

So don't execute them in sequence. Run them in parallel. Problem solved. If it's especially important to not spend the cloud's compute time, in GitHub Actions there is always an if field to exclude some steps from running. An example is when some steps are run only when PR changes a specific directory. With git hooks, it always starts small and then it always turns into a mess when you now have to wait a minute for every commit locally. Execution of GitHub actions happens on a server (which I believe is usually faster than local machine) and doesn't block a committer from continuing their work, whereas adding a Git hook does.

[mxmvshnvsk]

@nikelborm Hi. The topic of implementing hooks is useful for skipping basic problems with formatting and running tests. In actions, there is a problem in the launch stage that developers only find out about problems after launching the maintainer's actions (it tires to keep everything in mind). It seems to me that some compromise can be found, for example, tests should be run in actions, and formatting and linting should be run in a pre-commi hook.

[easymikey]

Running Github actions is free and doesn't require any manual steps to be run. Running tests and other checks as well on each PR on every commit should be a norm. And it is the case for thousands of repos on Git Hub. But when I looked closer and noticed that they weren't being run in this repo NGL I was quite shocked.

Yes, it is free, but it will take a few minutes until all the jobs are completed and the developer gets the result and will have to run all the jobs locally. But only maintainers can tell the logic of why this path is chosen.

So don't execute them in sequence. Run them in parallel. Problem solved. If it's especially important to not spend the cloud's compute time, in GitHub Actions there is always an if field to exclude some steps from running. With git hooks, it always starts small and then it always turns into a mess when you now have to wait a minute for every commit locally. Execution of GitHub actions happens on a server and doesn't block a committer from continuing their work, whereas adding a Git hook does.

I partially agree with your point, but I would like to add that running tests locally can help developers take more responsibility for their code. However, it's also important to understand why the team behind the project has not implemented a launch flow for every push.

[easymikey]

@nikelborm Hi. The topic of implementing hooks is useful for skipping basic problems with formatting and running tests. In actions, there is a problem in the launch stage that developers only find out about problems after launching the maintainer's actions (it tires to keep everything in mind). It seems to me that some compromise can be found, for example, tests should be run in actions, and formatting and linting should be run in a pre-commi hook.

Hi @mxmvshnvsk , I understand your point, and I agree that there needs to be a compromise of some kind.

[easymikey]

I wonder how many more people will join us 😄

[nikelborm]

The compromise I found in my company was that hooks are being run for:

1. checking the presence of accidental push of credentials
2. other security related things
3. checking the presence of big files
4. validating git commit message
5. other smallest possible things that don't require more than half a second to run

Everything else is on GitHub Actions. It may seem surprising but even running linter and formatted can be quite heavy especially if you're a fan of something similar to https://github.com/EvgenyOrekhov/eslint-config-hardcore like me. NGL if you run all the checks on at least somewhat large project, the usual person's computer won't be usable during the check. That's why we moved even that to Actions.

[nikelborm]

If a dev wants to merge something to an opensource repo it's their responsibility to follow the rules of the repo. If tests that were run automatically on your PR didn't pass, it won't get merged. Plain and simple.

Responsible devs will run the tests and checks locally anyway or if they aren't sure that their code passes them. But the repo shouldn't take away the possibility to

1. choose whether to run them or not
2. ability to run them in parallel while doing other work and in a non-blocking way

[easymikey]

If a dev wants to merge something to an opensource repo it's their responsibility to follow the rules of the repo. If tests that were run automatically on your PR didn't pass, it won't get merged. Plain and simple.

Responsible devs will run the tests and checks locally if they aren't sure that their code passes them. But the repo shouldn't take away the possibility to

1. choose whether to run them or not
2. ability to run them in parallel while doing other work and in a non-blocking way

You're talking about repository rules, but your ideas are just suggestions. The rules are set by the maintainers, and if they decide that a precommit hook is necessary, you will have to follow these rules.

It seems that you propose your own rules, and do not take into account the opinions of the maintainers, which sounds strange. What is strange to you may not be strange to others. Therefore, it makes no sense to discuss this issue without the maintainers.

[nikelborm]

I know that. And also I believe that sharing knowledge, experience, review, and feedback as Github's platform allows us to do freely (since it's an opensource repo) is a good and productive thing. I'm giving the other side of the story to a question of adding git-hooks. I've seen it ending badly with an awful developer experience and I'm trying to share what I faced and warn others.

---

Having automated tests in the cloud doesn't prevent the devs from running the tests locally. But running tests or other heavy stuff (it's a slippery slope once git-hooks are introduced) on each commit locally in a blocking way may lead to quite bad consequences, especially for new contributors with low-end hardware, and may become a big stopping factor for them.

[easymikey]

I know that. And also I believe that sharing knowledge, experience, review, and feedback as Github's platform allows us to do freely (since it's an opensource repo) is a good and productive thing. I'm giving the other side of the story to a question of adding git-hooks. I've seen it ending badly with an awful developer experience and I'm trying to share what I faced and warn others.

Having automated tests in the cloud doesn't prevent the devs from running the tests locally. But running tests or other heavy stuff (it's a slippery slope once git-hooks are introduced) on each commit locally in a blocking way may lead to quite bad consequences.

I agree with you, each approach has its pros, cons, and limitations. But I think you should not just describe that it's bad, but add justifications and suggestions. This is necessary in order to immediately familiarize yourself and think about what you are offering.

[nikelborm]

I added both justifications:

1. git hooks make creating every commit take more time.
2. There is nothing more irritating than when you can't save and sync your own work to the cloud.
3. There's no reason to prevent people from committing to their own forks
4. Nobody here pushes directly to the main branch and there are not many reasons to prevent commits before they're pushed
5. Running Github Actions is free
6. Execution of GitHub actions happens on a server (which I believe is usually faster than a local machine), so it's faster than running local hooks
7. Running GitHub Action doesn't block a committer from continuing their work, whereas adding a Git hook does
8. Running heavy stuff on each commit locally in a blocking way may become a big stopping factor for new contributors with low-end hardware
9. Github actions can easily be configured to run after every commit and are quite flexible in cases when you need more granular control over their execution

and suggestions:

1. run git-hooks for
   1. checking the presence of accidental push of credentials
   2. other security related things
   3. checking the presence of big files
   4. validating git commit message
   5. other smallest possible things that don't require more than half a second to run
2. run everything else on GitHub Actions
   1. tests
   2. heavy checks
   3. formatting
   4. linting

[easymikey]

I added both justifications and suggestions

They are scattered, and the maintainers may not read them all to become familiar with them. Therefore, I suggest putting everything in one message for you, and it would be better to use the features and benefits of one of the mantainers.

[nikelborm]

suggest putting everything in one message for you

I was just in the progress of doing that

[antongolub]

@easymikey, @mxmvshnvsk, how about this?

pre-push:
  parallel: true
  commands:
    format:
      glob: '*.{js,ts,md,yml}'
      run: npm run fmt:check
    license:
      run: npm run test:license
    size:
      run: npm run test:size
    circular:
      run: npm run test:circular

[mxmvshnvsk]

Looks good.

[easymikey]

@easymikey, @mxmvshnvsk, how about this?

pre-push:
  parallel: true
  commands:
    format:
      glob: '*.{js,ts,md,yml}'
      run: npm run fmt:check
    license:
      run: npm run test:license
    size:
      run: npm run test:size
    circular:
      run: npm run test:circular

These are lightweight checks and they should definitely be added to the pre-commit process.

Maybe we could add a simple task to check the tests after each push?

[easymikey]

one second per run

[antonmedv]

On pre-push.

[easymikey]

On pre-push.

Are you suggest to return the pre-push hook or add all from pre-commit to pre-push?

[antongolub]

Are you suggest to return the pre-push hook or add all from pre-commit to pre-push?

Long tests suites should be processed via CI, annoying fast checks — on pre-push step.

[easymikey]

Are you suggest to return the pre-push hook or add all from pre-commit to pre-push?

Long tests suites should be processed via CI, annoying fast checks — on pre-push step.

Replace

[antongolub]

lgtm