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 performs redundant props check and invalid fix in coerce mode #3431

Closed
wialy opened this issue Sep 11, 2022 · 7 comments · Fixed by #3511
Closed

jsx-no-leaked-render performs redundant props check and invalid fix in coerce mode #3431

wialy opened this issue Sep 11, 2022 · 7 comments · Fixed by #3511

Comments

@wialy
Copy link

wialy commented Sep 11, 2022

Context

There is a sample Checkbox component:

const Checkbox = ({
  isChecked = false,
  isIndeterminate = false,
}: {
  isChecked?: boolean;
  isIndeterminate?: boolean;
}) => (
  <input 
    checked={isIndeterminate ? false : isChecked} 
    type="checkbox"
  />
);

The eslint config file contains a rule:

"react/jsx-no-leaked-render": ["error", { validStrategies: ["coerce"] }],

Expected behavior

Linting the code raises no issues

Actual behavior

  1. Eslint displays an error for the line:
checked={isIndeterminate ? false : isChecked}
Potential leaked value that might cause unintentionally rendered values or rendering crasheseslint[react/jsx-no-leaked-render](https://github.com/jsx-eslint/eslint-plugin-react/tree/master/docs/rules/jsx-no-leaked-render.md)
  1. When auto-fix is performed, the code above is replaced with
checked={!!isIndeterminate && false} 

which causes the logical issues (the isChecked is removed in that case).

@ljharb
Copy link
Member

ljharb commented Sep 11, 2022

isChecked could be undefined, according to the types, and default arguments aren’t something that can really be used reliably with static analysis - so i think the warning may be correct.

I agree, though, that the autofix is incorrect - it should be fixing it to !isIndeterminate && isChecked

@ljharb
Copy link
Member

ljharb commented Sep 11, 2022

cc @Belco90

@himanshu007-creator
Copy link
Contributor

hey, would like to work on this issue

@ljharb
Copy link
Member

ljharb commented Nov 1, 2022

Go for it!

@himanshu007-creator
Copy link
Contributor

i was not able to reproduce the issue

@ljharb
Copy link
Member

ljharb commented Nov 1, 2022

In that case, can you make a PR with passing test cases, and it can close this issue?

@tcl333
Copy link

tcl333 commented Nov 29, 2022

I had an identical situation to the OP, and this error still occurred. @himanshu007-creator, did you remember to change the allowed strategies to only coerce?

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.

4 participants