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
[New] jsx-no-useless-fragment
: add ignoreUnsafeChildren
flag
#2967
base: master
Are you sure you want to change the base?
[New] jsx-no-useless-fragment
: add ignoreUnsafeChildren
flag
#2967
Conversation
…tion With `ignoreNeedsMoreChildren` enabled, this rule will only warn about useless fragments that removing would guarantee no change in behavior. addresses some of: jsx-eslint#2584
jsx-no-useless-fragment
: support ignoreNeedsMoreChildren
op…jsx-no-useless-fragment
: add ignoreNeedsMoreChildren
flag
|
||
```js | ||
... | ||
"react/jsx-no-useless-fragments": [<enabled>, { "ignoreNeedsMoreChildren": <boolean> }] |
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 think the ignoreNeedsMoreChildren
name could use some work, open to ideas
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.
maybe something along the lines of ignorePossiblyUnsafeExprs
?
Codecov Report
@@ Coverage Diff @@
## master #2967 +/- ##
=======================================
Coverage 97.58% 97.59%
=======================================
Files 110 110
Lines 7256 7266 +10
Branches 2643 2647 +4
=======================================
+ Hits 7081 7091 +10
Misses 175 175
Continue to review full report at Codecov.
|
jsx-no-useless-fragment
: add ignoreNeedsMoreChildren
flagjsx-no-useless-fragment
: add ignoreUnsafeChildren
flag
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.
Per #2584 (comment), the intention was to add an option to only allow a prop value, and nothing else, to contain an otherwise useless fragment - because that's the only case where removing the fragment could in fact cause a runtime error.
@ljharb which fragments should we be warning about? // function could be expecting a react node and raise an error at runtime if it got null instead
<></>
// children could be undefined
<>{children}</>
// foo could be undefined
<>{foo && <Foo/>}</>
// foo could be undefined
<>{foo?.map(x => <Bar x={x}/>)}</> |
In that case, you should be doing: <></> // fair enough
// children could be undefined
{children ?? <></>}
// foo could be undefined
{foo ? <Foo/> : <></>}
// foo could be undefined
{foo?.map(x => <Bar x={x}/>) ?? <></>} In general, if a component is so badly written that it crashes when it receives a nullish value instead of an element, i don't think it makes much sense for the consumer to need to worry about it. I also think that's an appropriate place for an eslint override comment, where you explain that the override is necessary because the other component is incompetently authored. |
59af733
to
865ed16
Compare
069314a
to
181c68f
Compare
I think this would solve the only issue I have with the rule. JSX.Element, React.Element and React.ReactNode are not interchangeable types, and however you pull them you end up with type errors. If you have a While
|
@fregante can you elaborate (maybe with a ts playground link) on what you're hoping this PR would fix? |
I haven't looked at the code, but I assume The setting might make more sense as I think the entirety of #2584 (except #2584 (comment)) has this single complaint, so you have a lot more examples there. |
With
ignoreUnsafeChildren
enabled, this rule will only warn aboutuseless fragments that removing would guarantee no change in behavior.
addresses some of: #2584