Repository navigation
Conversation
github-actions
Bot
added
the
Scope: Frontend
📊 Type Coverage Diff✅ no issues found |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a14ed25. Configure here.
|
The rule misses styles returned from switch cases. Could we check those too, so it catches DOM selectors in non-core style helpers? |
Sentry Snapshot Testing
|
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Looking great! Just a few suggestions, I don't think they're blocking at all.
I ran my proto lint cleanup skill and it came out with #128361. I leave that for your perusal if you want. 🙂
| schema: [], | ||
| messages: { | ||
| forbidden: | ||
| 'Do not select components, DOM elements, universal targets, or unscoped children. Use an explicit class, ID, or attribute target instead.', |
There was a problem hiding this comment.
[Feature] You might want to add a data type so the message can specifically sway which forbidden thing is being targeted. It'll help folks who aren't super familiar with what the differences are.
Also, it's good to have a brief mention of "why" in the message, so the rule feels like relevant issue-reporting and not just griping.
Starting to wordsmith a bit (?s are my actual not-sure-which-es):
| 'Do not select components, DOM elements, universal targets, or unscoped children. Use an explicit class, ID, or attribute target instead.', | |
| '{{target}} is an overly (broad? deep?) target that relies on implementation details. Use an explicit class, ID, or attribute target instead.', |
| if (node.type === 'tag' || node.type === 'universal') { | ||
| forbidden = true; | ||
| } | ||
| if (node.type === 'combinator' && node.value.trim() === '>') { |
There was a problem hiding this comment.
[Bug] Only > checks whether the subject is scoped, so an unscoped pseudo-class subject after any other combinator gets through:
css`& > :first-child {color: red;}`; // reported
css`& :first-child {color: red;}`; // not reported
css`& :not(.a) {color: red;}`; // not reported
css`& + :first-child {color: red;}`; // not reportedThe descendant cases couple to even more of the DOM than the > one does. Should this check the subject after every combinator?
| return ['']; | ||
| } | ||
| return expression.body.type === 'BlockStatement' | ||
| ? returnedValues(expression.body).flatMap(resolve) |
There was a problem hiding this comment.
[Bug] A block-bodied function with no return doesn't give back any candidate. So nothing after it gets checked:
styled.div`${p => { f(p); }} button {color: red;}`; // not reported
styled.div`${p => { return ''; }} button {color: red;}`; // reportedFalling back to [''] when there are no returns would keep checking the static text.
lint rule
@sentry/scraps/no-dom-couplingto reject child selectors, component selectors, universal target selectors etc. instyled()andcss()so that we don’t write css that couples us to implementation details anymore!815 existing violations, so added to the incubator. Turned off for
scrapsbecause we do this internally and it’s actually fine because it’s scoped.Closes DE-1714