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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Sep 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
### Added
* add type generation ([#3830][] @voxpelli)
* [`no-unescaped-entities`]: add suggestions ([#3831][] @StyleShit)
* [`forbid-component-props`]: add `allowedForPatterns`/`disallowedForPatterns` options ([#3805][] @Efimenko)

[#3831]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3831
[#3830]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3830
[#3805]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3805

## [7.36.1] - 2024.09.12

Expand Down
45 changes: 37 additions & 8 deletions docs/rules/forbid-component-props.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,17 @@ custom message, and a component allowlist:
}
```

For glob string patterns:
Use `disallowedFor` as an exclusion list to warn on props for specific components. `disallowedFor` must have at least one item.

```js
{
"propName": "someProp",
"disallowedFor": ["SomeComponent", "AnotherComponent"],
"message": "Avoid using someProp for SomeComponent and AnotherComponent"
}
```

For `propNamePattern` glob string patterns:

```js
{
Expand All @@ -65,23 +75,42 @@ For glob string patterns:
}
```

Use `disallowedFor` as an exclusion list to warn on props for specific components. `disallowedFor` must have at least one item.
```js
{
"propNamePattern": '**-**',
"allowedForPatterns": ["*Component"],
"message": "Avoid using kebab-case except components that match the `*Component` pattern"
}
```

Use `allowedForPatterns` for glob string patterns:

```js
{
"propName": "someProp",
"disallowedFor": ["SomeComponent", "AnotherComponent"],
"message": "Avoid using someProp for SomeComponent and AnotherComponent"
"allowedForPatterns": ["*Component"],
"message": "Avoid using `someProp` except components that match the `*Component` pattern"
}
```

Use `disallowedForPatterns` for glob string patterns:

```js
{
"propName": "someProp",
"disallowedForPatterns": ["*Component"],
"message": "Avoid using `someProp` for components that match the `*Component` pattern"
}
```

For glob string patterns:
Combine several properties to cover more cases:

```js
{
"propNamePattern": "**-**",
"disallowedFor": ["MyComponent"],
"message": "Avoid using kebab-case for MyComponent"
"propName": "someProp",
"allowedFor": ['div'],
"allowedForPatterns": ["*Component"],
"message": "Avoid using `someProp` except `div` and components that match the `*Component` pattern"
}
```

Expand Down
75 changes: 67 additions & 8 deletions lib/rules/forbid-component-props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ module.exports = {
uniqueItems: true,
items: { type: 'string' },
},
allowedForPatterns: {
type: 'array',
uniqueItems: true,
items: { type: 'string' },
},
message: { type: 'string' },
},
additionalProperties: false,
Expand All @@ -66,12 +71,20 @@ module.exports = {
minItems: 1,
items: { type: 'string' },
},
disallowedForPatterns: {
type: 'array',
uniqueItems: true,
minItems: 1,
items: { type: 'string' },
},
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 •
edited
Loading

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 •
edited
Loading

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.

{ required: ['disallowedFor'] },
{ required: ['disallowedForPatterns'] },
],
additionalProperties: false,
},

{
type: 'object',
properties: {
Expand All @@ -81,6 +94,11 @@ module.exports = {
uniqueItems: true,
items: { type: 'string' },
},
allowedForPatterns: {
type: 'array',
uniqueItems: true,
items: { type: 'string' },
},
message: { type: 'string' },
},
additionalProperties: false,
Expand All @@ -95,9 +113,18 @@ module.exports = {
minItems: 1,
items: { type: 'string' },
},
disallowedForPatterns: {
type: 'array',
uniqueItems: true,
minItems: 1,
items: { type: 'string' },
},
message: { type: 'string' },
},
required: ['disallowedFor'],
anyOf: [
ljharb marked this conversation as resolved.
Show resolved Hide resolved
{ required: ['disallowedFor'] },
{ required: ['disallowedForPatterns'] },
],
additionalProperties: false,
},
],
Expand All @@ -114,8 +141,10 @@ module.exports = {
const propPattern = value.propNamePattern;
const prop = propName || propPattern;
const options = {
allowList: typeof value === 'string' ? [] : (value.allowedFor || []),
disallowList: typeof value === 'string' ? [] : (value.disallowedFor || []),
allowList: [].concat(value.allowedFor || []),
allowPatternList: [].concat(value.allowedForPatterns || []),
disallowList: [].concat(value.disallowedFor || []),
disallowPatternList: [].concat(value.disallowedForPatterns || []),
message: typeof value === 'string' ? null : value.message,
isPattern: !!value.propNamePattern,
};
Expand All @@ -140,10 +169,40 @@ module.exports = {
return false;
}

function checkIsTagForbiddenByAllowOptions() {
if (options.allowList.indexOf(tagName) !== -1) {
return false;
}

if (options.allowPatternList.length === 0) {
return true;
}

return options.allowPatternList.every(
(pattern) => !minimatch(tagName, pattern)
);
}

function checkIsTagForbiddenByDisallowOptions() {
if (options.disallowList.indexOf(tagName) !== -1) {
return true;
}

if (options.disallowPatternList.length === 0) {
return false;
}

return options.disallowPatternList.some(
(pattern) => minimatch(tagName, pattern)
);
}

const hasDisallowOptions = options.disallowList.length > 0 || options.disallowPatternList.length > 0;

// disallowList should have a least one item (schema configuration)
const isTagForbidden = options.disallowList.length > 0
? options.disallowList.indexOf(tagName) !== -1
: options.allowList.indexOf(tagName) === -1;
const isTagForbidden = hasDisallowOptions
? checkIsTagForbiddenByDisallowOptions()
: checkIsTagForbiddenByAllowOptions();

// if the tagName is undefined (`<this.something>`), we assume it's a forbidden element
return typeof tagName === 'undefined' || isTagForbidden;
Expand Down
Loading
Loading