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-target-blank: Add warnOnSpreadAttributes option #2855

Merged
merged 1 commit into from Dec 23, 2020
Merged

[New] jsx-no-target-blank: Add warnOnSpreadAttributes option #2855

merged 1 commit into from Dec 23, 2020

Conversation

michael-yx-wu
Copy link
Contributor

@michael-yx-wu michael-yx-wu commented Nov 11, 2020

Resolves #2827

I've changed the rule to warn on the JSXOpeningElement instead of a specific attribute. There is a technical reason and an architectural reason:

  • when disabling trustSpreadAttributes, we could end up with multiple warnings for the same problem e.g. once on a target="_blank" attribute and once on {...myUnsafeObject} spread attribute
  • it makes more sense (to me) that the rule warns on the element instead of a specific attribute since the problem is not necessarily due to any specific attribute, but the combination and order of the attributes on the element

Commit message

Add trustSpreadAttributes for jsx-no-target-blank

Defaults to true. When disabled, treats spread attributes as dangerous
unless explicitly overriden e.g. the following is safe:

<a {...dangerousObject} rel="noreferrer" target="_blank"></a>

This change also extends target="_blank" detection to include
conditional expressions whose alternate or consequent is the "_blank"
string (case-insensitive).

Defaults to `false`. When set to `true`, treats spread attributes as dangerous unless explicitly overriden.

e.g. the following is safe:

<a {...dangerousObject} rel="noreferrer" target="_blank"></a>

This change also extends target="_blank" detection to include conditional expressions whose alternate or consequent is the "_blank" string (case-insensitive).

Fixes #2827
@michael-yx-wu
Copy link
Contributor Author

cc @ljharb

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.

LGTM pending comment.

docs/rules/jsx-no-target-blank.md Outdated Show resolved Hide resolved
@michael-yx-wu michael-yx-wu changed the title Add trustSpreadAttributes for jsx-no-target-blank Add warnOnSpreadAttributes for jsx-no-target-blank Dec 22, 2020
@michael-yx-wu
Copy link
Contributor Author

I've changed the option name to warnOnSpreadAttributes (defaults to false). Thanks for pointing me to the right pattern for these things. This is ready for review again!

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.

LGTM overall

}
}
return false;
})(node.attributes[targetIndex]);
Copy link
Member

Choose a reason for hiding this comment

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

this IIFE is a bit weird here. can it be refactored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, yes maybe it's a bit too terse. I will break out the function, which might also make this a little faster since the rule won't have to recreate the function.

&& relAttribute.value.expression
&& relAttribute.value.expression.value
));
const tags = value && value.toLowerCase && value.toLowerCase().split(' ');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const tags = value && value.toLowerCase && value.toLowerCase().split(' ');
const tags = value && typeof value === 'string' && value.toLowerCase().split(' ');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had left this as is since it was carried over from the previous change, but I like this more explicit check as well.

@michael-yx-wu
Copy link
Contributor Author

This is ready for review again.

@ljharb ljharb changed the title Add warnOnSpreadAttributes for jsx-no-target-blank [New] jsx-no-target-blank: Add warnOnSpreadAttributes option Dec 23, 2020
@ljharb ljharb merged commit c4ecce9 into jsx-eslint:master Dec 23, 2020
@michael-yx-wu michael-yx-wu deleted the mw/jsx-no-target-blank-spread-attrs branch December 24, 2020 21:10
@astorije
Copy link

astorije commented Jan 5, 2021

Any chance this could be enabled by default or added to the recommended preset in the next major release? We're using the recommended preset, and that would save an override of the preset with this rule simply to enable this option 🙏

@ljharb
Copy link
Member

ljharb commented Jan 5, 2021

In the next major release, yes (along with many other changes), because it’d be a breaking change - but there’s no major releases planned any time soon.

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.

jsx-target-no-blank should warn when using JSX spread attributes
3 participants