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

[New] add jsx-no-leaked-render #3203

Merged
merged 2 commits into from Apr 26, 2022

Conversation

Belco90
Copy link
Contributor

@Belco90 Belco90 commented Feb 12, 2022

New rule jsx-no-leaked-zero to prevent potential 0 leaked to the DOM from numeric inline conditions. It includes 2 autofix strategies.

@Belco90 Belco90 changed the title New rule: jsx-no-leaked-zero feat: new rule jsx-no-leaked-zero Feb 12, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2022

Codecov Report

Merging #3203 (160134a) into master (5bc571d) will increase coverage by 0.00%.
The diff coverage is 98.41%.

@@           Coverage Diff           @@
##           master    #3203   +/-   ##
=======================================
  Coverage   97.67%   97.68%           
=======================================
  Files         121      122    +1     
  Lines        8575     8627   +52     
  Branches     3112     3124   +12     
=======================================
+ Hits         8376     8427   +51     
- Misses        199      200    +1     
Impacted Files Coverage Δ
index.js 100.00% <ø> (ø)
lib/rules/jsx-no-leaked-zero.js 98.03% <98.03%> (ø)
lib/rules/jsx-wrap-multilines.js 98.13% <100.00%> (-0.10%) ⬇️
lib/util/ast.js 96.59% <100.00%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bc571d...160134a. Read the comment docs.

@Belco90
Copy link
Contributor Author

Belco90 commented Feb 13, 2022

I'm not sure how to fix the 2 remaining checks failing.

@Belco90
Copy link
Contributor Author

Belco90 commented Feb 26, 2022

@yannickcr pinging you for getting some review.

@ljharb
Copy link
Member

ljharb commented Feb 26, 2022

@Belco90 there's no need for pings; if you haven't gotten a review yet, it's because nobody's had time, not because the notifications we already get from the PR itself were missed.

@Belco90
Copy link
Contributor Author

Belco90 commented Feb 26, 2022

@ljharb my bad, sorry for any inconvenience caused.

docs/rules/jsx-no-leaked-zero.md Outdated Show resolved Hide resolved
lib/rules/jsx-no-leaked-zero.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-leaked-zero.js Outdated Show resolved Hide resolved
lib/util/ast.js Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-leaked-zero.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Feb 26, 2022

This may also solve some/most of #2073.

@Belco90 Belco90 force-pushed the new_rule-jsx-no-leaked-zero branch from 3bbdea6 to 04472dd Compare March 9, 2022 00:09
@Belco90 Belco90 requested a review from ljharb March 9, 2022 00:10
@Belco90 Belco90 force-pushed the new_rule-jsx-no-leaked-zero branch 2 times, most recently from 02efc2d to 6c6a49a Compare March 27, 2022 13:25
@Belco90
Copy link
Contributor Author

Belco90 commented Mar 27, 2022

Everything finally sorted out!

@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #3203 (eda90f0) into master (4b4bba9) will increase coverage by 0.00%.
The diff coverage is 98.41%.

❗ Current head eda90f0 differs from pull request most recent head ef733fd. Consider uploading reports for the commit ef733fd to get more accurate results

@@           Coverage Diff           @@
##           master    #3203   +/-   ##
=======================================
  Coverage   97.70%   97.70%           
=======================================
  Files         122      123    +1     
  Lines        8667     8719   +52     
  Branches     3148     3160   +12     
=======================================
+ Hits         8468     8519   +51     
- Misses        199      200    +1     
Impacted Files Coverage Δ
index.js 100.00% <ø> (ø)
lib/rules/jsx-no-leaked-render.js 98.03% <98.03%> (ø)
lib/rules/jsx-wrap-multilines.js 98.13% <100.00%> (-0.10%) ⬇️
lib/util/ast.js 95.83% <100.00%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b4bba9...ef733fd. Read the comment docs.

docs/rules/jsx-no-leaked-zero.md Outdated Show resolved Hide resolved
@Belco90 Belco90 changed the title feat: new rule jsx-no-leaked-zero feat: new rule jsx-no-leaked-render Apr 25, 2022
@Belco90
Copy link
Contributor Author

Belco90 commented Apr 25, 2022

I've renamed the rule to jsx-no-leaked-render since:

  1. NaN is also leaked in React
  2. Not casting to bool causes crashes in React Native

@Belco90 Belco90 force-pushed the new_rule-jsx-no-leaked-zero branch from ad3589f to 95883de Compare April 25, 2022 10:51
@Belco90 Belco90 requested a review from ljharb April 25, 2022 10:53
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rebased this; it's almost there

docs/rules/jsx-no-leaked-render.md Outdated Show resolved Hide resolved
lib/rules/jsx-no-leaked-render.js Outdated Show resolved Hide resolved
@Belco90
Copy link
Contributor Author

Belco90 commented Apr 26, 2022

I'll fix the failing tests and apply the suggested changes later. Thanks!

@ljharb ljharb force-pushed the new_rule-jsx-no-leaked-zero branch from 95883de to 6b7b191 Compare April 26, 2022 16:56
@Belco90 Belco90 requested a review from ljharb April 26, 2022 18:14
@ljharb
Copy link
Member

ljharb commented Apr 26, 2022

@Belco90 re 866f532, we should use fragments - you just need to add features: ['fragments'] to the test object.

@Belco90
Copy link
Contributor Author

Belco90 commented Apr 26, 2022

Not sure why the "Automatic Rebase" check keeps failing

@ljharb
Copy link
Member

ljharb commented Apr 26, 2022

because instead of deleting your local branch and checking out my rebase, you merged your original local branch into my rebased one, and that means there's merge conflicts when trying to rebase it. Don't worry about that tho, I can handle it when it's time to land this.

@ljharb ljharb force-pushed the new_rule-jsx-no-leaked-zero branch 2 times, most recently from e565c15 to ef733fd Compare April 26, 2022 21:51
@ljharb ljharb changed the title feat: new rule jsx-no-leaked-render [New] add jsx-no-leaked-render Apr 26, 2022
@ljharb ljharb merged commit ef733fd into jsx-eslint:master Apr 26, 2022
@prichodko
Copy link

@Belco90 and @ljharb – thanks for making this happen ❤️

@Belco90
Copy link
Contributor Author

Belco90 commented May 6, 2022

@Belco90 and @ljharb – thanks for making this happen ❤️

Looking forward to getting it released!

@karlhorky
Copy link
Contributor

karlhorky commented Oct 3, 2022

@Belco90 @ljharb is this rule description still valid for React Native?

In React Native, your render method will crash if you render 0, '', or NaN:

const Example = () => {
  return (
    <>
      {0 && <Something/>}
      {/* React: renders undesired 0 */}
      {/* React Native: crashes 💥 */}

      {'' && <Something/>}
      {/* React: renders nothing */}
      {/* React Native: crashes 💥 */}

      {NaN && <Something/>}
      {/* React: renders undesired NaN */}
      {/* React Native: crashes 💥 */}
    </>
  )
}

I can't get an empty string to crash React Native without a wrapping <Text> component 😮 (at least in an Expo Snack) https://twitter.com/karlhorky/status/1576958348288987137

I would do the PR to update the docs if this is no longer a thing...

@ljharb
Copy link
Member

ljharb commented Oct 3, 2022

That would be a major DX change for React Native, so I'd want to see some kind of announcement that it was intentional by the RN team.

@karlhorky
Copy link
Contributor

I also was surprised - but couldn't find anything official after a bit of digging. Asking some maintainers here: https://twitter.com/karlhorky/status/1576987525914464256

@karlhorky
Copy link
Contributor

Ok, so I didn't really get a clear answer from any maintainers there, but I just started a new React Native app just now (npx react-native init with react-native@0.70.1) and confirmed empty strings outside of <Text> components do not cause this error:

Screen Shot 2022-10-04 at 13 10 49

Whereas a string with content in it does:

Screen Shot 2022-10-04 at 13 14 34

Repo: https://github.com/karlhorky/react-native-empty-string-outside-text-component

@ljharb enough proof?

@karlhorky
Copy link
Contributor

Asked some more maintainers about whether this is new behavior: https://twitter.com/karlhorky/status/1577257935796740096

@ljharb
Copy link
Member

ljharb commented Oct 4, 2022

In which RN version did this change?

To be clear, I’m not looking for proof it changed, I’m looking for proof it was intentional :-)

@karlhorky
Copy link
Contributor

Good point about it being intentional, @rickhanlonii is going to look into it: https://twitter.com/rickhanlonii/status/1577291418485362688

@karlhorky
Copy link
Contributor

Since I haven't received any feedback yet, I opened an issue on the React Native GitHub repo: facebook/react-native#35002

@karlhorky
Copy link
Contributor

Looks like indeed an intentional change - actually in React itself (in version 18): facebook/react-native#35002 (comment)

I would update the docs if you agree @ljharb @Belco90

@ljharb
Copy link
Member

ljharb commented Oct 18, 2022

@karlhorky sounds like we need a PR to update the docs, as well as the rule when react is >= 18

@karlhorky
Copy link
Contributor

karlhorky commented Oct 19, 2022

Docs PR: #3468

@Belco90 would you maybe want to do the 2nd PR that @ljharb mentioned above? I think the idea is to modify the rule to not report errors on empty strings if React is detected to be above or equal to v18.

@Belco90
Copy link
Contributor Author

Belco90 commented Oct 19, 2022

@Belco90 would you maybe want to do the 2nd PR that @ljharb mentioned above? I think the idea is to modify the rule to not report errors on empty strings if React is detected to be above or equal to v18.

I'm afraid I don't have enough time right now to take care of it 😢

@karlhorky
Copy link
Contributor

Ok, opened #3479 for the rule change

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

Successfully merging this pull request may close these issues.

None yet

5 participants