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.

Sign up for GitHub

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

Handle spread props in jsx-no-target-blank rule #679

Merged
merged 2 commits into from
Jul 24, 2016
Merged

Handle spread props in jsx-no-target-blank rule #679

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eaab3fa
Handle spread props in jsx-no-target-blank rule
randycoulman Jul 13, 2016
3db97e4
Add another test case
randycoulman Jul 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rules/jsx-no-target-blank.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module.exports = function(context) {
var relFound = false;
var attrs = node.parent.attributes;
for (var idx in attrs) {
Copy link
Member

Choose a reason for hiding this comment

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

@yannickcr is there any reason this uses for..in instead of Object.keys(…).forEach(…)?

Copy link
Member

Choose a reason for hiding this comment

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

@ljharb If I remember correctly Object.keys is only supported since Node.js 4.0

Copy link
Member

Choose a reason for hiding this comment

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

@yannickcr Object.keys is ES5, and is supported since node 0.6.

Copy link
Member

Choose a reason for hiding this comment

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

@ljharb Ho ok, my bad. No reason to not using it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want this as part of this PR, or should it be a separate one? Feels unrelated to the bug IMO, but I'm willing to do it if you want.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with separate, but either way :-)

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 looked at this today. There is a break inside the for loop which doesn't work in the forEach version. I'm sure there's a way around that, but I think this should be in a separate PR. I'd like to have this one considered done and merged (if acceptable).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that'd require .some or similar. Separate PR is fine.

if (attrs[idx].name.name === 'rel') {
if (attrs[idx].name && attrs[idx].name.name === 'rel') {
var tags = attrs[idx].value.value.split(' ');
if (tags.indexOf('noopener') >= 0 && tags.indexOf('noreferrer') >= 0) {
relFound = true;
Expand Down
3 changes: 2 additions & 1 deletion tests/lib/rules/jsx-no-target-blank.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ ruleTester.run('jsx-no-target-blank', rule, {
valid: [
{code: '<a href="foobar"></a>', parserOptions: parserOptions},
{code: '<a randomTag></a>', parserOptions: parserOptions},
{code: '<a href="foobar" target="_blank" rel="noopener noreferrer"></a>', parserOptions: parserOptions}
{code: '<a href="foobar" target="_blank" rel="noopener noreferrer"></a>', parserOptions: parserOptions},
{code: '<a target="_blank" {...spreadProps} rel="noopener noreferrer"></a>', parserOptions: parserOptions}
Copy link
Member

Choose a reason for hiding this comment

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

since the ordering of "rel" and the spread props seems to matter, should we also have a test for one where "rel" appears first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only matters because we break out of the for loop as soon as we find the rel attribute we're looking for. I could add another test case, but it would pass on the old code, so doesn't illustrate the problem. I didn't think it worth it at the time, but if you think it is, I'm happy to add it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's important to add, to prevent a regression - if there's already order-dependent bugs, there could easily be more added later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thought. Added in 3db97e4.

],
invalid: [
{code: '<a target="_blank"></a>', parserOptions: parserOptions,
Expand Down