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

[New] forbid-component-props: add allowedForPatterns and disallowedForPatterns options #3805

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

Efimenko
Copy link
Contributor

@Efimenko Efimenko commented Aug 22, 2024

Initially, issue was described here, but it was closed by another PR (although this PR implements different case).
So, this PR just reimplements Regex implementation with globs as was suggested in one of the comments.

@Efimenko
Copy link
Contributor Author

@ljharb what do you think?

lib/rules/forbid-component-props.js Outdated Show resolved Hide resolved
lib/rules/forbid-component-props.js Outdated Show resolved Hide resolved
lib/rules/forbid-component-props.js Outdated Show resolved Hide resolved
lib/rules/forbid-component-props.js Outdated Show resolved Hide resolved
@ljharb ljharb marked this pull request as draft September 11, 2024 19:42
@Efimenko
Copy link
Contributor Author

Thanks a lot for the feedback, I will apply the changes.

@Efimenko Efimenko requested a review from ljharb September 19, 2024 16:03
@Efimenko

This comment was marked as resolved.

@Efimenko Efimenko marked this pull request as ready for review September 20, 2024 08:45
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.

Should there also be examples of using propNamePatterns with one of these new options?

message: { type: 'string' },
},
required: ['disallowedFor'],
anyOf: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be:

Suggested change
anyOf: [
oneOf: [

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, because with oneOf you can't use disallowedFor and disallowedForPatterns simultaneously, while it is still a case. Here is one of the test cases for example: https://github.com/jsx-eslint/eslint-plugin-react/pull/3805/files#diff-0095adf2f1f3e139bb1a1666d7611671ad90ae66f67de0f5cdc7cb8884351968R825-R847

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with oneOf, only one is required but that shouldn't preclude using both, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb so, based on the documentation, only one can be successful at a time https://json-schema.org/draft/2020-12/json-schema-core#section-10.2.1.3-2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. but "only one is required" is the intended semantics, right?

i suppose they're identical semantics, but i read "anyOf" with more optionality than "oneOf", personally ¯\_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, semantically about "at least one required" they are identical probably, but "use several of them simultaneously" is not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right - and "any of" implies multiple matches are expected, "one of" implies only one match is expected

Copy link
Contributor Author

@Efimenko Efimenko Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to summarize:

  • anyOf gives the ability to use several fields like disallowedFor and disallowedForPatterns at the same time;
  • oneOf forbids the use of several fields at the same time, as the usage of two fields is not valid in this case;

So, here we are, just discussing the difference, or are we considering the replacement with oneOf? A bit hard to understand 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol i'm saying that i think that the actual behavior will be identical with anyOf or oneOf here, but that oneOf seems more appropriate to me.

I won't hold up the PR further on this topic, though.

Copy link
Contributor Author

@Efimenko Efimenko Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it, so, actually, no.
With oneOf, if you pass at the same time disallowedFor and disallowedForPatterns - it will be a validation error.
With anyOf - it won't.

lib/rules/forbid-component-props.js Show resolved Hide resolved
@Efimenko
Copy link
Contributor Author

Should there also be examples of using propNamePatterns with one of these new options?

I've added one: https://github.com/jsx-eslint/eslint-plugin-react/pull/3805/files#diff-1cf5e390e9444274ee8064c64a72965facfa2171c5453a130f0329e57fe991adR79-R83

Also, I've added one for a more complex setup: https://github.com/jsx-eslint/eslint-plugin-react/pull/3805/files#diff-1cf5e390e9444274ee8064c64a72965facfa2171c5453a130f0329e57fe991adR106-R113 (Relates to this comment #3805 (comment))

@Efimenko
Copy link
Contributor Author

For the future, correct me if I'm wrong, but here, rebase is a preferable option over merge.

@ljharb
Copy link
Member

ljharb commented Sep 24, 2024

Yes, feel free to do whatever you like, but I'll rebase and clean it up as part of landing it.

@ljharb ljharb merged commit 3073214 into jsx-eslint:master Sep 26, 2024
376 checks passed
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this pull request Sep 27, 2024
##### [v7.37.0](https://github.com/jsx-eslint/eslint-plugin-react/blob/HEAD/CHANGELOG.md#7370---20240926)

##### Added

-   add type generation ([#3830][] [@voxpelli](https://github.com/voxpelli))
-   \[`no-unescaped-entities`]: add suggestions ([#3831][] [@StyleShit](https://github.com/StyleShit))
-   \[`forbid-component-props`]: add `allowedForPatterns`/`disallowedForPatterns` options ([#3805][] [@Efimenko](https://github.com/Efimenko))
-   \[`no-unstable-nested-components`]: add `propNamePattern` to support custom render prop naming conventions ([#3826][] [@danreeves](https://github.com/danreeves))

##### Changed

-   \[readme] flat config example for react 17+ ([#3824][] [@GabenGar](https://github.com/GabenGar))

[7.36.2]: jsx-eslint/eslint-plugin-react@v7.36.1...v7.36.2

[#3831]: jsx-eslint/eslint-plugin-react#3831

[#3830]: jsx-eslint/eslint-plugin-react#3830

[#3826]: jsx-eslint/eslint-plugin-react#3826

[#3824]: jsx-eslint/eslint-plugin-react#3824

[#3805]: jsx-eslint/eslint-plugin-react#3805
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants