-
Notifications
You must be signed in to change notification settings - Fork 9
chore: update stylelint to v16 #84
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
Conversation
|
Thanks for the work on this! I have one comment above. I also was wondering if we might leverage some AI to generate a few more stylelint tests... I would be happy to take that on after this is merged if that is preferable. |
| declaration-colon-space-after: always | ||
| declaration-colon-space-before: never | ||
| declaration-block-trailing-semicolon: always | ||
| declaration-empty-line-before: never |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know there is a desire for more flexibility with empty lines, and I've been thinking a lot about #82.
While I understand this perspective, I have some concerns about removing this constraint entirely and allowing arbitrary spacing based on personal preference. Without any constraints, we might see inconsistent spacing patterns like this example:
.btn-tertiary {
background: gray;
color: white;
padding: 0.5rem 1rem;
border: none;
margin: 10px;
&:hover {
background: darkgray;
}
&:focus {
outline: 2px solid lightgray;
}
}Regarding the other PR's concerns about our blank line principles: while I recognize that the current rules may feel restrictive, I don't believe they actually violate our stated principle. The principle states "Related statements that accomplish a single task should be grouped together without blank lines. Use one blank line to separate unrelated statements."
I'd argue that allowing arbitrary spacing between property declarations would violate this principle. While color and background-color may seem unrelated as individual properties, they're logically grouped because they all relate to the parent selector and together form the complete specification for that element. Many properties are closely related (such as display: flex; flex-direction: row;), and removing this rule would rely solely on convention and code review to prevent their separation.
As a potential middle ground that addresses both concerns, I'd suggest this rule set as a step toward more permissive formatting:
'@stylistic/declaration-empty-line-before': never
'@stylistic/rule-empty-line-before':
- always
- ignore:
- after-comment
- first-nested
- blockless-after-blockless # Helps with @else
- ignoreAtRules:
- else # Specifically ignore @else spacing
- else-if # And @else if
This approach would enforce separation of nested blocks (better aligning with our blank line principles) while improving readability. If a set properties is excessively long they can easily be separated visually and with explanation by code comments. I believe these rules should be auto-fixable, allowing older codebases to be updated automatically, though this should be validated.
With these proposed rules, the formatting would look like this:
.btn-tertiary {
// Colors
background: gray;
color: white;
// Spacing
padding: 0.5rem 1rem;
margin: 10px;
// Decoration
border: none;
&:hover {
background: darkgray;
}
&:focus {
outline: 2px solid lightgray;
}
@if $theme == 'dark' {
border: 1px solid white;
} @else {
border: 1px solid black;
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was actually my intention with this PR to keep the result as close to our existing standards as possible, to avoid any linting/standards discussions for the upgrade PR if we could.
In the PR description above, I noted:
A few rules were not moved there and are now removed from this config:
- declaration-empty-line-before: never
- rule-empty-line-before: never
so unfortunately both of these rules are among the ones that were not re-implemented in the @stylelistic repo and the proposed config just results in "rule not found" errors.
I don't disagree with the need for some kind of constraints on blank lines, and at first glance I like the results of the proposed config! We'll just need to implement those two rules ourselves as a plugin in a subsequent PR.
Sorry, I should've called that out more clearly, especially since there was an open issue about the blank line standards 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed separately, many stylistic rules were moved to stylistic... but the ones I commented on you are saying were not available are not in the deprecated list and thus not moved. They still exist in stylelint code and documentation as far as I can tell.
- https://github.com/stylelint/stylelint/tree/main/lib/rules/declaration-empty-line-before
- https://github.com/stylelint/stylelint/tree/main/lib/rules/rule-empty-line-before
May need an audit of those rules you thought were moved/deprecated to be double sure. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Sorry about that—The vibes were good but reality was bad, if you know what I mean ;)
Me, the human, has re-checked the config file and rules and made a few changes:
- Restored the config for all rules that I thought were deprecated but weren't. (Only
function-calc-no-invalidis removed without replacement.) - For rules that moved to
@stylistic, I added the@stylistic/name prefix where the existing rule was rather than grouping@stylisticrules together so that it's easier to see what rules were removed and which were just renamed - Restored the
extends: stylelint-config-standardstatement so that we continue to benefit from the default ruleset. This necessitated disabling some new-since-v14+ rules that were enabled by default and throwing errors. We can evaluate what those settings should be in another PR - Moved SCSS-specific overrides to the new
overridessection. Now, rules that need to be disabled for SCSS syntax support are only disabled for SCSS files.
I re-ran this updated config against our larger internal projects and verified that linting standards are enforced as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the adjustments are much easier to understand now as a straight upgrade!
That would be great! |
1d08521 to
8ec7b15
Compare
BREAKING CHANGE: Stylelint deprecated and removed many rules that they considered to be 'stylelistic'. Many of those rules are re-implemented in the @stylelistic/stylelint package. One rule was not moved there and so is now no longer linted: `function-calc-no-invalid`. See: stylelint/stylelint#5713
8ec7b15 to
2473318
Compare
kmuncie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good now as a straight upgrade.
This updates Stylelint from 13.x to 16.x.
BREAKING CHANGE: Stylelint deprecated and removed many rules that they considered to be 'stylelistic'. Many of those rules are re-implemented in the @stylelistic/stylelint package. A few rules were not moved there and are now removed from this config:
declaration-empty-line-before: neverrule-empty-line-before: neverlength-zero-no-unit: truevalue-keyword-case: lowerblock-no-empty: trueproperty-no-unknown: truecolor-hex-length: shortstring-no-newline: trueI tested the changes against some of our internal repos that have a large amount of SCSS. I found that the new
@stylelistic/stylelintpackage, in addition to not implementing the above rules, produce a few false-positives in theindent,function-comma-space-after, andfunction-parentheses-space-insiderules.Apparently there were already false-positives with these or related rules in v13, because there were
stylelint-disablecomments just before the offending line. For example:Whereas the
stylelint-disable-next-linecomment seemed to prevent the false positive in v13 for the entire multi-line declaration, v16 throws errors for the lines after.The point is, upgrading to use v16 and the new config in this file may cause a handful of new linting errors and require us to extend the
stylelint-disablecomments a bit. Other than that, and missing out on a few linting rules we had before, the update should be seamless.