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

[Fix] jsx-no-target-blank: allow ternaries with literals #3464

Merged

Conversation

akulsr0
Copy link
Contributor

@akulsr0 akulsr0 commented Oct 15, 2022

Fixes #3462

I have added the fix for support of conditionals in jsx-no-target-blank rule.
But not sure about one test case and it's failing. Would need some help on that.

<a href={href} target={isExternal ? "_blank" : undefined} rel={isExternal ? "noopener noreferrer" : undefined} />

As per the test case this is expected to be invalid. I am not sure if this should be invalid or not?

ljharb
ljharb approved these changes Oct 15, 2022
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, thanks

lib/rules/jsx-no-target-blank.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-target-blank.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the fix/jsx-no-target-blank-support-conditionals branch from b91124c to 8e55e8d Compare Oct 17, 2022
@ljharb
Copy link
Member

ljharb commented Oct 17, 2022

@akulsr0 i've freshly rebased this, but there's failing tests.

@akulsr0
Copy link
Contributor Author

akulsr0 commented Oct 18, 2022

I have moved the following testcase (which was failing) to valid, since it is a valid scenario.

<a href={href} target={isExternal ? "_blank" : undefined} rel={isExternal ? "noopener noreferrer" : undefined} />

Reference: https://stackoverflow.com/questions/50709625/link-with-target-blank-and-rel-noopener-noreferrer-still-vulnerable

@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Merging #3464 (590bde4) into master (611b9ee) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Current head 590bde4 differs from pull request most recent head 5efd774. Consider uploading reports for the commit 5efd774 to get more accurate results

@@           Coverage Diff           @@
##           master    #3464   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files         128      128           
  Lines        9110     9118    +8     
  Branches     3320     3321    +1     
=======================================
+ Hits         8886     8894    +8     
  Misses        224      224           
Impacted Files Coverage Δ
lib/rules/jsx-no-target-blank.js 93.12% <100.00%> (+0.44%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ljharb
ljharb approved these changes Oct 18, 2022
@ljharb ljharb force-pushed the fix/jsx-no-target-blank-support-conditionals branch from 590bde4 to 5efd774 Compare Oct 18, 2022
@ljharb ljharb changed the title fix: jsx-no-target-blank support for conditionals [Fix] jsx-no-target-blank: allow ternaries with literals Oct 18, 2022
@ljharb ljharb merged commit 5efd774 into jsx-eslint:master Oct 18, 2022
264 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

react/jsx-no-target-blank not working as expected on conditionals
2 participants