Add husky pre-commit hook, add lint-staged

[rickycodes]

This gives us a pre-commit hook via husky and lint-staged similar to what we do in metamask-mobile

[whymarrh]

I have a few reservations about adding Husky to the project, where it runs arbitrary code on each git command. Namely:

1. I don't want to be required to have to set HUSKY_SKIP_INSTALL to prevent arbitrary code running on each Git command. (i.e. I don't want this to be opt-out.)
2. I see each dependency we add to the project as a liability. A dependency that intentionally runs arbitrary code as a very large liability.

I would be open to, instead of Husky, adding a prominent note the the README.md about how a user can opt-in to having a hook installed. e.g. cp development/hooks/pre-commit .git/hooks/pre-commit.

On lint-staged: I'm cool with it. Can we move the config to a .lintstagedrc file? I don't quite want the package.json to become a collection of configs.

[whymarrh]

I see each dependency we add to the project as a liability. A dependency that intentionally runs arbitrary code as a very large liability.

I think this is somewhat reflected in the work @kumavis has done to lockdown what's run on install and what can touch what files (e.g. #7208 and #7210).

[rickycodes]

My thinking here is that we're already using this in metamask-mobile so maybe some commonalities between the two repos would be nice? Personally I find it rather useful, but I understand your reservations.

However, I think when it comes to new contributors on-boarding, having something like this is a big help.

re: opt-out: I don't think it's that hard to append --no-verify to the git commit command and if you care about it enough you could just permanently add that to the [alias] section of your ~/.gitconfig

[whymarrh]

re: opt-out. I don't think it's that hard to append --no-verify to the git commit command and if you care bout it enough you could just permanently add that to the [alias] section of your ~/.gitconfig

This seems heavy-handed—why would I want to "bypasses the pre-commit and commit-msg hooks" permanently? What if I have custom personal hooks installed? Or hooks in a different repo?

I think this is a classic opt-in vs. opt-out debate, and I think as a security & privacy-focused product we should be extremely intentional about what we require opt-out for, and I think requiring users to opt-out of arbitrary code execution is a no-go. I understand that this could be/is useful for some devs, but I don't see a way to make this remotely secure.

[rickycodes]

This seems heavy-handed—why would I want to "bypasses the pre-commit and commit-msg hooks" permanently? What if I have custom personal hooks installed? Or hooks in a different repo?

I mean... I don't understand why you would not want to lint your JavaScript before pushing it to a remote only to discover the build broke there?

If we're really concerned about this from a security stand point perhaps we should consider removing it from the mobile repo?

[whymarrh]

I don't understand why you would not want to lint your JavaScript before pushing it to a remote only to discover the build broke there?

This doesn't need to exist for me to be able to run yarn lint:staged or yarn test (or use my editor to run lint or tests). I can do that already.

If we're really concerned about this from a security stand point perhaps we should consider removing it from the mobile repo then?

I want to make space for other devs to chime in here, but I would't be opposed it. gaba also uses it and I would support removing it from there as well.

As another alternative, we could update the README.md to offer users who want hooks to use core.hooksPath, as introduced in Git 2.9.^[1] For example:

git config core.hooksPath ./development/hooks
yarn
yarn start

This approach is opt-in, and still allows us to update the hooks with the repository without requiring users to rerun a command.

[rickycodes]

closing this to implement what was discussed here in a separate PR