eslint-plugin-react
eslint-plugin-react copied to clipboard
jsx-eslint
False positive for react/jsx-no-leaked-render
Following code triggers react/jsx-no-leaked-render although it's perfectly valid:
return <MyReactComponent
isDisabled={foo && bar === 0}
onClick={handleOnClick} />;
with:
foo: boolean;
bar: number;
@ljharb thanks for pinging! I can take care of this, feel free to assign it to me.
@Robloche regarding the false positive reported: I think sometimes you would be interested into having this rule applied to props also, like:
<MyReactComponent
header={someNumber && <Header />}
isDisabled={foo && bar === 0}
/>
So I think the proper fix for this would be to make sure the right side of the && operator is not a condition.
Agreed. So what's you recommendation?
As for now, I simply rewrote my code, adding a local isDisabled boolean.
And since this makes the code clearer, I guess it's a good solution.
The right side should be able to be any value that’s safe to render, which includes booleans.
The right side should be able to be any value that’s safe to render, which includes booleans.
That's true, but now I don't know what to do with this false positive since I can't figure the types of the operators. Any suggestions?
The RHS is an === comparison; it can’t ever be anything but a boolean. You don’t need to know any types to figure that part out.
The RHS is an === comparison; it can’t ever be anything but a boolean. You don’t need to know any types to figure that part out.
Exactly, so since the RHS is something renderable, this rule should complain about it since we don't know the type for the LHS (it could be a zero, so the rule reported it as expected).
I was suggesting to report props only when there is a && operator and the RHS is a component so we can assume you want to pass that prop to render something, rather than just use the value for logic purposes.
This could be even fixed by introducing a new option: reportProps. It could receive the following values:
"always": as it is now"components": when the RHS is a component, as suggested"never": so no props are reported
@Belco90 in this example tho, foo is a boolean since it's using the TS parser - so the rule should be able to know that the entire prop is only a boolean.
@Belco90 in this example tho,
foois a boolean since it's using the TS parser - so the rule should be able to know that the entire prop is only a boolean.
Is it? If so, it's ok for that case. What about the cases described before?
In the case of https://github.com/jsx-eslint/eslint-plugin-react/issues/3292#issuecomment-1132947856, with no type information, obviously both of those should warn.
I'm also getting a very incorrect error for this:
const DISPLAY = {
WELCOME: 'WELCOME',
FOO: 'FOO'
};
const Component = () => {
const [display, setDisplay] = useState(DISPLAY.WELCOME);
const test = !display || display === DISPLAY.WELCOME;
console.log(typeof(test)); // logs 'boolean'
return (
<div>
{(!display || display === DISPLAY.WELCOME) && (
<span>my condition produces a linting error even though it is correctly interpreted as a boolean</span>
)}
</div>
);
};
In that case, since !display and display === something are booleans, there shouldn't be any TS required to statically know the condition is safe (the runtime typeof is irrelevant)
@ljharb I'll need way more time to try checking the TS types when available for the operators of the logical expression as discussed in this issue, since I've never done this before.
Not sure it can help but I have been using this relatively simple rule so far https://github.com/grailed-code/eslint-plugin-grailed/blob/main/lib/rules/jsx-no-ampersands.js Tests: https://github.com/grailed-code/eslint-plugin-grailed/blob/main/tests/lib/rules/jsx-no-ampersands.js It doesn't raise the false positives, even without any knowledge of the types.
We don’t want to forbid && entirely tho - many uses of it are perfectly fine.
A new option would be good. Props should not be validated by default.
Also the fix should not result to null in ternary operator but to undefined or it should be configurable. null according to Crockford should not be used: if you want a falsy value use undefined, because null does not work with default params and therefore completely changes the code flow.
Also the fix should not result to
nullin ternary operator but toundefinedor it should be configurable.nullaccording to Crockford should not be used: if you want a falsy value useundefined, becausenulldoes not work with default params and therefore completely changes the code flow.
Returning null is required since prior to React v18 returning undefined is considered as a missed return statement. React itself will complain with a message like: Uncaught Error: Error(...): Nothing was returned from render. This usually means a return statement is missing. Or, to render nothing, return null.
re: null thats correct for render returns but not for props. I was unclear. I meant for props fixes it should use undefined and not null.
@pke I see, I guess we can discuss on how to improve the rule reporting on props other than children. Not really sure what to do with it to be honest, apart from the option to decide between null or undefined.
Given those 2 components:
function Button({ type = "primary"}) {
...
}
function Parent({ count: number }) {
return <Button type={count && "secondary"}/>
}
the linting rule would change this:
-return <Button type={count && "secondary"}/>
+return <Button type={count ? "secondary": null }/>
And when it sends null for type to the <Button> the Buttons default assignment for the type prop does not work anymore.
"null should not be used" is useless and antiquated advice that the larger JS ecosystem has resoundingly rejected, so let's just throw that out the window right now.
Yes, but a present undefined prop isn't the same thing as an absent prop either.
In this specific case, if the prop value is foo && bar === 0 then the rule simply shouldn't be checking it - so yes, I do agree that this rule should not be checking prop values that aren't "children on an HTML element".
Yes, but a present undefined prop isn't the same thing as an absent prop either.
Like for the JSX compiler? Would you mind to explain?
Like for the props object the component receives. <Foo /> and <Foo x={undefined} /> are different - the former gets a props object of {}, the latter { x: undefined }, and these are observably different.
Has typescript awareness landed for this rule?
@shamilovtim I'm afraid it didn't, and I'm not really available to handle this for now.