tooling: update pre-commit hook to stop failure propagation of no-import rule of missing dependencies #5430
Conversation
🦋 Changeset detectedLatest commit: f32711d The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsChromeicon permalinktest-basic
Firefoxicon permalinktest-basic
|
packages/icon/src/IconBase.ts
Outdated
| @@ -33,7 +33,6 @@ export class IconBase extends SpectrumElement { | |||
| public static override get styles(): CSSResultArray { | |||
| return [iconStyles]; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Did you mean to remove this whitespace? It seems useful to delineate the two properties.
There was a problem hiding this comment.
Ah! I was testing the linting with some additional fail checks here. Should be removed. Thanks for catching this
| @@ -88,7 +88,8 @@ | |||
| ], | |||
| "dependencies": { | |||
| "@spectrum-web-components/base": "1.6.0", | |||
| "@spectrum-web-components/iconset": "1.6.0" | |||
| "@spectrum-web-components/iconset": "1.6.0", | |||
| "@spectrum-web-components/reactive-controllers": "1.6.0" | |||
| STAGED_FILES_TO_ANALYZE=$(git diff --name-only --cached --diff-filter=d -- "{packages,tools}/*/src/**/!(*.css).ts") | ||
| STAGED_CSS_FILES=$(git diff --name-only --cached --diff-filter=d -- "{packages,tools}/**/*.css") | ||
| VERSION_FILE=$(dirname "$0")/../tools/base/src/version.js | ||
| #!/bin/bash |
There was a problem hiding this comment.
Yeah I've noticed this issue as well, however here's the documentation around why we added the bash wrapper: https://typicode.github.io/husky/how-to.html#bash
We're not currently supporting Windows so it seemed a fine compromise. The if syntax however is Bash syntax so removing that would be a potential issue. What do you suggest?
There was a problem hiding this comment.
I was thinking we could migrate all these rules into a lint-staged config and just run the yarn lint-staged command here instead of all this logic. What are your thoughts? Was playing around with that here: #5393
There was a problem hiding this comment.
I like the idea of migrate these in a module in a lint-staged config. More scalable. Let me update this here and test. Please feel free to add your thoughts too!
Description
This PR updates the
pre-commithook to reliably lint, analyze, and format only the staged files using ESLint, Lit Analyzer, Stylelint, and Prettier. The script has been rewritten as a standalone shell script with strict error handling usingset -e. It ensures that the commit process is blocked if any linting or analysis tool fails. Additionally,genversionis executed and the resulting file is re-staged for inclusion in the commit.Related issue(s)
Motivation and context
Previously, the
pre-commithook used abash << EOFheredoc block, which caused failures in expected linting behavior. Errors from linters were not blocking commits as expected due to the lack of strict failure propagation in the heredoc context. This update resolves that by using a standard shell script with correct error handling and scoped linting to staged files only.How has this been tested?
Manually tested in a local development environment using the following steps:
Test case 1
.tsfile that introduces an ESLint error.Test case 2
.cssand.tsfiles without lint issues.Test case 3
@spectrum-web-components/reactive-controllerfrom the dependencies inpackages/icon/package.json.import/no-extraneous-dependenciesrule is blocking the commit due to missing dependenciesDid it pass in Desktop?
Did it pass in Mobile?
Did it pass in iPad?
Screenshots (if appropriate)
N/A
Types of changes
Checklist