Browser-compat-data: Add a pre-commit hook that executes tests

[Elchi3]

Over at interactive-examples, we run a neat pre-commit hook that runs tests before a git commit. I'd like to add the same here, because we're still getting PRs that fail on travis.

[ExE-Boss]

I just hope it will work on Windows, as I tend to disable git hooks due to compatibility issues with my environment.

[jwhitlock]

I'm opposed to pre-commit hooks. I find they cause more issues then they solve, including lost data when rebasing. I prefer running the test suite in a continuous integration environment like TravisCI.

[Elchi3]

@wbamberg @schalkneethling what's your experience with pre-commit hooks in interactive-examples? Worth it or troublesome?

[schalkneethling]

@Elchi3 on interactive examples I reckon they are very useful. I am a fan ;) so might be biased. I do agree that running during CI is great, but that little nudge pre-commit ensure that things are caught early.

If everyone used the same editor config with prettier, eslint etc. configured etc. then I would say pre-commit hooks are not so important.

The other option is to make npm scripts available that runs formatting and linting checks, and document these. There is then a certain trust, or understanding that contributors "should" run these manually before committing. The understanding being that, if they do not, and their PR fails on CI because of formatting rules or linting errors, the time it takes to review the PR will be affected, and it is their responsibility to ensure CI passes.

You can see samples of such scripts here:
https://github.com/schalkneethling/iex-pen/blob/master/package.json#L7

[schalkneethling]

Also, if you do decide to go this route, I would highly recommend using Husky ~ https://www.npmjs.com/package/husky

[ddbeck]

One additional thing to note about Git hooks: even if you include the scripts in the repo, hooks require some local setup. You can't guarantee that someone who clones a repo will necessarily set up the hooks (e.g., you have to get them to run git config core.hooksPath <something> or symlink the scripts into .git/hooks or do those things during npm install, none of which happens on git clone).

I'm a fan of hooks—I've got several private BCD helper scripts myself—but they seem to be surprising or frustrating sometimes, more if you don't use them routinely. I like the idea of populating /scripts and package.json with some commands and documenting them (including their use as hooks). We could even clean up our CI output a bit and, on failure, recommend using those scripts with CI Reporter.

[schalkneethling]

@ddbeck Husky will take care of that for you. https://github.com/typicode/husky/blob/master/DOCS.md

[ddbeck]

@schalkneethling right, but the users still need to run npm install to get Husky. I'm pretty sure lots of first-time contributors to this repo aren't doing that (they're directly editing the JSON, sometimes directly on GitHub).

[Elchi3]

So, it seems like this would mostly affect the workflow of core contributors then (those with a local set-up for bcd). I'd be happy to hear more thoughts from core contributors on this then to make a call here.

[ddbeck]

I guess if we're not worried about _enforcing_ hooks usage and it's more about making things easier for regular contributors, then let's add some hooks, use Husky, and see how we like it? If it's an annoyance, we can always revert Husky as a dependency, making the hooks optional.

[Elchi3]

Sounds good to me.
https://github.com/mdn/browser-compat-data/pull/3329 is ready for review then I guess.

[wbamberg]

Sorry to be slow on this thread.

I think one problem about pre-commit hooks is this: people often seem to get lint errors that they don't understand and can't fix on their own. If these errors are found post-commit, then we can see the errors and help people fix them. If they are pre-commit, then they are more likely to get stuck.

I also like the idea of minimizing the number of packages we install on people's computers, and think we shouldn't add more dependencies lightly.

As a core contributor to browser-compat-data, I always run lint anyway because I don't want to make broken PRs. I also test my changes with the {{Compat}} macro. So lint it just a part of the checks I go through here, and not the most time-consuming part.

For interactive-examples, we used to have them and tbh I found they often didn't seem to work properly. We don't currently use them (AFAICT) although we do have the on bob repo.

[Elchi3]

Not being able to see where a contributor is stuck is actually an excellent argument for not adding it! Thanks Will, I hadn't thought about that.

[Elchi3]

Closing, as there seem to be more disadvantages than advantages. Re-open or comment if you disagree.