Husky rules are slow

[WilliamKelley]

Our husky pre-commit / pre-push rules can be very slow, like a minute slow. That's a very frustrating / tedious dev experience.

It seems like the test suite takes 5-15 seconds to run, if NX's cache is warm. And it seems like all tests run, not just for those components that have changed. The affected prefix that NX provides seems to determine by project (i.e. spark as a whole vs spark-e2e app), not diffing within a project.

Linting sometimes takes over a minute, even with a warm cache. It's very odd; I thought it was because all of our icon files, but i deleted all them to test and the lint still took forever. The answer might lie in NX's re-implementation of eslint; their documentation is subpar on this, but it appears that part of the lint or affected process compares the current branch to the last commit on main. So as long as the icon files are on main they'll crush linting times?? I'm really not sure, I couldn't verify this in testing, but also i couldn't avoid abysmal linting times. Maybe this will clear once we address #73 .

• I also tried the --uncommitted and other options, nothing helped in my experience.

A solution in the meantime is to delete pre-commit hooks, only leave pre-push -- they'll still be slow, but at least a dev can choose to push less often.

Another solution is to get rid of husky straight-out (often the recommended path) and write CI checks that fail a build if it fails a test (already done), has linting errors/warnings (i think this would be easy?), would be changed by formatting (i.e. prettier / format:write -- this might be the hardest one?)

Thoughts @seigler @shadoath ?

[seigler]

The long linting is absolutely due to the icon files. Before that it was quite fast, however linting as a hook has another problem, which is that since linting occurs after staging, the fixes are not included with the commit, and must be added and the commit amended. I would be fine with just a pass/fail lint in CI, with linting run manually. IMO pre-push Jest tests are still valuable but it seems fine to remove pre-commit hooks.

[landon-buttars]

The value of having pre-commit hooks for formatting and testing is it ensures code doesn't get committed (unintentionally) if it is broken or not linted properly.

If files are being linted when calling affected:lint that have not been changed i.e. all the icon files then the affected command is not doing it's job. Also, the format command runs prettier across all files in the project indiscriminately only abiding by the .prettierignore file. One of these two are causing the issues and I suspect it's the format command.

A good compromise could be to move the format command to the CI and only run affected:lint on the husky hook.

[WilliamKelley]

The value of having pre-commit hooks for formatting and testing is it ensures code doesn't get committed (unintentionally) if it is broken or not linted properly.

Yes that was the motivation for them. But when commits take over a minute, that's not acceptable. My proposed solution would ensure that a PR merge is blocked if it doesn't pass our rule checks.

... then the affected command is not doing it's job

It actually is doing it's job from what I can understand of the NX docs. It groups projects as affected or not, not individual files.

A good compromise could be to move the format command to the CI and only run affected:lint on the husky hook.

Can a CI job/step commit changes that format:write would make? That'd be nice

[landon-buttars]

My proposed solution would ensure that a PR merge is blocked if it doesn't pass our rule checks.

The issue with that approach is then the dev only knows there is an issue once they attempt to open a PR and isn't immediately obvious why the merge is failing.

Can a CI job/step commit changes that format:write would make? That'd be nice

Even if it can it would have to append a format commit which can get messy. (Ignore this if we're squashing commits.)

Sounds like there's an initiative to move the icons out of the spark library anyway which might fix the issue outright.

[landon-buttars]

And if all else fails... https://github.com/azz/pretty-quick

[WilliamKelley]

Yeah the alternatives aren't ideal, and we are squashing commits.

Also I can confirm that format is not the longer step, its always lint that's abysmal

[WilliamKelley]

Our convo about format does bring up another notable point though. This probably deserves another issue but ill preface it here for initial feedback, When format:write runs as a pre-commit, sometimes the writing won't be captured in the ensuing commit. I use an autoformat prettier extension, so my files rarely are affected; I've noticed it a couple times but haven't exactly honed in on the cause. My first guess is that it has trouble with partially staged files, but I also remember it happening when everything was staged, so it literally just might not be the right command for pre-commit formatting as it doesnt stage its writes.

Examining https://prettier.io/docs/en/precommit.html , we might want to find a parallel to Option 4) there

[WilliamKelley]

I'm realizing we can just add the icon file folders to .eslintignore and .prettierignore ...

[WilliamKelley]

Rules are no longer slow with merging of icon PRs and addition of folders to respective ignores.