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

Rule enhancement: allow regex in forbid-component-props allowedFor / disallowedFor #3686

Open
2 tasks done
bschlenk opened this issue Feb 8, 2024 · 11 comments · May be fixed by #3687
Open
2 tasks done

Rule enhancement: allow regex in forbid-component-props allowedFor / disallowedFor #3686

bschlenk opened this issue Feb 8, 2024 · 11 comments · May be fixed by #3687
Labels

Comments

@bschlenk
Copy link

bschlenk commented Feb 8, 2024

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
  • My issue appears in the command-line and not only in the text editor

Description Overview

My team uses react/forbid-component-props to disallow className and style props, but we maintain a list of legacy components that took these props before we introduced this rule. We now also have a bunch of icon components in the codebase that all have names like Icon24Something. We like to pass style or className props to these to override css properties in some cases, so it'd be nice to be able to allow any component that matches /^Icon\d+/.

Does this sound like a change you'd be happy to accept? I'd be okay sending a PR. Otherwise I'm also happy to fork the rule internally for our use case, but figured I'd check here to contribute back to the community.

Expected Behavior

I'd expect to be able to pass a regex to allowedFor.

eslint-plugin-react version

v7.32.2

eslint version

v8.36.0

node version

v18.17.1

@bschlenk bschlenk added the bug label Feb 8, 2024
@bschlenk
Copy link
Author

bschlenk commented Feb 8, 2024

What I'd add is (in addition to allowedFor and disallowedFor) allowedForRegex and disallowedForRegex options that take an optional string. Then you can check in both the arrays and test the regex against the string. This would be a backwards compatible change.

@ljharb
Copy link
Member

ljharb commented Feb 8, 2024

Allowing regex strings in eslint config is an extremely bad idea (it's a CVE farm), but allowing globs would be tenable.

@bschlenk
Copy link
Author

bschlenk commented Feb 8, 2024

What do you mean by CVE farm?

Looks like this package already depends on minimatch, I can rework my approach.

@bschlenk
Copy link
Author

bschlenk commented Feb 8, 2024

I think I get it. It isn't a real issue (the user of this package owns their config so why would they ddos themself with a bad regex) but you'd rather not deal with automated security reports against your package. If that's the case, seems unfortunate but understandable.

@bschlenk
Copy link
Author

bschlenk commented Feb 8, 2024

Alright, unless you think globs would be a useful addition here, I'm going to close this one out. Globs aren't really meant for generic string matching IMO, more for paths. I'm fine with modifying this rule in our repo or figuring out a different api for our icons that doesn't involve className or style.

@ljharb
Copy link
Member

ljharb commented Feb 8, 2024

If globs can help you solve your use case, I think it's useful, but I agree they're more for paths than strings.

@figmatom
Copy link

figmatom commented Feb 9, 2024

Thoughts on supporting StartsWith and EndsWith instead? Gives you the 95% case of regexes.

@ljharb
Copy link
Member

ljharb commented Feb 9, 2024

You can do that with globs - *end or start*

@figmatom
Copy link

Yeah, they will solve most things, but globs are just such a weird API, since folks associate globs pretty strongly with file systems, and this applies to the names. I would make the assumption taht it was a glob on the import path, not the filename, and do a triple take on the docs 😅 But that's definitely only one antecdote. That's why I was suggesting a startswith/endswith or just an includes.

@dominicfraser
Copy link

@bschlenk We'd also love to be able to allow any component prop based on a prefix, for very similar reasons.

Keeping an interested eye on the discussion here.

@ljharb
Copy link
Member

ljharb commented Feb 18, 2024

We already use that pattern in a few places, as does eslint itself, so I think it'll be fine.

If you can adapt #3687 to use a glob-based approach, I'd be happy to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 participants