Handle spread props in jsx-no-target-blank rule #679
Conversation
| @@ -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} | |||
There was a problem hiding this comment.
since the ordering of "rel" and the spread props seems to matter, should we also have a test for one where "rel" appears first?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good thought. Added in 3db97e4.
Since the bug being fixed here depends on the relative order of the spread props and the `rel` attribute, add a second test case with the opposite order to protect against regressions.
|
Merged, thanks! |
When running the
jsx-no-target-blankrule against code like the following:the following error would be raised:
The root cause is that the node representing the spread props does not have a
nameproperty.Note that if the desired
relattribute appears before the spread props, the error does not occur.