feat: run eslint/prettier fix against files as a pre-commit hook with lint-staged #5462

raymondfeng · 2020-05-16T23:51:45Z

Run prettier/eslint fix before git commit.

I have seen a lot of lint-related issues in PRs. Hopefully this pre-push
hook can decrease such problems and save us from keep telling contributors
to run npm run lint:fix.

A pre-push hooks is also added to run npm run build to prevent code that does not compile from being pushed to remote.

@frbuceta Thank you for the inspiration at #4330

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

Run prettier/eslint fix before `git commit`. I have seen a lot of lint related issues in PRs. Hopefully this pre-commit hook can minimize such problems and save us from keep telling contributors to run `npm run lint:fix`.

frbuceta

This was a feature that I proposed a long time ago, I like that it was implemented

bajtos

FYI: we have been discussing lint-staged in the past in #4330 and #4382

I see two problems in this proposal:

Linting only stages files may create false positives, where staged lint pass but full lint fails (see my first comment below).
New git hooks are IMO adding way too much friction. I don't mind to have these checks in place for others (if you like so), as long as there is an easy and well-documented way how to disable them in my local clone. Please note that I want to keep commit-msg check because I find that one as useful.

bajtos · 2020-05-18T07:03:13Z

lint-staged.config.js

+    const commands = [
+      `node packages/build/bin/run-prettier --write ${files.join(' ')}`,
+      `node packages/build/bin/run-eslint --report-unused-disable-directives` +
+        ` --fix --cache ${files.join(' ')}`,


Please note that with TypeScript-powered linter checks, it's not enough to lint the staged files. For example, when the signature of a method inside @loopback/context changes (e.g. a void function is returning a Promise now), we need to lint all packages using that particular method. Many of them will not pass no-floating-promises check.

bajtos · 2020-05-18T07:03:16Z

package.json

@@ -77,7 +78,9 @@
  "copyright.owner": "IBM Corp.",
  "husky": {
    "hooks": {
-      "commit-msg": "commitlint -E HUSKY_GIT_PARAMS"
+      "commit-msg": "commitlint -E HUSKY_GIT_PARAMS",
+      "pre-commit": "lint-staged",


Cross-posting from #4330 (comment):

In my experience, commit hooks are not very fast, which I find annoying and thus prefer to run checks when I decide on my own. Of course, that's just me, other people may have different preferences.

and from #4382 (comment):

My experience with pre-commit hooks is not the best, the hook is often run in situations when I don't want it to be executed (e.g. when performing git rebase) and thus slowing me down.

bajtos · 2020-05-18T07:05:00Z

package.json

-      "commit-msg": "commitlint -E HUSKY_GIT_PARAMS"
+      "commit-msg": "commitlint -E HUSKY_GIT_PARAMS",
+      "pre-commit": "lint-staged",
+      "pre-push": "npm run build"


This is even worse.

npm run build takes more ~~than a minute~~ a while to complete, that's way too long waiting just to be able to push my changes to github.

What if I have a work-in-progress branch that does not pass the build but I want the code to be backed up on GitHub and/or open a WIP pull request to discuss the early version? IIUC, this new hook will not allow me to do that.

bajtos · 2020-05-18T11:28:26Z

I have seen a lot of lint-related issues in PRs. Hopefully this pre-push
hook can decrease such problems and save us from keep telling contributors
to run npm run lint:fix.

If your main motivation for this pull request is to save typing the comments, then I'd like to offer few alternatives:

Use GitHub Saved Replies to quickly post a comment using the same text.
Move the CI linting step from Travis CI to GitHub actions and post a comment to the pull request if the linting failed. This opens new opportunities - we can distinguish between eslint and prettier errors and tell the user to run npm run prettier:fix if the problem is in formatting, or even fix formatting for them and commit the changes to their feature branch.

raymondfeng · 2020-05-18T16:17:38Z

My primary motivation is to minimize basic issues in PRs that fail CI builds and require manual ping-pongs.

To make sure commit messages follow our rules
To make sure npm run lint passes
To make sure npm run build passes

I can tolerate edge cases, for example, lint-staged misses a few eslint violations as our CI will catch that.

Please note the hooks can be skipped during rebase. See https://github.com/typicode/husky#skip-all-hooks-rebase.

We can also use -n or --no-verify on git commands to skip hooks. See https://git-scm.com/docs/git-commit#Documentation/git-commit.txt--n.

bajtos · 2020-05-19T07:22:36Z

My primary motivation is to minimize basic issues in PRs that fail CI builds and require manual ping-pongs.

Makes sense. I am afraid pre-commit hooks are not a very reliable solution for that, I have seen pull requests that were failing commit lint checks, despite the fact that we already have a pre-commit hook for that.

In general, I am reluctant to introduce duplicate checks for exceptional situations (a PR that fails CI on linting or build problem), because they negatively impact the usual path where developers run npm test before pushing to GitHub.

Maybe we should ask ourselves why are people not running npm test often enough? I am guilty too. Back in LB3 days, I was running npm test almost all the time because it took less than a minute to complete: full npm test in loopback takes 36 seconds and that includes Node.js tests, Karma tests in browser and linting, full npm test in strong-remoting is done in 12 seconds. Compare that with npm test in loopback-next that can take up to 9 minutes 😱

Instead of adding pre-commit/push checks forcing developers to suffer these long waits, I am proposing a different approach:

Create GitHub Actions to post comments to pull requests so that you don't have to post pings manually. We can even tweak this action to post the comment only to first-time contributors or perhaps only to people that are not maintainers.
Invest more time in optimizing our build & lint process. Our goal should be to speed up npm test to finish under 1 minute, so that contributors are not disincentivized from running npm test often.

Please note the hooks can be skipped during rebase. See https://github.com/typicode/husky#skip-all-hooks-rebase.

We can also use -n or --no-verify on git commands to skip hooks. See https://git-scm.com/docs/git-commit#Documentation/git-commit.txt--n.

Thank you for the pointers. Personally, I want to keep commit-message hook but skip all other hooks. I don't think that's possible using your proposed hooks and the options listed above.

If we decide to add new hooks, then let's add all of them to pre-push. That way I can create a git alias that will run push --no-verify, while keeping git commit with verification.

raymondfeng · 2020-05-30T05:05:28Z

If we decide to add new hooks, then let's add all of them to pre-push. That way I can create a git alias that will run push --no-verify, while keeping git commit with verification.

Unfortunately not all checks can/should be performed on pre-push. For example, lint-staged only works with pre-commit.

InvictusMB · 2020-05-31T10:11:54Z

+1 for pre-commit hook. It would save me tons of context switching.

I'm not in favor of pre-push check though. Build and test are something CI does well. And if either fails it probably highlights a bigger issue than a missed semicolon and would require a due attention. The primary goal of a hook is to address small nonsensial issues that are time killers otherwise.

Also the full test suite never runs successfully on my Windows machine.

I've made a similar PR #5616 with only a pre-commit and a small prettier tweak.

InvictusMB · 2020-05-31T10:21:30Z

@bajtos One could permanently disable hooks by adding HUSKY_SKIP_HOOKS=1 to his shell.

dougal83 · 2020-05-31T12:33:25Z

Also the full test suite never runs successfully on my Windows machine.

@InvictusMB Please raise an issue to investigate the issue as it would help to identify sources of friction with the development experience. Happy to take a look. 👍

bajtos · 2020-06-04T14:05:36Z

One could permanently disable hooks by adding HUSKY_SKIP_HOOKS=1 to his shell.

Thank you for the tip. Unfortunately, I don't want to disable all hooks - I want to keep the hook that's checking that my commit message is well formatted.

bajtos

Looks like I am the only person opposing the idea of lint-staged, which makes me feel a bit uncomfortable. I was thinking a lot how to move this PR forward in a way that will allow you all to have lint-staged checks while allowing me to opt out of them.

I am proposing to implement the following behavior:

If process.env.LINT_STAGED is set to string 0 or false, then no linting should be performed.
It would be great to implement & contribute this feature to lint-staged, but that's a long term goal.
For this pull request, we can implement support for LINT_STAGED ourselves. There are many ways how to accomplish that. For example, we can implement a wrapper script for lint-staged that will return immediately when LINT_STAGED is disabled. Another options is to leverage the fact that our lint-staged config is dynamically produced by lint-staged.config.js, we can return an empty config when LINT_STAGED is disabled.

Thoughts?

bajtos · 2020-06-05T12:08:59Z

lint-staged.config.js

+
+const path = require('path');
+
+module.exports = {


Suggested change

module.exports = {

if (['0', 'false'].includes(process.env.LINT_STAGED)) {

console.log('Skipping lint-staged as requested via LINT_STAGED env var.');

return;

}

module.exports = {

raymondfeng · 2020-08-07T17:09:48Z

Close it as #5616 has been landed.

feat(build): export eslint command

380d259

raymondfeng requested review from bajtos and emonddr as code owners May 16, 2020 23:51

raymondfeng self-assigned this May 16, 2020

raymondfeng added the Internal Tooling Issues related to our tooling and monorepo infrastructore label May 16, 2020

raymondfeng force-pushed the pre-push-lint branch from a59e753 to fab3a75 Compare May 17, 2020 01:14

raymondfeng requested a review from frbuceta May 17, 2020 04:18

chore: run build as a git pre-push hook

63e8f6e

frbuceta approved these changes May 17, 2020

View reviewed changes

bajtos suggested changes May 18, 2020

View reviewed changes

InvictusMB mentioned this pull request May 31, 2020

chore: lint staged files before commit #5616

Merged

7 tasks

bajtos reviewed Jun 5, 2020

View reviewed changes

raymondfeng closed this Aug 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: run eslint/prettier fix against files as a pre-commit hook with lint-staged #5462

feat: run eslint/prettier fix against files as a pre-commit hook with lint-staged #5462

raymondfeng commented May 16, 2020 •

edited

frbuceta left a comment

bajtos left a comment

bajtos May 18, 2020

bajtos May 18, 2020

bajtos May 18, 2020 •

edited

bajtos commented May 18, 2020

raymondfeng commented May 18, 2020

bajtos commented May 19, 2020

raymondfeng commented May 30, 2020

InvictusMB commented May 31, 2020

InvictusMB commented May 31, 2020

dougal83 commented May 31, 2020

bajtos commented Jun 4, 2020

bajtos left a comment

bajtos Jun 5, 2020 •

edited

raymondfeng commented Aug 7, 2020

feat: run eslint/prettier fix against files as a pre-commit hook with lint-staged #5462

feat: run eslint/prettier fix against files as a pre-commit hook with lint-staged #5462

Conversation

raymondfeng commented May 16, 2020 • edited

Checklist

frbuceta left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

bajtos May 18, 2020

Choose a reason for hiding this comment

bajtos May 18, 2020

Choose a reason for hiding this comment

bajtos May 18, 2020 • edited

Choose a reason for hiding this comment

bajtos commented May 18, 2020

raymondfeng commented May 18, 2020

bajtos commented May 19, 2020

raymondfeng commented May 30, 2020

InvictusMB commented May 31, 2020

InvictusMB commented May 31, 2020

dougal83 commented May 31, 2020

bajtos commented Jun 4, 2020

bajtos left a comment

Choose a reason for hiding this comment

bajtos Jun 5, 2020 • edited

Choose a reason for hiding this comment

raymondfeng commented Aug 7, 2020

raymondfeng commented May 16, 2020 •

edited

bajtos May 18, 2020 •

edited

bajtos Jun 5, 2020 •

edited