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

Add disallowedFor to forbid-component-props and allowedFor to forbid-dom-props #3557

Merged
merged 2 commits into from
Apr 15, 2023

Conversation

cincodenada
Copy link

@cincodenada cincodenada commented Mar 28, 2023

My use case is component props: I am using a component system (Antd) and want to disallow the required parameter on Form.Item to avoid confusion with a require parameter in HOCs that we create.

I found #3422 already existing, so this is a PR to solve that, as well as adding allowList to forbid-dom-props for symmetry.

I haven't yet figured out what the best way is to raise configuration issues, right now I'm just throwing an error, let me know if there's a better way!

(Motivation details: in Antd require only controls styling, and doesn't actually validate, in our HOCs I want it to validate, but don't want that confusion to bleed over into using require in the base (non-HOC) Form.Items)

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #3557 (a2600b2) into master (1fc7d34) will decrease coverage by 0.02%.
The diff coverage is 89.47%.

❗ Current head a2600b2 differs from pull request most recent head 4a92667. Consider uploading reports for the commit 4a92667 to get more accurate results

@@            Coverage Diff             @@
##           master    #3557      +/-   ##
==========================================
- Coverage   97.62%   97.60%   -0.02%     
==========================================
  Files         132      133       +1     
  Lines        9289     9296       +7     
  Branches     3396     3399       +3     
==========================================
+ Hits         9068     9073       +5     
- Misses        221      223       +2     
Impacted Files Coverage Δ
lib/rules/forbid-dom-props.js 96.15% <75.00%> (-3.85%) ⬇️
lib/rules/forbid-component-props.js 96.55% <83.33%> (-3.45%) ⬇️
lib/util/isForbidden.js 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cincodenada
Copy link
Author

cincodenada commented Mar 28, 2023

Sigh, okay, so I just realized that this is largely a duplicate of #3417, which I missed because it wasn't tagged on #3422 becuase that issue started as a discussion on #2946.

The discussion on #3417 also states:

In the future, please do not send PRs for changes you're not sure are wanted.

If there is some sort of policy or preference here, it would be nice to have stated in CONTRIBUTING.md - generally I don't file PRs without raising an issue first, but since there was an existing issue without neither satisfying resolution nor refusal, and it was a fairly small change, I went ahead and drafted it up.

@ljharb
Copy link
Member

ljharb commented Apr 14, 2023

I'm going to pull your changes into #3417, but i'll drop the forbid-dom-props changes, since that needs to happen in another PR.

@ljharb ljharb force-pushed the add-disallowed-allowed branch 2 times, most recently from 61d31b8 to 4a92667 Compare April 14, 2023 22:21
@ljharb ljharb merged commit 4a92667 into jsx-eslint:master Apr 15, 2023
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.

2 participants