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

jsx-no-leaked-render should only complain if there is a certian leaked render #3754

Open
1 task done
AhmedBaset opened this issue May 11, 2024 · 7 comments
Open
1 task done
Labels

Comments

@AhmedBaset
Copy link

AhmedBaset commented May 11, 2024

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique

Description Overview

This was previously discussed in #3719

The goal of the rule is to avoid leaked render specially when the left side of && is 0, NaN, or "" because react-dom will render a 0 in the first case (while some would expect to render nothing) and react-native will crach in the three. Currently, the rule complain for potential leaked render while most of the cases there is no any leaked render. for example, when the left side is boolean, undefined, null, ojbect, or array.

The following code is an eslint error, while it doesn't have any side effects.

let data: Item[] | undefined = getData();

return (
  <div>
     {data && <List items={data} />}
  </div>
)

Proposal

It should only complain if the left side is one of the potential leaked render number (0 or NaN) or string (""). Others types should be valid. AFAIK, this is possible with the @typescript-eslint/parser

eslint-plugin-react version

7.33.2

eslint version

8.56.0

node version

22

@ljharb
Copy link
Member

ljharb commented May 11, 2024

Sounds like a great enhancement. It should be behind an option (perhaps the option disables this behavior so that the default is friendlier?). It should only kick in when the AST has type info and it knows for sure it’s not a number or string or bigint.

@AhmedBaset
Copy link
Author

AhmedBaset commented May 11, 2024

It should only kick in when the AST has type info and it knows for sure it’s not a number or string or bigint.

Does the plugin use TS type parsing at the moment?

@ljharb
Copy link
Member

ljharb commented May 11, 2024

If the TS parser is used, and attaches type info to the AST, we can (and often do) use it.

@yusufkinatas
Copy link

I think this issue means making the rule recognize all 3 variables below are essentially boolean values.

Screenshot 2024-05-29 at 12 21 45

I guess similar to how VSCode tells the type of a variable when hovered?

I would love to help improving this rule, but pretty new to this repository. Do we have any examples of similar TS type parsing?

@ljharb
Copy link
Member

ljharb commented May 31, 2024

@yusufkinatas we don't typically have type-specific stuff, so off the top of my head i'm not sure where to look. however, you could always do the brute force approach and add some test cases and some console.logs, and see what type info is available on the AST nodes :-)

Rel1cx added a commit to Rel1cx/eslint-react that referenced this issue Jun 19, 2024
@Rel1cx
Copy link

Rel1cx commented Jun 20, 2024

The detection accuracy of eslint-plugin-react-x/src/rules/no-leaked-conditional-rendering.ts is qualitatively improved by introducing type information.

@ljharb
Copy link
Member

ljharb commented Jun 20, 2024

@Rel1cx it sounds like something worth upstreaming (i'm not sure why that project even exists tbh, if there's rules or enhancements in it that this project should have, please PR them in :-) )

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

No branches or pull requests

4 participants