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 "Move . to lint:js and lint:md" #17661

Closed
wants to merge 1 commit into from
Closed

Conversation

ndelangen
Copy link
Member

This reverts commit 7b03831.

Issue: The precommit hooks now run yarn lint on ALL files, not just the changed ones.

What I did

reverted the change see above

@nx-cloud
Copy link

nx-cloud bot commented Mar 8, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit 496b4a4. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen ndelangen requested a review from IanVS March 8, 2022 13:56
@ndelangen ndelangen self-assigned this Mar 8, 2022
@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Mar 8, 2022
@IanVS
Copy link
Member

IanVS commented Mar 8, 2022

Oh I didn't realize there's a pre-commit hook. It's kind of a bummer to use the same npm script for linting in CI and also linting individual files, since the cache will be constantly cleaned out due to running eslint against different files. I wonder if it might be possible to create a separate script for the pre-commit hook, with a different cache-location. Would you be willing to review a PR to try it out if I work on that real quick?

@ndelangen
Copy link
Member Author

constantly cleaned out due to running eslint against different files.

I am unaware of this. I'd assume eslint would keep a cache based on files, so running ona subset of files would not clear the cache. Is this an assumption? or do you have a link to the docs about this?

@IanVS
Copy link
Member

IanVS commented Mar 8, 2022

That's how it used to work, I'll do a little investigating.

@ndelangen
Copy link
Member Author

We could change the precommit hook of course, it's configured here:

storybook/package.json

Lines 88 to 95 in 7c87145

"lint-staged": {
"*.{html,js,json,jsx,mjs,ts,tsx}": [
"yarn lint:js --fix"
],
"package.json": [
"yarn lint:package"
]
},

@IanVS
Copy link
Member

IanVS commented Mar 8, 2022

Alright cool, either things changed or I was just wrong, but changing the target does not remove the cache. But running --fix does remove the file(s) from the cache (https://eslint.org/docs/user-guide/command-line-interface#--cache), but that's not a huge deal as long as husky is only running on the files that changed (oops).

I've opened an alternative to this revert PR at #17662. Please let me know what you think.

@ndelangen ndelangen closed this Mar 8, 2022
@stof stof deleted the tech/fix-commit-hooks branch August 31, 2022 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants