Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add pre-commit hook for prettier + eslint #32

Closed

Conversation

with-heart
Copy link
Contributor

Closes #30

This change adds a pre-commit hook via husky and lint-staged, which enables the configured tasks to be run on staged changes. With this configuration, prettier and eslint are run on their respective files.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@with-heart with-heart requested a review from nihaals March 5, 2021 19:13
Copy link
Member

@nihaals nihaals left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I "like" (it might be just that I'm used to it, it can be annoying though when you're making multiple commits in one go by staging parts of files) when using pre-commit is that when a formatter makes a change, it aborts the commit and you have to stage the new changes before committing again, while this changes the staged file (not sure how this works, maybe just immediately staging the lines that were affected?) and doesn't neccesarily make it clear it made changes before letting you enter a commit message (assuming no -m). This causes some slightly weird things when staging only parts of files when you might want more control. I'm undecided if this should block this though, especially with husky being the standard tool for this (not sure if lint-staged is responsible for this behaviour or husky). My tests weren't super realistic though, this may be a non issue in a more standard file.

}
},
"lint-staged": {
"*.{js,jsx,ts,tsx,md,mdx,yml,json}": "prettier --write",
Copy link
Member

@nihaals nihaals Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yaml isn't included here. Is this intentional (and trying to set a standard of using yml)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I'll add yaml. Was just following the pattern that the only yaml files in the project currently are yml but they're both totally fine.

@with-heart
Copy link
Contributor Author

My personal workflow utilizes heavy partial staging for atomic commits, and I haven't noticed an issue with this on any of the projects that I contribute to with the same or similar setups. From what I could find in the lint-staged issues, they added support for partial staging quite some time ago. With that said, I've never directly verified that it works, so maybe it has been subtly committing the wrong things for a long time and I've just never noticed 🙃 Worth testing a little more!

@MythicManiac
Copy link
Member

I would feel more comfortable using pre-commit, as that's what I have experience with and know the ecosystem around (not to mention it's already used in the backend repo), but I'm also not opposed to using husky if it's what's better known in the javascript world.

That being said, I do have some questions left unanswered after skimming through husky docs:

  • Can we run this on all files in the repo? With pre-commit, you can do pre-commit run --all-files, which makes it easy to run checks on the CI runner.
  • What are the hooks based on? Shell scripts? Javascript? Interestingly enough the documentation doesn't seem to mention how would you actually implement hooks if you needed to
  • What kind of an ecosystem is there around husky? Are there useful hooks that we could easily depend on, or is it more common to roll your own?

One more thing that I find particularly annoying is if this workaround is an actual requirement

@nihaals
Copy link
Member

nihaals commented Mar 7, 2021

  • Can we run this on all files in the repo? With pre-commit, you can do pre-commit run --all-files, which makes it easy to run checks on the CI runner.

We could probably add a lint script to the package.json that just runs prettier and eslint. We could then add one for writing the changes and one which just runs as a check to support different use cases.

One more thing that I find particularly annoying is if this workaround is an actual requirement

Not sure if you can just use cmd or PowerShell but if not we could recommend WSL since that seems to be what a lot of devs on Windows are using now, especially with the WSL support in VSCode. Also #38 would also help but users wouldn't be able to use a git GUI other than VSCode's if they're used to it (although maybe they support using a custom shell?). Even then you could use the GUI, just not for actually committing. My workflow at the moment is a mix between git add, VSCode's VCS and GitKraken to stage changes then I always use git commit myself (partially due to my PGP key's passphrase being saved only when running it myself).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up husky+lint-staged+prettier
3 participants