Don't issue errors for <a/> with target=_blank and non-external URLs
#1216
Conversation
lib/rules/jsx-no-target-blank.js
Outdated
| @@ -33,8 +33,21 @@ module.exports = { | |||
| var relFound = false; | |||
| var attrs = node.parent.attributes; | |||
| for (var idx in attrs) { | |||
| if (attrs[idx].name && attrs[idx].name.name === 'rel') { | |||
| var tags = attrs[idx].value.type === 'Literal' && attrs[idx].value.value.toLowerCase().split(' '); | |||
| if (!Object.prototype.hasOwnProperty.call(attrs, idx)) { | |||
There was a problem hiding this comment.
the guard is for guard-for-in.
There was a problem hiding this comment.
Since we have node 4+ now, let's just use Object.keys().forEach - then no guard is needed.
There was a problem hiding this comment.
in forEach(), only there's onlyreturn available to control loops, but this loop needs return, continue and breaks, so forEach() is not suitable.
There was a problem hiding this comment.
In that case I'd prefer to rewrite it to use .filter in lieu of the continues, and .find in lieu of the breaks and returns.
However, to keep this PR small, we can stick with this guard (which should have been here in the first place).
There was a problem hiding this comment.
IMO your review seems right and the conditions are too complex; I also prefer to re-write them with ES2015 features.
Refactored in 89c33b6
lib/rules/jsx-no-target-blank.js
Outdated
| } | ||
| if (attr.name.name === 'href') { | ||
| if (attr.value.type === 'Literal' && !/^\w+:/.test(attr.value.value)) { | ||
| // it's safe because it is not an external link (i.e. doesn't start with a protocol) |
There was a problem hiding this comment.
//google.com/foo is an external link - please add a test case ensuring that protocol-less URLs still treated as external.
<a href="relative/path" target="_blank"/>and<a href="/absolute/path" target="_blank"/>are considered safe because the link is placed in the same host, so the rule only issues for URLs with protocols.