Add includeDOM option to rule "forbid-component-props".

[EdwardDrapkin]

I wanted to forbid some prop types on DOM nodes as well as Components, so I added the option here. Let me know if there's anything that needs to be done here to get this approved. Thanks in advance!

[EdwardDrapkin]

How about forbid and forbidOnDOM? forbidOnDOM would be an array of strings just like forbid except that it's empty by default. I can implement a second, clearer error message this way, but I'd have to rework the way the documentation is written a fair bit. Is that alright with you?

If that API is okay, I'll get this PR updated with new documentation, the new API, and tests tomorrow.

[ljharb]

I think that allowing string-or-object in the array gives far greater flexibility - that would also let me potentially forbid a prop on one list of components, but not on others.

[EdwardDrapkin]

I think the flexibility is the same:

 {
  "forbid":  ["className", "style"], 
  "forbidOnDOM": ["style"] 
 }  

would achieve the same thing, no?

I think that's easier for users to manage than: `

   {
     "forbid": [ "style", { "className": { "forbidOnDOM": true } } ]
   }

Or:

   {
     "forbid": [ "style", { "property": "className", "forbidOnDOM": true } ]
   }

(or any other permutation of nesting an object inside the array of string that I can imagine)

It's a completely subjective decision and ultimately it's your code, so I'll do what you think is best, but IMO the first example is the easiest to read if I'm a hypothetical engineer and I'm reading someone else's .eslintrc and I'm not necessarily familiar with this particular plugin.

[ljharb]

@EdwardDrapkin imagine a hypothetical future option where you could forbid "foo" on a Foo component, but allow "foo" on anything else - or where you want to forbid "style" on both React and DOM components, but simultaneously forbid "className" only on React components.

The latter certainly can be achieved with your suggestion, but that won't allow for the former, and that also requires that property names be duplicated in both lists, which can be hard to maintain.

[EdwardDrapkin]

Ahhhhh, I wasn't even considering that! You make excellent points.

Perhaps we could also take this opportunity to allow people the flexibility you imagine? I have two ideas.

First, we could allow passing custom validator functions like:

  {
    "property": "style",
    "validator": "no-dom-components"|"only-dom-components"|"all-components"|function 
  }

where the function is just (componentName:string) => boolean that the plugin would use to choose whether or not to apply the filters to the component? Alternatively, I could do (componentName:string, property:string) => boolean and expose the validation step itself to the user, but I'm having trouble imagining a case where that more useful than the former.

This would be pretty simple to implement, and is probably the most flexible approach, but not exactly user friendly because it's too complex for what I believe is likely to be the common use case...

So maybe, instead we just allow a regular expression to be passed?

  {
    "property": "style",
    "isValid": "^[A-Z]" 
  }

This is probably the easiest for users to use and covers like 99% of the cases people would want, but would require documentation to tell people to use something like isValid: "." to match all components' names, or a special case.

What do you think? I'm not sure what to name the config options in either case to be the most user friendly.

[ljharb]

eslint configs are defined in JSON or yaml, so it'd be impossible to provide a function.

I think adding regex support is unneeded complexity - being able to configure a boolean for DOM and another for React, along with simple component names per-property that can override the booleans, seems like it would cover all the current and potential use cases pretty well.

[EdwardDrapkin]

Can't you use a JS file as an .eslintrc.js? It's irrelevant anyway...

How does this look?

 {
   "property": <string>,
   "include": "React"|"DOM"|"Both"|<array> 
 }

Is include a good param name?

[ljharb]

I think I'd prefer:

{
  "property": <string>,
  "allowOnComponents": <boolean>, // default false
  "allowOnDOM": <boolean>, // default false
  "forbid": <array of component name strings>, // overrides previous two booleans
}

where passing a simple string "foo" as the property name is equivalent to:

{
    "property": "foo",
    "allowOnComponents": false,
    "allowOnDOM": true, // simply so it's not a breaking change
    "forbid": [],
}

[EdwardDrapkin]

Perfect. I'll get this PR updated sometime in the next 24-48 hours with these changes!

[EdwardDrapkin]

Okay, the football sucked, so I went ahead and got to work on this.

I did some extra work that we hadn't discussed, but I was adding examples to the documentation and surfaced some cases I wanted to address.

I added defaultAllowOnComponents and defaultAllowOnDOM as top level config options to do exactly what they say. I also allowed a white allow alongside the blacklist forbid you asked for, because I imagined a case where someone was okay with font icon i tags having classNames but everything else going through something like css modules.

I updated the documentation and I think it's okay, but please do read and update it if you feel it's lacking. Or tell me and I gladly will!

I also added more test cases so Istanbul reports 100% coverage for me.

Let me know if there's any issues this time.

Thanks so much for your time helping me get this working (and for the rule in the first place)!

[ljharb]

I think this is looking much better, and I like the additions you've made! I think we'll want to add a very large quantity of test cases, that cover as many interesting combinations of rules as possible.

[EdwardDrapkin]

I added some more test cases, up to 21 now. I'm having trouble coming up with cases that aren't already tested.

[EdwardDrapkin]

I think I addressed everything from your notes - and I'll make a separate pull request to add React.createElement support - but is there anything else that needs to be done now?

[burabure]

this might clash with https://github.com/yannickcr/eslint-plugin-react/pull/936

[ljharb]

@burabure i think perhaps we might want to follow this PR's lead for "forbidPatterns", and let the "dom or component" options be pattern-specific instead of rule-specific.

[burabure]

@EdwardDrapkin @ljharb maybe I should take this PR as an starting point for #936 does that sound like a good idea?

[ljharb]

@EdwardDrapkin would you be alright with @burabure building off your commits to achieve both this PR and #936?

[burabure]

@EdwardDrapkin ping

[burabure]

@ljharb I don't think @EdwardDrapkin is going to reply. Should I go ahead?

[ljharb]

Sure, go for it.

[EdwardDrapkin]

I'm sorry, I've been really sick lately fighting off MRSA (yikes!) and I won't have time to get to this. You should definitely go ahead.

[ljharb]

Thanks for confirming - @burabure, do your thing :-)

[burabure]

@EdwardDrapkin sorry to hear that, I have a friend that had MRSA and I know it really sucks. Hope you feel better soon!

[burabure]

I don't have time to see this through, if anyone is up to it please do =)