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

Revert "Add svelte check in pre-commit hook" #702

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

efstajas
Copy link
Contributor

@efstajas efstajas commented Oct 2, 2023

IMO there are a few workflow problems with this:

  • It runs the check on the entire repo, not just on your staged changes (like the linter does), so you are blocked from committing a perfectly valid change if you have any in-progress unstaged changes elsewhere in the repo that svelte-check doesn't like.
  • It practically makes the VSCode commit button useless, at least on my macbook — takes anywhere between 30 seconds to 1 minute to commit. It's a bit better, but still pretty slow on the terminal, especially if you're making many commits in a row.
  • If your workflow is to make a lot of changes, and then selectively stage & commit to break them into multiple chunks, you now have to wait for svelte-check to scan the entire repo for every individual commit, even if you already ran svelte-check before and know there's no issues in the repo. You can skip commit verification of course, but then you're also skipping lint-staged and may commit badly formatted code.

IMO, it's enough to have svelte-check ran as a check on PRs, which still finishes pretty quickly and effectively prevents bad changes from ending up on main. Of course, you can also run npm run check on-demand anytime you want locally.

@efstajas efstajas requested a review from jtourkos October 2, 2023 15:28
@netlify
Copy link

netlify bot commented Oct 2, 2023

Deploy Preview for drips-app-v2 ready!

Name Link
🔨 Latest commit ce7424b
🔍 Latest deploy log https://app.netlify.com/sites/drips-app-v2/deploys/651ae18a4c9c440008e27cee
😎 Deploy Preview https://deploy-preview-702--drips-app-v2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@efstajas efstajas requested a review from evvvritt October 2, 2023 15:28
@efstajas efstajas merged commit cbcaee9 into main Oct 3, 2023
9 checks passed
@efstajas efstajas deleted the revert-665-add-svelte-check-in-commit-hook branch October 3, 2023 09:49
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.

2 participants