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 7d9713e to 3bbdea6 Compare Feb 28, 2022
@Belco90 Belco90 force-pushed the new_rule-jsx-no-leaked-zero branch from 3bbdea6 to 04472dd Compare Mar 9, 2022
@Belco90 Belco90 requested a review from ljharb Mar 9, 2022
@Belco90 Belco90 force-pushed the new_rule-jsx-no-leaked-zero branch 2 times, most recently from 02efc2d to 6c6a49a Compare Mar 27, 2022
@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 Apr 25, 2022
@Belco90 Belco90 requested a review from ljharb Apr 25, 2022
Copy link
Member

@ljharb ljharb left a comment

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 Apr 26, 2022
@Belco90 Belco90 requested a review from ljharb Apr 26, 2022
@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.

@Belco90
Copy link
Contributor Author

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

Ah, damn! Sorry about that 🤦‍♂️

docs/rules/jsx-no-leaked-render.md Outdated Show resolved Hide resolved
docs/rules/jsx-no-leaked-render.md Outdated Show resolved Hide resolved
lib/rules/jsx-no-leaked-render.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-leaked-render.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-leaked-render.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-leaked-render.js Outdated Show resolved Hide resolved
@Belco90 Belco90 requested a review from ljharb Apr 26, 2022
@ljharb ljharb force-pushed the new_rule-jsx-no-leaked-zero branch 2 times, most recently from e565c15 to ef733fd Compare Apr 26, 2022
ljharb
ljharb approved these changes Apr 26, 2022
@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
243 checks passed
@prichodko
Copy link

prichodko commented May 6, 2022

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

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

4 participants