False positive for react/jsx-no-leaked-render

[Robloche]

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]

cc @Belco90

[Belco90]

@ljharb thanks for pinging! I can take care of this, feel free to assign it to me.

[Belco90]

@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.

[Robloche]

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.

[ljharb]

The right side should be able to be any value that’s safe to render, which includes booleans.

[Belco90]

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?

[ljharb]

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.

[Belco90]

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.

[Robloche]

Yes, I'm aligned with your suggestion. That should do the trick.

[Belco90]

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

[ljharb]

@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]

@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.

Is it? If so, it's ok for that case. What about the cases described before?

[ljharb]

In the case of #3292 (comment), with no type information, obviously both of those should warn.

[texonidas]

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>
  );
};

[ljharb]

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)

[Belco90]

@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.

[fabiendem]

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.

[ljharb]

We don’t want to forbid && entirely tho - many uses of it are perfectly fine.

[pke]

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.

[Belco90]

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.

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.

[pke]

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.

[Belco90]

@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.

[pke]

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.

[ljharb]

"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.

[pke]

You see the problem with null in that case though?

[ljharb]

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".

[pke]

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?

[ljharb]

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.

[shamilovtim]

Has typescript awareness landed for this rule?

[Belco90]

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.

[Jackman3005]

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.

That's a bummer :/ I imagine it would be quite a bit easier to determine if the variable is a boolean in a Typescript aware context.

[levrik]

This rules is sadly completely broken (at least in TS ?). I'm using it with { "validStrategies": ["ternary"] } and it complains about this perfectly valid code:

checked={selectedCount > 0 && selectedCount === items.length}

and wants to change it to

checked={selectedCount > 0 ? selectedCount === items.length : null}

which doesn't make sense at all.

Another example:

// source
contentEditable={!disabled && editable}
// fixed
contentEditable={!disabled ? editable : null}

For me personally it would be enough if the rule would not apply to props. I acknowledge that there are valid cases where this rule should apply to props as well but IMO without looking at TS types this can result in a lot of false positives like my example above. The rule should target props that accept JSX only.

Actually I was quite surprised that the rule was also applied to props as I didn't expect this from looking at the examples in the documentation. My suggestion would be to either remove this rule from props until it got a bit smarter or at least make it configurable if it should be applied to props or not.

Please let me know which of these 2 options would be preferred as a short-term solution and I'm happy to contribute that change. But for me, in its current state, this rule is unusable.

[Belco90]

@levrik IMO we should include an option in the rule to avoid reporting props other than children (enabled by default to avoid changing the default behavior I guess).

[ljharb]

@levrik have you tried the latest version?

[levrik]

@ljharb I tested with version 7.31.8

[aleclarson]

I've fixed this in #3441 (new ignoreAttributes option)

Can one of you write the docs for that PR? Just type them in this issue, and I'll copy them into my PR. Thanks!

Of course, the best solution would involve TypeScript type-checking. Does this plugin allow that, or maybe someone can provide guidance here?

[aleclarson]

Oh I just found this plugin, which uses TypeScript:
https://www.npmjs.com/package/eslint-plugin-jsx-expressions

[karlhorky]

Oh I just found this plugin, which uses TypeScript:
npmjs.com/package/eslint-plugin-jsx-expressions

Oh nice, looks like a nice plugin!

@hpersson since you're the author of this, would you be interested in contributing TypeScript type checking also to react/jsx-no-leaked-render rule in eslint-plugin-react?

[tcl333]

This rule is unusable to me until PR #3441 is landed. I need to handle a very basic usecase of <Component isFooBar={isFoo && isBar}>. This rule currently auto-breaks that line by converting it into <Component isFooBar={isFoo ? isBar : null }>. My component doesn't take null as a prop, nor should it be required to.

[shamilovtim]

Yeah we can't use this rule either. It's not inferring bools. One of these days I'll be on a project where I can afford to contribute back here. Thank you to the maintainers and contributors.

[danielrotaermel]

Oh I just found this plugin, which uses TypeScript:
https://www.npmjs.com/package/eslint-plugin-jsx-expressions

Unfortunately the plugin does not work with @typescript-eslint/parser ^6 right now.
But there is a an open issue about it hluisson/eslint-plugin-jsx-expressions#13

[karlhorky]

We're on the latest version by overriding the package version installed, eg using npm Overrides:

package.json

{
  "overrides": {
    "@typescript-eslint/eslint-plugin": "6.9.1",
    "@typescript-eslint/parser": "6.9.1"
  }
}

(or Yarn Resolutions or pnpm Overrides)

[ljharb]

Using overrides will cause a number of rules to be broken; peer dep warnings need to be respected.

[karlhorky]

Yeah, good warning for many cases. I was talking about a workaround for this specific case, where it does not cause breakage in our experience.

[ljharb]

If that were true, we wouldn’t need #3629 ¯\_(ツ)_/¯

[iway1]

makes no sense that this checked props in the first place

[siuming-qiu]

Has this problem been resolved?