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

EdwardDrapkin · 2016-09-18T10:14:29Z

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!

ljharb

Either way, this will need tests.

ljharb · 2016-09-18T15:24:56Z

docs/rules/forbid-component-props.md


 An array of strings, with the names of props that are forbidden. The default value of this option is `['className', 'style']`.
+
+### `includeDOM`


This seems like something that will vary per prop name. Perhaps the enhancement should be that forbid can have strings or objects, and if an object, it can be something like { "property": "foo", "includeDOM": true }?

EdwardDrapkin · 2016-09-18T23:28:26Z

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 · 2016-09-19T00:08:19Z

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 · 2016-09-19T00:35:25Z

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 · 2016-09-19T00:46:44Z

@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 · 2016-09-19T01:10:33Z

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 · 2016-09-19T01:12:53Z

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 · 2016-09-19T01:16:58Z

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 · 2016-09-19T01:21:12Z

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 · 2016-09-19T01:23:01Z

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

ljharb · 2016-09-19T03:08:38Z

docs/rules/forbid-component-props.md

 ```js
 ...
-"forbid-component-props": [<enabled>, { "forbid": [<string>] }]
+"forbid-component-props": [<enabled>, { "forbid": [<string>|<object>], "defaultAllowOnComponents": <bool>, "defaultAllowOnDOM": <bool>}]


the two "default" booleans need to be on the object, not on the outer config object. not sure how to denote this tho.

unless you're suggesting that they'd be top-level settings as well? if so, i do think i like that.

yeah, there's an outer config option for specifying the default behavior and an option for overloading the default, but i wasn't quite sure how to make that clear in the documentation other than in the examples...

EdwardDrapkin · 2016-09-19T03:09:17Z

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 · 2016-09-19T03:10:29Z

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 · 2016-09-19T03:41:13Z

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

ljharb · 2016-09-19T04:09:43Z

lib/rules/forbid-component-props.js

+      {
+        type: 'object',
+        properties: {
+          defaultAllowOnDOM: {


i think the schema is missing defaultAllowOnComponents?

oops is all i got

ljharb · 2016-09-19T04:10:29Z

lib/rules/forbid-component-props.js

-      additionalProperties: true
-    }]
+        },
+        additionalProperties: true


i think we can maybe set this to false, so nobody accidentally passes an invalid config option?

okay, I can change it... I just didn't wanna mess with what you already had

yeah that's a good point, it might constitute a breaking change. probably best to leave it true for now.

ljharb · 2016-09-19T04:11:22Z

lib/rules/forbid-component-props.js

+          defaultAllowOnDOM: {
+            type: 'boolean'
+          },
+          forbid: {


the schema also seems to be missing allow, which i think should probably share a schema with forbid? (ie, reuse the array schema in both)

okay i was a little excited

ljharb · 2016-09-19T04:11:50Z

lib/rules/forbid-component-props.js


-      var forbid = configuration.forbid || DEFAULTS;
-      return forbid.indexOf(prop) >= 0;
+      for (var i = 0; i < input.length; i++) {


can we use input.forEach here, instead of a for loop?

ljharb · 2016-09-19T04:13:42Z

lib/rules/forbid-component-props.js

+
+    function getError(prop, tag) {
+      if (rules[prop].forbid.indexOf(tag) > -1) {
+        return 'Prop `' + prop + '` is specifically forbidden on tags named ' + tag;


'… on ' + tag + ' components'?

It swaps between "$tag Components" and "$tag DOM nodes" now.

ljharb · 2016-09-19T04:15:46Z

tests/lib/rules/forbid-component-props.js

+    ]
+  }, {
+    code: [
+      'var First = React.createClass({',


for each of the tests you're using for React.createClass, let's please also add a test case for an SFC, and a class-based component.

I only added one total test case for each. I figure if one test case works for all three, all test cases should, because we're only getting at the JSX tags inside the (render) function body, right? I can add more if you think they're necessary.

Ah, that's another thing - we should check for React.createElement calls too (ie, non-JSX) :-)

If you want me to, that's cool and I'm down to learn how, but I don't know... so can you point me in the right direction if there's another plugin I might be able to read? If not, I'll just RTFM I guess.

And do you have any problem if I do it in a separate pull request? It seems outside the scope of this one and I might not have time to get to it until this upcoming weekend.

Hmm, you may have opened a can of worms, at first glance there are several other plugins that do things with props that rely on JSXExpression too... no-duplicate-props, no-unknown-property, sort-props, maybe more....

What is the best way to go forward if this is an issue that affects multiple rules? Open a new issue and then read each rule one by one and make sure they're not affected?

There are a number of rules that address both - however, I'm fine if this rule covers jsx right now, but is expanded to non-JSX in a future PR.

ljharb · 2016-09-19T04:30:23Z

lib/rules/forbid-component-props.js

+          defaultAllowOnComponents: {
+            type: 'boolean'
+          },
          forbid: {


is there not a top-level allow as well?

Wouldn't that mean the entire plugin would have to switch into whitelist mode? Would that be useful to anyone?

Hm, yeah i guess if i wanted to override a forbid i'd now just modify the object inside the array. This is fine as-is.

EdwardDrapkin · 2016-09-19T06:18:13Z

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 · 2016-11-02T14:32:36Z

this might clash with #936

ljharb · 2016-11-03T03:19:02Z

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

burabure · 2016-11-10T14:58:17Z

@EdwardDrapkin ping

burabure · 2016-11-16T14:36:24Z

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

ljharb · 2016-11-16T16:35:19Z

Sure, go for it.

EdwardDrapkin · 2016-11-18T00:41:35Z

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 · 2016-11-18T01:36:20Z

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

burabure · 2016-11-18T21:06:42Z

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

burabure · 2017-04-19T15:37:13Z

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

first try to includeDOM option

f77bb9e

ljharb requested changes Sep 18, 2016

View reviewed changes

New API per request + tested

44345f6

ljharb reviewed Sep 19, 2016

View reviewed changes

ljharb added the enhancement label Sep 19, 2016

Edward Drapkin added 2 commits September 18, 2016 23:14

lint errors, my bad

aa810cd

Even more test cases

f11be2d

ljharb requested changes Sep 19, 2016

View reviewed changes

Edward Drapkin added 4 commits September 19, 2016 00:23

fix missing schema elements

4b0b6c1

switch to foreach

df54612

butter error messages

f5201bd

tests for SFC and es6 classes

e378ae7

ljharb reviewed Sep 19, 2016

View reviewed changes

Edward Drapkin added 2 commits September 19, 2016 00:36

code readability

99ab61f

revert potentially breaking change

043b842

Merge remote-tracking branch 'upstream/master'

9c31009

EdwardDrapkin closed this Nov 18, 2016

ljharb reopened this Apr 19, 2017

ljharb added the help wanted label Apr 19, 2017

yannickcr force-pushed the master branch from f4737f0 to 73abadb Compare August 9, 2017 21:48

yannickcr force-pushed the master branch from c8dedeb to dc9f85e Compare September 24, 2017 18:16

yannickcr force-pushed the master branch from dca5cd5 to c148893 Compare November 18, 2017 17:26

ljharb marked this pull request as draft December 30, 2020 04:39

ljharb force-pushed the master branch from 0d1a62e to f7fe38f Compare July 11, 2022 15:02

ljharb force-pushed the master branch 6 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45

ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19

ljharb force-pushed the master branch from 57bb8d1 to ae64aa8 Compare June 8, 2023 22:37

ljharb force-pushed the master branch 2 times, most recently from 380e32c to 51d342b Compare July 4, 2024 15:25

ljharb force-pushed the master branch from 301a48d to 2c0b10c Compare December 9, 2024 20:26


		An array of strings, with the names of props that are forbidden. The default value of this option is `['className', 'style']`.

		### `includeDOM`

Uh oh!

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

EdwardDrapkin commented Sep 18, 2016

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdwardDrapkin commented Sep 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Sep 19, 2016

Uh oh!

EdwardDrapkin commented Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Sep 19, 2016

Uh oh!

EdwardDrapkin commented Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Sep 19, 2016

Uh oh!

EdwardDrapkin commented Sep 19, 2016

Uh oh!

ljharb commented Sep 19, 2016

Uh oh!

EdwardDrapkin commented Sep 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdwardDrapkin commented Sep 19, 2016

Uh oh!

ljharb commented Sep 19, 2016

Uh oh!

EdwardDrapkin commented Sep 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdwardDrapkin commented Sep 18, 2016 •

edited

Loading

EdwardDrapkin commented Sep 19, 2016 •

edited

Loading

EdwardDrapkin commented Sep 19, 2016 •

edited

Loading

burabure commented Nov 18, 2016 •

edited

Loading