-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: require-baseline rule #33
base: main
Are you sure you want to change the base?
Conversation
Web Features contributor here- your assumption is correct.
Thanks for doing this! |
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.
This is really exciting to see!
I added a few comments that may help with feature detection. The keys that are used in the web-features
package are defined by BCD. While I think they can get you pretty close on feature detection, that isn't the intended use necessarily, and I suspect there will be some edge cases.
src/data/baseline-data.js
Outdated
["color", 10], | ||
["color-scheme", 10], | ||
["break-after", 0], | ||
["break-after.multicol_context", 0], |
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.
This is an example of a contextual modifier on the BCD key- the string "multicol_context" won't show up in CSS, but this modifier denotes that the break-after
would work inside a layout that using column
.
The convention is that these keys will have underscores, and this potentially could be useful down the road to further target linting. However, CSS properties are going to never have a period, so I think the regex could be adjusted to drop the .multicol_context
, or anything after a period. Essentially, anything beginning with css.properties
, you could do something like key.split('.')[2]
to get the property name.
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.
Ah, that's very helpful, thanks. Yeah, I left these because I was trying to figure out if I could do something with the values after the period.
src/data/baseline-data.js
Outdated
["custom-property", 10], | ||
["display", 10], | ||
["display.none", 10], | ||
["display.contents", 0], |
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.
display.contents
signifies a rule like this-
a {display: contents}
. If there is not an underscore in the second portion, it is the value. It would be nice to be able to check for this as well.
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 for explaining this. I looked into it a bit and this is more complicated than it seems. Sometimes the identifier after the property name is a literal (as in display.content
means display: contents
), but sometimes it's a type (content.url
means content: url(foo)
). So it's not enough to just pull that value and check for it, we'd also need to know what type of node to look for in CSSTree.
And it gets even more complicated with types that actually represent multiple values, like border-radius.percentages
, which may be represented by 1-4 nodes. I suppose we could filter and just check on the properties that are known to only have literal values, but I don't know how to automate that.
Ultimately, it looks like sussing out all of these details would be a labor-intensive process, so it's unlikely to happen without outside contributions.
src/data/baseline-data.js
Outdated
["namespace", 10], | ||
["page", 0], | ||
["page.size", 0], | ||
["media.prefers-color-scheme", 10], |
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.
This would indicate a rule like
@media (prefers-color-scheme: dark){}
.
src/data/baseline-data.js
Outdated
["starting-style", 0], | ||
["supports", 10], | ||
]); | ||
export const types = new Map([ |
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 noticed you're not testing types yet, so this is more of a note for the future.
Types are going to be tricky to detect from the key. Some will be fairly straightforward- abs
, sign
, anchor
, for instance, will all show up as functions with that name, in a value in an AST.
A few items like gradient.*
and easing-function.*
would show similarly show up as functions if you took the substring after the .
.
Others like time
or string
would not show up literally, but would need to be detected in a different way.
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.
Yeah, I figured for the first version of the rule I'd punt on this, but I wanted the data here for easy reference later.
The only way I can think to make this work is to maintain a separate dictionary that maps types to their expected AST locations.
src/data/baseline-data.js
Outdated
["percentage", 10], | ||
["integer", 10], | ||
]); | ||
export const selectors = new Map([ |
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.
For selectors
, feature detection could be fairly straightforward. Some like :has
would have a single :
and show up in the AST as PseudoClassSelector
, others like ::before
would have 2 ::
and show up in the AST as a PseudoElementSelector
.
I don't think there's a way to differentiate between the two classes based on this data.
There are also some items here that are not actually verbatim selectors- for instance, "class" refers to the .
notation for selecting classes.
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.
Yeah, that's the challenge I ran into, so similar to types, I figured I'd just make the data was there and I'd come back to it later. Probably will need to be addressed similar to types where there needs to be a separate dictionary indicating which are pseudoclasses vs. pseudoelements vs. other.
Okay, I think I've got this in pretty good shape now.
I'd love for folks to try this out and give me some feedback on if it's helpful. FYI: I'm now offline until December 30, but please feel free to leave your feedback and I'll review when I return. Happy holidays! |
Okay, I think this rule is ready to go! Some updates:
Open question for me: Is |
fixes #26
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.
🔥 Core functionality all looks great! The logic encapsulation in the classes made it pretty straightforward to review functionally.
Only one blocking comment: the [Bug] around _
in names. If that is fixed then ✅ from me.
Left some other suggestions too, but nothing I think is critical.
docs/rules/baseline.md
Outdated
@@ -0,0 +1,71 @@ | |||
# baseline | |||
|
|||
Enforce the use of baseline features |
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.
[Docs] This reads to me as the opposite of what the rule does. I see the rule as being in the family of "Disallow ..." because it reports to stop devs from using certain syntax.
Suggesting making the description more in line with that:
Enforce the use of baseline features | |
Disallow use of CSS not in the Baseline available feature set |
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.
We only use "disallow" when the rule begins with "no". For others, we use "Enforce" (or "Suggest").
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.
Is this documented anywhere? I can never keep track of the flowchart for how to name a rule.
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.
This is probably the closest we have:
https://eslint.org/docs/latest/contribute/core-rules#rule-naming-conventions
If you want to update that doc, feel free.
// export each group separately as a Set, such as highProperties, lowProperties, etc. | ||
const code = `/** | ||
* @fileoverview CSS features extracted from the web-features package. | ||
* @author tools/generate-baseline.js |
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.
[Non-Actionable] TIL the syntax highlighting for author
is sensitive to a slash, interesting:
Additional TIL that there's no TypeScript->editor-native way to link to another file. microsoft/TypeScript#47718
🤷 I don't know of a better way to say this is authored by another file. So, not requesting changes, just ... vague interest.
docs/rules/baseline.md
Outdated
@@ -0,0 +1,71 @@ | |||
# baseline |
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.
Open question for me: Is
baseline
the right name? Should it maybe beuse-baseline
orrequire-baseline
instead?
[Naming] Maybe, baseline-support
or baseline-supported
, to indicate it's about specifically the support aspect of baseline
? But that disambiguation is probably only useful if there are other baseline-related rules to disambiguate from. And there are none. So no strong preference here.
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.
(wrong button)
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Co-authored-by: Josh Goldberg ✨ <[email protected]>
The results of my asking online revealed |
@JoshuaKGoldberg Can you please check if your comments have been addressed? |
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.
🚀 Looks great to me! Great showcase of why linting CSS should be a top-of-mind concern for a web dev linter.
@eslint/eslint-tsc last call for reviews on this. I'll assume silence equals approval at this point. 😄 |
Prerequisites checklist
What is the purpose of this pull request?
Create a rule that warns when non-baseline features are used.
What changes did you make? (Give an overview)
baseline
ruleRelated Issues
fixes #26
Is there anything you'd like reviewers to focus on?
I'm not 100% sure that the baseline data matches what's on the website. Thehigh = widely, low = newlyweb-features
package doesn't tag things as "widely" or "newly", just "high", "low", orfalse
. I started with the assumption that "high" means "widely" and "low" means "newly", but I'm not sure that's correct.