-
Notifications
You must be signed in to change notification settings - Fork 653
feat(rome_js_analyzer): code action for noVoidElementsWithChildren
#3286
Conversation
✅ Deploy Preview for rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
noVoidElementsWithChildren
crates/rome_js_analyze/src/semantic_analyzers/nursery/no_void_elements_with_children.rs
Outdated
Show resolved
Hide resolved
/// If the current element has children props in style `<Component>'children'</Component> | ||
children_cause: bool, | ||
/// If the current element has the prop `dangerouslySetInnerHTML` | ||
dangerous_prop_case: bool, | ||
dangerous_prop_case: Option<UnwelcomedProp>, | ||
/// If the current element has the prop `children` | ||
children_prop: Option<UnwelcomedProp>, | ||
/// An instance of [ReactCreateElementCall] | ||
create_react_element: Option<ReactCreateElementCall>, |
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 it possible to have all combinations of children_cause
, dangerous_prop_case
, children_prop
, and
crate_react_element` or can we limit the possible combinations and represent them as a union?
I'm asking because limiting the possibilities can help readers to understand what combinations can exist and using an enum to name them makes them into explicit concepts (that can even be documented). It may also help to avoid some unreachable
branches in the code.
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 really depends on how we want the rule to work. My initial implementation was using an enum, but then I noticed that the old rule works by tracking all the invalid cases.
In this rule, we could have up to three errors at once:
React.createElement('img', {
children: 'child', // <= diagnostic triggered
dangerouslySetInnerHTML: 'html' // <= diagnostic triggered
}, 'child') // <= diagnostic triggered for 'child'
The upside of using an enum state would make the rule simple to understand for us, on the other hand this would generate up to three diagnostics that will show up to three code actions - basically we shift the burden to the user.
Tracking all the states at once allows us to emit one single code action and one single diagnostics.
Eventually ALL the suggested fixes are applied and the resultant code will be the same in both cases.
I don't mind both approaches, but I am more keen to use the single diagnostic approach.
crates/rome_js_analyze/src/semantic_analyzers/nursery/no_void_elements_with_children.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/nursery/no_void_elements_with_children.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/nursery/no_void_elements_with_children.rs
Outdated
Show resolved
Hide resolved
55473cf
to
670b855
Compare
crates/rome_js_analyze/src/semantic_analyzers/nursery/no_void_elements_with_children.rs
Show resolved
Hide resolved
crates/rome_js_analyze/src/semantic_analyzers/nursery/no_void_elements_with_children.rs
Outdated
Show resolved
Hide resolved
9fc9658
to
1f39c0f
Compare
Summary
Closes #3272
The PR implements the code action for the rule
noVoidElementsWithChildren
Test Plan
Updated the test cases and added new test case for the documentation