New `boolean-prop-naming` rule for enforcing the naming of booleans #1264

EvHaus · 2017-06-19T19:55:51Z

At my workplace, we prefer to name our boolean props as "isSomething" or "hasWhatever". This is an ESLint rule which allows devs to specify a Regex pattern for how they want their boolean props named.

Tested with createReactClass, ES6 components, stateless components and Flowtype.

I support, in the future, this rule can be expanded to support naming rules for ANY props, but that felt like overkill for the time being. Hopefully this rule will be useful as is.

ljharb

What about custom propTypes that enforce booleans but aren't PropTypes.bool? For example, mutuallyExclusiveTrueProps from https://www.npmjs.com/package/airbnb-prop-types.

Can there be a way to customize the name of custom propTypes that are booleans?

ljharb · 2017-06-19T20:13:01Z

docs/rules/boolean-prop-naming.md

+```jsx
+var Hello = createReactClass({
+  propTypes: {
+    enabled: PropTypes.boolean


PropTypes.bool - .boolean isn't a thing (same throughout)

Oops. Good catch.

ljharb · 2017-06-19T20:13:23Z

docs/rules/boolean-prop-naming.md

+
+### `rule`
+
+The Regex pattern to use when validating the name of the prop. Some common examples are:


regex or RegExp

ljharb · 2017-06-19T20:13:50Z

docs/rules/boolean-prop-naming.md

+
+## About component detection
+
+For this rule to work we need to detect React components, this could be very hard since components could be declared in a lot of ways.


We should be using Components.detect in all our rules; is there a reason this paragraph needs to exist?

Was copy/pasted from another rule's document. Should I remove it?

Yeah, this seems more like a project-level thing, not a rule-level thing.

WIll remove.

ljharb · 2017-06-19T20:14:00Z

lib/rules/boolean-prop-naming.js

+ */
+'use strict';
+
+var has = require('has');


Thanks. Not all rules were using ES6, wasn't sure which version of Node we're supporting. Will update accordingly.

ljharb · 2017-06-19T20:14:34Z

tests/lib/rules/boolean-prop-naming.js

+    // createReactClass components with PropTypes
+    code: [
+      'var Hello = createReactClass({',
+      '  propTypes: {isSomething: PropTypes.boolean},',


i think all the PropType test cases are invalid - s/boolean/bool/g.

EvHaus · 2017-06-19T22:00:07Z

What about custom propTypes that enforce booleans but aren't PropTypes.bool? For example, mutuallyExclusiveTrueProps from https://www.npmjs.com/package/airbnb-prop-types.

Can there be a way to customize the name of custom propTypes that are booleans?

Good idea. I've added a new propTypeNames configuration option.

ljharb · 2017-06-20T07:52:20Z

docs/rules/boolean-prop-naming.md

+
+### `rule`
+
+The RegExp pattern to use when validating the name of the prop. Some common examples are:


what's the default for this option?

null is the default at the moment (ie. the naming checks are disabled). It didn't seem right to enforce my own preferences on all users. Do you think is makes sense to set "(is|has)*" as the default?

Since if not, the rule does nothing by default, yes.

The ideal scenario is that nobody ever needs to write a regex, except for the minority edge cases.

Agreed. Will set a default.

ljharb · 2017-06-20T07:53:26Z

lib/rules/boolean-prop-naming.js

+  create: Components.detect(function(context, components, utils) {
+    const sourceCode = context.getSourceCode();
+    const config = context.options[0] || {};
+    const rule = config.rule ? new RegExp(config.rule) : null;


i'm really really not a fan of user-supplied regexes, and passing unsanitized input into new RegExp can definitely cause catastrophic backtracking and other malicious regexes to be invoked.

Can we simplify this somehow, perhaps requiring a list of prefixes?

I'm not opposed to replacing the RegExp entry to just prefixes, however, that makes the rule much less powerful and less customizable. Having user-supplied regexes in ESLint rules isn't uncommon (eg. http://eslint.org/docs/rules/id-match) when specifying naming conventions for things.

ljharb · 2017-06-20T07:53:57Z

lib/rules/boolean-prop-naming.js

+        return false;
+      }
+
+      return Boolean(


usually we just do ‼x instead of Boolean(x), nbd tho

ljharb · 2017-06-20T07:54:19Z

tests/lib/rules/boolean-prop-naming.js

+require('babel-eslint');
+
+var parserOptions = {
+  ecmaVersion: 8,


this can probably be 6/2015, yes?

ljharb · 2017-06-20T07:54:29Z

tests/lib/rules/boolean-prop-naming.js

+  ecmaVersion: 8,
+  sourceType: 'module',
+  ecmaFeatures: {
+    experimentalObjectRestSpread: true,


i don't think we're using this feature in these tests

ljharb · 2017-06-20T16:33:00Z

lib/rules/boolean-prop-naming.js

+          type: 'array'
+        },
+        rule: {
+          default: null,


Can we put the default here, in the schema, and use minLength: 1 to avoid the empty string?

ljharb · 2017-06-20T16:34:03Z

lib/rules/boolean-prop-naming.js

+    const sourceCode = context.getSourceCode();
+    const config = context.options[0] || {};
+    const rule = new RegExp(config.rule || DEFAULT_RULE);
+    const propTypeNames = config.propTypeNames ? config.propTypeNames : ['bool'];


Similarly, can this default go in the schema instead?

For some reason, default schema definition doesn't work for this option. Maybe because it's an array type? Specifying a default at the schema level here never returns anything from config object.

i guess you can't have a default array in jsonschema.

in that case, a ? a : b should always be a || b :-)

Updated. Thanks!

ljharb · 2017-06-20T16:34:27Z

lib/rules/boolean-prop-naming.js

+          items: {
+            type: 'string'
+          },
+          type: 'array'


Let's add uniqueItems: true and minItems: 1

ljharb · 2017-06-21T02:03:00Z

lib/rules/boolean-prop-naming.js

+        }
+
+        const list = components.list();
+        for (let component in list) {


Let's avoid for..in; can we use Object.keys().forEach here instead?

ljharb

Thanks!

Let's wait til we get at least one other collab review before merging.

lencioni

Thanks for the contribution!

lencioni · 2017-06-26T18:24:15Z

lib/rules/boolean-prop-naming.js

+          (
+            prop.type === 'ObjectTypeProperty' &&
+            prop.value.type === 'BooleanTypeAnnotation' &&
+            getPropName(prop).search(rule) < 0


I think rule.test(getPropName(prop)) might be more appropriate here (and below)

Will change to use .exec() actually, since (after some research) that appears to be the most performant way to execute Regex rules. Hope that's ok.

exec mutates; test doesn't - test might be better.

lencioni · 2017-06-26T18:25:40Z

lib/rules/boolean-prop-naming.js

+        const propName = getPropName(propNode);
+        context.report({
+          node: propNode,
+          message: `Prop name (${propName}) doesn't match specified rule (${config.rule})`,


"specified" seems redundant in this message. I think you can remove it without losing any meaning. What do you think?

Agreed. Will remove.

lencioni · 2017-06-26T18:26:40Z

lib/rules/boolean-prop-naming.js

+    const propTypeNames = config.propTypeNames || ['bool'];
+
+    // Remembers all Flowtype object definitions
+    var ObjectTypeAnnonations = {};


typo? ObjectTypeAnnotations?

Also, since this isn't instantiated, it should probably start with a lowercase letter.

And it might be a bit nicer to use a Map instead of a plain object.

+1 on all 3 points, nice catch

…op types

wmertens · 2019-07-15T10:27:56Z

How should you name a prop that enables some feature? Normally I'd write e.g. <Nav showHeaders />. Should this instead be <Nav hasHeaders />, <Nav isHeadered />, …?

EvHaus · 2019-07-15T14:07:36Z

@wmertens I would call it hasHeader. Keep in mind, this rule is optional and configurable. If you prefer to use showHeaders then you can add the "show" prefix to the rule's options.

EvHaus added the new rule label Jun 19, 2017

EvHaus requested review from ljharb, lencioni and yannickcr June 19, 2017 19:56

ljharb requested changes Jun 19, 2017

View reviewed changes

ljharb added the question label Jun 19, 2017

EvHaus force-pushed the boolean-prop-naming branch from ae2b074 to 82e35b8 Compare June 19, 2017 21:57

EvHaus force-pushed the boolean-prop-naming branch from 82e35b8 to b16fd19 Compare June 19, 2017 22:00

ljharb reviewed Jun 20, 2017

View reviewed changes

EvHaus force-pushed the boolean-prop-naming branch 2 times, most recently from c9d8647 to d76c2fb Compare June 20, 2017 16:23

ljharb reviewed Jun 20, 2017

View reviewed changes

EvHaus force-pushed the boolean-prop-naming branch 3 times, most recently from fe71deb to 38e479a Compare June 20, 2017 20:32

ljharb reviewed Jun 21, 2017

View reviewed changes

EvHaus force-pushed the boolean-prop-naming branch from 38e479a to 519dc2b Compare June 21, 2017 15:10

ljharb approved these changes Jun 23, 2017

View reviewed changes

lencioni approved these changes Jun 26, 2017

View reviewed changes

EvHaus force-pushed the boolean-prop-naming branch 2 times, most recently from d27f42e to f9c5fae Compare June 26, 2017 22:17

ljharb approved these changes Jun 26, 2017

View reviewed changes

New boolean-prop-naming rule for enforcing the naming of boolean pr…

da0affa

…op types

EvHaus force-pushed the boolean-prop-naming branch from f9c5fae to da0affa Compare June 26, 2017 22:59

lencioni merged commit 9f3b740 into jsx-eslint:master Jun 27, 2017

dependencies bot mentioned this pull request Aug 25, 2017

Update eslint-plugin-react from 7.1.0 to 7.3.0 chauncey-garrett/dotfiles#13

Open

renovate bot mentioned this pull request Feb 2, 2018

Update dependency eslint-plugin-react to v7 JimmyLv/Haiku#34

Open

1 task

alexzherdev mentioned this pull request Aug 13, 2018

rule proposal: force boolean props to be auxiliary verb's #970

Closed

renovate bot mentioned this pull request Sep 22, 2018

Update dependency eslint-plugin-react to v7 chintan9/react-d3#41

Closed

1 task

This was referenced Oct 25, 2018

Update dependency eslint-plugin-react to v7 efcsydney/efcsydney-roster#502

Merged

Update dependency eslint-plugin-react to v7 CaptainGlac1er/StudentHomeworkProject#38

Open

renovate bot mentioned this pull request Nov 7, 2018

chore(deps): update dependency eslint-plugin-react to v7 vgmtv/rsvg#12

Merged

renovate bot mentioned this pull request Nov 23, 2018

chore(deps): update dependency eslint-plugin-react to v7 JLHwung/tianzige#31

Open

1 task

renovate bot mentioned this pull request Dec 5, 2018

Update dependency eslint-plugin-react to v7 Automattic/jetpack#10827

Closed

1 task

mesosphere-web mentioned this pull request Dec 19, 2018

chore(deps): update dependency eslint-plugin-react to v7 mesosphere/reactjs-components#475

Closed

1 task

This was referenced Jan 8, 2019

Update dependency eslint-plugin-react to v7 SonyaMoisset/Team-Treehouse_Training#8

Closed

Update dependency eslint-plugin-react to v7 - autoclosed amytych/react-escape-outside#10

Merged


		### `rule`

		The Regex pattern to use when validating the name of the prop. Some common examples are:


		## About component detection

		For this rule to work we need to detect React components, this could be very hard since components could be declared in a lot of ways.


		### `rule`

		The RegExp pattern to use when validating the name of the prop. Some common examples are:

New boolean-prop-naming rule for enforcing the naming of booleans #1264

New boolean-prop-naming rule for enforcing the naming of booleans #1264

Conversation

EvHaus commented Jun 19, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvHaus commented Jun 19, 2017

Choose a reason for hiding this comment

EvHaus Jun 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

lencioni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvHaus Jun 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmertens commented Jul 15, 2019

EvHaus commented Jul 15, 2019

New `boolean-prop-naming` rule for enforcing the naming of booleans #1264

New `boolean-prop-naming` rule for enforcing the naming of booleans #1264

EvHaus Jun 20, 2017 •

edited

EvHaus Jun 26, 2017 •

edited