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] jsx-no-leaked-render: add ignoreAttributes option #3441

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aleclarson
Copy link

@aleclarson aleclarson commented Sep 26, 2022

Fixes #3292

When ignoreAttributes is true, validation of JSX attribute values is skipped.

Todo

  • write docs

When true, validation of JSX attribute values is skipped.
@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.65%. Comparing base (5c23573) to head (ab154a4).

Files with missing lines Patch % Lines
lib/rules/jsx-no-leaked-render.js 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3441      +/-   ##
==========================================
- Coverage   97.71%   97.65%   -0.06%     
==========================================
  Files         133      136       +3     
  Lines        9958     9981      +23     
  Branches     3693     3696       +3     
==========================================
+ Hits         9730     9747      +17     
- Misses        228      234       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@levrik
Copy link

levrik commented Sep 27, 2022

I think ignoreProps would be a more suitable name here.

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.

Can you elaborate on the use case here?

Certainly the component you're passing the prop value to might not be directly rendering it - but I'm not sure why that makes it OK to pass a renderable falsy value.

@ljharb ljharb marked this pull request as draft September 27, 2022 20:02
@aleclarson
Copy link
Author

Can you elaborate on the use case here?

Certainly the component you're passing the prop value to might not be directly rendering it - but I'm not sure why that makes it OK to pass a renderable falsy value.

The use case is preferring to avoid false positives. The component receiving the prop is responsible for avoiding leaky render. Many prefer to keep the warning local to where the prop is actually rendered.

@ljharb
Copy link
Member

ljharb commented Sep 29, 2022

I see - so you're saying, you still want to report on leaky renders on DOM elements, but you don't want to do that on props on custom elements so that the custom component has the responsibility to check for it?

In that case, what i'd expect is that this could be ignoreNonDOMProps, and it would only suppress warning on props of non-DOM elements - would that meet your use case?

@ljharb ljharb force-pushed the master branch 6 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45
@ljharb ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19
@tcl333
Copy link

tcl333 commented Nov 29, 2022

This rule is unusable to me until this PR 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.

@l1135677068
Copy link

I want to know how to use the ignoreAttributes option?
i use it like belows code, but it throw an error.

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

Is there a use case that i can refer to?

@ljharb
Copy link
Member

ljharb commented May 28, 2024

@l1135677068 this PR isn't merged yet, and thus can't be released yet either, so you can't use it yet.

@ljharb
Copy link
Member

ljharb commented May 28, 2024

I've just rebased this PR; it's ready to go once there's documentation. If someone wants to leave a code review suggestion, I can pull it in and land this.

@l1135677068
Copy link

If someone wants to leave a code review suggestion, I can pull it in and land this.

ok, i get it.

@ljharb ljharb force-pushed the master branch 2 times, most recently from 380e32c to 51d342b Compare July 4, 2024 15:25
@mikedidomizio
Copy link

mikedidomizio commented Oct 22, 2024

I think considering this PR has been open for 2 years without movement from the author (who is active), it's potentially safe to say they have higher priorities at this time.

I would suggest to get this past the finish line, by someone championing it and picking up where the original author left off. Fork his fork, make the necessary documentation changes, and create a separate pull request, and mention this one. @artemxknpv you wrote a documentation change suggestion, maybe you'd like to take this on?

It's so close, and it's crazy to me that 2 years have passed on this and all it required were some documentation updates.

I would also suggest that ignoreAttributes be replaced with ignoreProps or ignoreNonDOMProps like @ljharb has suggested. I think ignoreProps is much more concise and simple but that's my opinion.

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.

Seems pretty good.

docs/rules/jsx-no-leaked-render.md Show resolved Hide resolved
@ljharb ljharb marked this pull request as ready for review October 22, 2024 18:54
@ljharb ljharb force-pushed the fix/3292 branch 2 times, most recently from ab154a4 to ef74762 Compare October 22, 2024 19:52
@CruuzAzul
Copy link

I agree with the use of ignoreProps, which I think is more in line with what the props do. Does anyone have any idea how much more needs to be done on the PR before we can merge? 🤗 Maybe @ljharb

@CruuzAzul
Copy link

Hi @ljharb! 👋🏼 Is there any news on this subject? Do you have any idea when it might be available? Thanks 🙏🏼

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

Successfully merging this pull request may close these issues.

False positive for react/jsx-no-leaked-render
8 participants