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-no-script-url]: use shared settings to specify link components #3673

Merged
merged 2 commits into from Jan 11, 2024

Conversation

burtek
Copy link
Contributor

@burtek burtek commented Jan 8, 2024

Fixes #2865

@burtek
Copy link
Contributor Author

burtek commented Jan 8, 2024

I'm probably gonna need to change getLinkComponents util to allow multiple props for single component. You can't really give it MyComponent with two different props at this time, while jsx-no-script-url allows that. Am thinking about changing a string prop linkAttribute to a string array prop linkAttributes (and internally mapping linkAttribute to linkAttributes: [linkAttribute] for backwards compatibility).

docs/rules/jsx-no-script-url.md Outdated Show resolved Hide resolved
docs/rules/jsx-no-script-url.md Outdated Show resolved Hide resolved
lib/rules/jsx-no-script-url.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-script-url.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-script-url.js Outdated Show resolved Hide resolved
@burtek burtek marked this pull request as ready for review January 8, 2024 22:35
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/rules/jsx-no-script-url.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-target-blank.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-target-blank.js Outdated Show resolved Hide resolved
lib/util/linkComponents.js Outdated Show resolved Hide resolved
lib/util/linkComponents.js Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3730edb) 97.65% compared to head (1d15932) 97.74%.

❗ Current head 1d15932 differs from pull request most recent head 1014f8c. Consider uploading reports for the commit 1014f8c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3673      +/-   ##
==========================================
+ Coverage   97.65%   97.74%   +0.08%     
==========================================
  Files         132      132              
  Lines        9397     9404       +7     
  Branches     3445     3443       -2     
==========================================
+ Hits         9177     9192      +15     
+ Misses        220      212       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@burtek burtek changed the title Make jsx-no-script-url use shared settings to specify link components [jsx-no-script-url: use shared settings to specify link components Jan 9, 2024
@burtek burtek changed the title [jsx-no-script-url: use shared settings to specify link components [jsx-no-script-url]: use shared settings to specify link components Jan 9, 2024
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.

This looks good.

I've split the PR into two commits - the first one is objectively semver-minor, and has the "multiple properties" feature.

The second, though, I'm a bit uncertain if it's semver-minor or semver-major. Could there be someone relying on linkAttributes not applying to this rule?

@burtek
Copy link
Contributor Author

burtek commented Jan 11, 2024

I don't know. Myself I have this plugin configured in multiple projects (both my own and at work) and we always had to duplicate the setup for this rule.

That being said, that's why the first implementation had the rule option override global settings - that way it wouldn't be a breaking change.

@ljharb
Copy link
Member

ljharb commented Jan 11, 2024

hmm.

even for the case where the rule option wasn't passed, though, someone might have the link components setting and expect the rule NOT to warn on them, no?

@burtek
Copy link
Contributor Author

burtek commented Jan 11, 2024

Yeah, you might be right actually.

I'm fine with this PR being marked as semver-major

@ljharb
Copy link
Member

ljharb commented Jan 11, 2024

Unfortunately that'd delay it from landing probably indefinitely.

How about instead, we modify the second commit so that an additional option is required, a boolean which defaults to false, that enables hooking in the linkComponents (whether the existing option is provided or not)?

@burtek
Copy link
Contributor Author

burtek commented Jan 11, 2024

I can do that

@burtek
Copy link
Contributor Author

burtek commented Jan 11, 2024

@ljharb how about now? :)

@ljharb ljharb merged commit 1014f8c into jsx-eslint:master Jan 11, 2024
309 checks passed
@burtek burtek deleted the patch-1 branch January 11, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Make jsx-no-script-url use shared settings to specify link components
2 participants