feat(linter): implement react/style-prop-object
#6342
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
| return false; | ||
| }; | ||
|
|
||
| let AstKind::VariableDeclarator(var_decl) = node.kind() else { |
There was a problem hiding this comment.
Consider checking FormalParameters as well
There was a problem hiding this comment.
I don't have a great grasp of the AST yet, but can FormalParameters also be something other than function parameters?
There was a problem hiding this comment.
They're used in type parameters, but I don't think that's relevant here.
| return false; | ||
| }; | ||
|
|
||
| return is_invalid_expression(var_decl.init.as_ref(), ctx); |
There was a problem hiding this comment.
You may be able to check type annotations here for simple cases. See no-throw-literal for an example.
There was a problem hiding this comment.
Do you mean as a cheaper way instead of checking .init?
There was a problem hiding this comment.
function SomeComponent(props) {
let foo: string | undefined
if (bar) foo = 'foo'
return <div style={foo} />
}|
|
||
| fn style_prop_object_diagnostic(span: Span) -> OxcDiagnostic { | ||
| OxcDiagnostic::warn("Style prop value must be an object") | ||
| .with_help("Make sure the 'style' prop value is an object") |
There was a problem hiding this comment.
This help message is redundant and adds no extra information that the diagnostic message does not already provide. Consider removing it or providing more information (e.g. what type we think style is, if not an object)
| declare_oxc_lint!( | ||
| /// ### What it does | ||
| /// Require that the value of the prop style be an object or a variable that is an object. | ||
| /// |
There was a problem hiding this comment.
Missing "Why is this bad?" section.
| --- | ||
| ⚠ eslint-plugin-react(style-prop-object): Style prop value must be an object | ||
| ╭─[style_prop_object.tsx:1:6] | ||
| 1 │ <div style="color: 'red'" /> |
There was a problem hiding this comment.
Shrink the span to only cover the attribute's value
| .map(|v| { | ||
| v.iter() | ||
| .filter_map(serde_json::Value::as_str) | ||
| .map(ToString::to_string) |
There was a problem hiding this comment.
use CompactStr instead of Strings
There was a problem hiding this comment.
Also, if you sort this, you can use a binary search for O(log(n)) containment checks (instead of .contains, which is O(n).
|
Thanks for your interest in Oxc! This PR will be a great contribution. Just a few small fixes and we should be able to merge this. |
CodSpeed Performance ReportMerging #6342 will not alter performanceComparing Summary
|
Implements not recommended rule
react/style-prop-object(#1022)