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

jsx-target-no-blank should warn when using JSX spread attributes #2827

Closed
michael-yx-wu opened this issue Oct 13, 2020 · 8 comments · Fixed by #2855
Closed

jsx-target-no-blank should warn when using JSX spread attributes #2827

michael-yx-wu opened this issue Oct 13, 2020 · 8 comments · Fixed by #2855

Comments

@michael-yx-wu
Copy link
Contributor

At the moment, something like this would not trigger a warning:

<a {...otherProps} target="_blank" />

Since the rule can't verify whether this will compile to have an href that points to an external url, wouldn't it be safer to warn the user? If they are sure that this will never compile to be an external link, they can explicitly disable the rule for this line.

@ljharb
Copy link
Member

ljharb commented Oct 13, 2020

What happens when you enable the enforceDynamicLinks option?

@michael-yx-wu
Copy link
Contributor Author

It still doesn't warn, unfortunately. Maybe because the spread isn't considered an attribute and even if it were, hasDynamicLink's check for attribute === "href" (or some other user-specified attribute type) would fail.

@ljharb
Copy link
Member

ljharb commented Oct 13, 2020

I think it's reasonable to either enhance enforceDynamicLinks to warn on a missing "href" when something's spread in, or, to add a new option for that behavior in case that's a breaking change.

@michael-yx-wu
Copy link
Contributor Author

in a similar vein, what about when target is dynamic? For example:

<a href="https://some-external-url" target={someBoolValue ? "_blank" : undefined} >Maybe opens in a new tab</a>

might be unsafe depending on the value of someBoolValue.

@ljharb
Copy link
Member

ljharb commented Oct 31, 2020

In that example, since one of the branches of the ternary is static and problematic, the rule should be able to warn as well.

@michael-yx-wu
Copy link
Contributor Author

it's my first time contributing. @ljharb, are you the right person to review the PR?

@ljharb
Copy link
Member

ljharb commented Nov 22, 2020

Yes, sorry for the delay - I’ll try to review it soon.

@michael-yx-wu
Copy link
Contributor Author

@ljharb friendly ping. Let me know if there's anything I can add in the PR description to make it easier to review :)

@ljharb ljharb closed this as completed in c4ecce9 Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants