Skip to content
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

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

Open
Robloche opened this issue May 20, 2022 · 47 comments · May be fixed by #3441
Open

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

Robloche opened this issue May 20, 2022 · 47 comments · May be fixed by #3441

Comments

@Robloche
Copy link

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
Copy link
Member

ljharb commented May 20, 2022

cc @Belco90

@Belco90
Copy link
Contributor

Belco90 commented May 20, 2022

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

@Belco90
Copy link
Contributor

Belco90 commented May 20, 2022

@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
Copy link
Author

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
Copy link
Member

ljharb commented May 20, 2022

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

@Belco90
Copy link
Contributor

Belco90 commented May 20, 2022

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
Copy link
Member

ljharb commented May 20, 2022

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
Copy link
Contributor

Belco90 commented May 21, 2022

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
Copy link
Author

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

@Belco90
Copy link
Contributor

Belco90 commented May 23, 2022

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
Copy link
Member

ljharb commented May 24, 2022

@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
Copy link
Contributor

Belco90 commented May 24, 2022

@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
Copy link
Member

ljharb commented May 24, 2022

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

@texonidas
Copy link

texonidas commented May 26, 2022

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
Copy link
Member

ljharb commented May 26, 2022

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
Copy link
Contributor

Belco90 commented May 26, 2022

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

ljharb pushed a commit to Belco90/eslint-plugin-react that referenced this issue May 26, 2022
@fabiendem
Copy link

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
Copy link
Member

ljharb commented May 27, 2022

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

darekkay added a commit to darekkay/darekkay-eslint-config that referenced this issue Jun 21, 2022
@pke
Copy link

pke commented Jun 22, 2022

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
Copy link
Contributor

Belco90 commented Jun 22, 2022

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
Copy link

pke commented Jun 22, 2022

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
Copy link
Contributor

Belco90 commented Jun 22, 2022

@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
Copy link

pke commented Jun 23, 2022

You see the problem with null in that case though?

@ljharb
Copy link
Member

ljharb commented Jun 23, 2022

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
Copy link

pke commented Jun 23, 2022

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
Copy link
Member

ljharb commented Jun 23, 2022

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
Copy link

Has typescript awareness landed for this rule?

@Belco90
Copy link
Contributor

Belco90 commented Jun 29, 2022

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
Copy link

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
Copy link

levrik commented Sep 19, 2022

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
Copy link
Contributor

Belco90 commented Sep 19, 2022

@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
Copy link
Member

ljharb commented Sep 19, 2022

@levrik have you tried the latest version?

@levrik
Copy link

levrik commented Sep 20, 2022

@ljharb I tested with version 7.31.8

@aleclarson aleclarson linked a pull request Sep 26, 2022 that will close this issue
1 task
@aleclarson
Copy link

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
Copy link

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

@karlhorky
Copy link
Contributor

karlhorky commented Oct 2, 2022

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

Oh nice, looks like a nice plugin!

@hluisson 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
Copy link

tcl333 commented Nov 29, 2022

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
Copy link

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
Copy link

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
Copy link
Contributor

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
Copy link
Member

ljharb commented Nov 3, 2023

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

@karlhorky
Copy link
Contributor

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
Copy link
Member

ljharb commented Nov 4, 2023

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

@iway1
Copy link

iway1 commented Feb 26, 2024

makes no sense that this checked props in the first place

@siuming-qiu
Copy link

Has this problem been resolved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.