fix(rome_js_analyze): false positives for noUselessFragments #3668
#3858
Conversation
✅ Deploy Preview for docs-rometools canceled.
|
|
Would you mind giving this PR a more descriptive title. It's hard to guess what it is about from looking at the PR list or git log |
| @@ -108,31 +106,42 @@ impl Rule for NoUselessFragments { | |||
| let model = ctx.model(); | |||
| match node { | |||
| NoUselessFragmentsQuery::JsxFragment(fragment) => { | |||
| let matches_allowed_parents = node | |||
| let parents_where_fragments_must_be_preserved = node | |||
There was a problem hiding this comment.
Would it be safer to instead allow list the parents where the fragment must not be preserved? That way adding new AST node types won't break the rule.
There was a problem hiding this comment.
I think it wouldn't. I personally see it way harder to maintain
ematipico
changed the title
noUselessFragments #3668
| .syntax() | ||
| .parent() | ||
| .and_then(|parent| { | ||
| if let Some(parenthesized_expression) = |
There was a problem hiding this comment.
This would need to use an equivalent of omit_parentheses iterating through the ancestor nodes, or it's only going to catch the first parenthesized expression it encounters and fail on a case like this: { key: (((<>content</>))) }
There was a problem hiding this comment.
Yes, I am aware of it and I intentionally left it out. I consider it an edge case that we can tackle if we really know if could cause issue.
There's to consider that the formatter will remove all the parentheses
Summary
Fixes #3668
Test Plan
Added two new test cases.