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

set up lint-staged and husky #788

Merged
merged 4 commits into from
Jan 17, 2024
Merged

set up lint-staged and husky #788

merged 4 commits into from
Jan 17, 2024

Conversation

stephenkilbourn
Copy link
Contributor

closes #787

This adds husky and lint-staged to run the lint and typecheck on only staged files. I had to pin lint-staged at an older version because they dropped support of node 16 with their last major release. Testing it out things work for me. I think it should be noted that anyone modifying an existing file that already has errors will need to either fix them as part of their MR or bypass the hook.

Copy link

netlify bot commented Jan 6, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit fd12c39
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/65a84ac3c50eb80008e2304f
😎 Deploy Preview https://deploy-preview-788--veda-ui.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.

@j08lue j08lue requested a review from sandrahoang686 January 8, 2024 15:46
@hanbyul-here
Copy link
Collaborator

Thank you so much for your contribution!
I just gave it a try. I wonder 1. if we can configure it to show where errors/warnings are so it helps me to fix them. 2. if we can be selective about whether to ignore the warnings (only care about errors - I can't commit with the current configuration now which I am assuming because of lint warnings - but then it is difficult to spot where those warnings are from the terminal.).

@stephenkilbourn
Copy link
Contributor Author

stephenkilbourn commented Jan 11, 2024

@hanbyul-here - good catch. lint-staged was actually failing and swallowing the error. I should have tested the happy path better 🤦

I added the missing yarn in the command. I also explicitly call eslint and stylelint on separate lines. The existing lint script with them command overrides the lint-staged feature by passing the apps/scripts path. If I call that, it runs the lint against unstaged files, too. It should only fail on errors and not warnings now.

Copy link
Collaborator

@sandrahoang686 sandrahoang686 left a comment

Choose a reason for hiding this comment

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

Pulled and ran through a test commit, lgtm

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

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

I made a small documentation pr as a follow-up: #798
Thanks for your contribution again 🙇

I was looking to see how I could bypass the pre-commit hook and I
thought, why not document it (knowing that I will need to look it up
again_ I won't bypass too much I promise.)
@stephenkilbourn stephenkilbourn merged commit 628f6d0 into main Jan 17, 2024
8 checks passed
@stephenkilbourn stephenkilbourn deleted the precommit-linting branch January 17, 2024 23:03
@hanbyul-here
Copy link
Collaborator

hanbyul-here commented Jan 22, 2024

I noticed that ts-check in the pre-commit stage gives an error incorrectly.
ex. When I made a misc change to common/layout-root, it throws an error that 'app/scripts/components/common/layout-root.tsx(19,37): error TS2307: Cannot find module '$utils/use-google-tag-manager' or its corresponding type declarations.'
It seems that precommit check only have access to the staged files? Do you know how we can address this problem? (Try this pr for testing: #808) @stephenkilbourn ?

sandrahoang686 added a commit that referenced this pull request Feb 13, 2024
https://github.com/NASA-IMPACT/veda-ui/releases/tag/v4.1.0

## 🎉 Features
- Experimental CMR Layer:
#805

## 🚀 Improvements
- Style 
- Documentation
- Rename critical error page:
#806
- Add back button on Exploration page:
#779
- Add ts, lint check for precommit hooks:
#788,
#809
- "skip to main" button for navigation:
#808

## 🐛 Fixes
- Return datasets even when there is a dataset without summaries:
#786
- Show all the datasets on Data Catalog page:
#837
- Block Map user defined position fix:
#784
- Geocoder centering on various projecctions:
#826
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 a precommit hook for eslint/ts error
3 participants