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

Feature request: [jsx-pascal-case] Support regex/glob match #2905

Closed
bcherny opened this issue Jan 23, 2021 · 4 comments · Fixed by #2906
Closed

Feature request: [jsx-pascal-case] Support regex/glob match #2905

bcherny opened this issue Jan 23, 2021 · 4 comments · Fixed by #2906

Comments

@bcherny
Copy link

bcherny commented Jan 23, 2021

Like the ESLint camelcase rule: https://github.com/eslint/eslint/blob/master/docs/rules/camelcase.md

This is to support suffixes like _DEPRECATED, etc.

bcherny pushed a commit to bcherny/eslint-plugin-react that referenced this issue Jan 23, 2021
bcherny pushed a commit to bcherny/eslint-plugin-react that referenced this issue Jan 23, 2021
@ljharb
Copy link
Member

ljharb commented Jan 23, 2021

It's not a good idea to allow regex configs. Globs are acceptable, though.

(in the future, it may be best to wait on a PR until a maintainer has responded to the issue).

@bcherny
Copy link
Author

bcherny commented Jan 23, 2021

Hey, thanks for the quick response and review! I figured the worst thing that can happen with user-supplied regex is it might make this specific ESLint rule run a bit slow (and only for a pathological case, eg. lots of backtracking in an old V8), and this is a worthwhile cost in order to provide an API consistent with ESLint core rules.

That said, I don't have a strong opinion, and am happy to use a glob instead. Do you have a preference between minimatch, micromatch, or hand-rolling something?

I'll also respond to your question on the linked PR here, to keep the convo in one place:

As to this rule specifically, I'd love to understand the use case a bit more - why do you need "deprecated" to be in the name on the consuming side? import { Foo_DEPRECATED as Foo }, for example, works fine.

The specific motivating use case is for a part of the FB codebase we ban import aliasing, and have an existing lint rule to warn about _DEPRECATED module usages. Currently, we double-warn: our lint rule warns when importing a _DEPRECATED module, and jsx-pascal-case warns again when using the module. My hope was to reduce noise a bit.

I'm also more than happy to fork the rule for our own use case, and not contribute the fix to this repo -- let me know what you prefer.

@ljharb
Copy link
Member

ljharb commented Jan 24, 2021

To be honest, the bigger danger is getting caught in another worthless CVE (like most of them are) about catastrophic regex backtracking. eslint-plugin-import uses minimatch, so we might as well follow suit here.

Banning import aliasing is an ... interesting ... choice.

would it be too much to suggest naming them Foo_Deprecated or FooDeprecated, assuming those pass pascal-case? If so, then sure, let’s go ahead with supporting the globs.

@bcherny
Copy link
Author

bcherny commented Jan 25, 2021

would it be too much to suggest naming them Foo_Deprecated or FooDeprecated, assuming those pass pascal-case? If so, then sure, let’s go ahead with supporting the globs.

We certainly could, though we tend to prefer suffixes like _DEPRECATED, _DO_NOT_USE_OR_YOU_WILL_BE_FIRED, etc. because they sound scarier, and at scale, this matters as a way to deter folks.

Let me update the diff with minimatch. Hopefully it comes in handy for others, too.

bcherny pushed a commit to bcherny/eslint-plugin-react that referenced this issue Jan 25, 2021
@ljharb ljharb changed the title Feature request: [jsx-pascal-case] Support regex match Feature request: [jsx-pascal-case] Support regex/glob match Jan 25, 2021
ljharb pushed a commit to bcherny/eslint-plugin-react that referenced this issue Jan 25, 2021
@ljharb ljharb closed this as completed in 4881bae Jan 25, 2021
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