Dash-Industry-Forum / dash.js

A reference client implementation for the playback of MPEG DASH via Javascript and compliant browsers.
http://reference.dashif.org/dash.js/nightly/samples/dash-if-reference-player/index.html
Other
5.15k stars 1.68k forks source link

Remove `grunt-githooks` and use `precommit` script in `package.json` #3155

Closed ProLoser closed 4 years ago

ProLoser commented 4 years ago

I noticed this project uses grunt-githooks in order to enforce calling grunt lint as a pre-commit hook. It's unnecessary to use grunt-githooks for this purpose.

This code will accomplish the same task and would allow this project to reduce superfluous dependencies, especially considering the package was last updated 4 years ago.

// package.json
{
  // ...
  "scripts": {
    "precommit": "grunt lint",
    // ...
  }
  // ...
}

In my opinion, it would also be wise to add a new hook:

"preversion": "npm test"

After more probing, I think the way grunt-githooks is configured is either broken or confusing. Although the githook is called pre-commit it appears to be explicitly getting called in package.json anyway in prepublish

mlasak commented 4 years ago

Thank you for mentioning this. It is true that the outdated grunt-githooks dependency should be replaced. However, your suggested precommit "script" does not appear to be supported by npm. Which module are you recommending for this?

The functionality (lint before commit) should not get lost.

ProLoser commented 4 years ago

Oh man, you are right and I'm wrong. I completely forget my team uses https://github.com/typicode/husky which voids my argument for fewer dependencies 😅

Feel free to close this issue.

mlasak commented 4 years ago

We plan to remove the 'grunt-githooks' module. As you pointed out it is very out dated. I am not decided yet if to include husky as new dep or to go for a more lightweight approach.

Thanks again

ProLoser commented 4 years ago

I will say 1 thing: at work, I put lint checks behind git push instead of git commit. I feel it gives a better compromise between disrupting productivity and protecting the code quality. This way people can save there work locally without being nitpicked about potentially WIP code, but still prevents them from pushing the quality regressions to the server. I also felt it made more sense for the average frequency of git push to git commit.

mlasak commented 4 years ago

This makes fully sense to me.