Add new rule simple-modifiers

README.md

@@ -198,6 +198,7 @@ Each rule has emojis denoting:
 | [require-valid-alt-text](./docs/rule/require-valid-alt-text.md)                                           | ✅  |     | ⌨️  |     |
 | [require-valid-named-block-naming-format](./docs/rule/require-valid-named-block-naming-format.md)         | ✅  |     |     | 🔧  |
 | [self-closing-void-elements](./docs/rule/self-closing-void-elements.md)                                   |     | 💅  |     | 🔧  |
+| [simple-modifiers](./docs/rule/simple-modifiers.md)                                                       |     |     |     |     |
 | [simple-unless](./docs/rule/simple-unless.md)                                                             | ✅  |     |     | 🔧  |
 | [splat-attributes-only](./docs/rule/splat-attributes-only.md)                                             | ✅  |     |     |     |
 | [style-concatenation](./docs/rule/style-concatenation.md)                                                 | ✅  |     |     |     |

docs/rule/simple-modifiers.md

@@ -0,0 +1,34 @@
+# simple-modifiers
+
+This rule strongly advises against using the `{{modifier}}` helper with its first argument being either:
+
+- a string literal (.e.g. `{{modifier "tracking-interaction"}}`)
+- a variable in the template's JS backing class context (e.g. `{{modifier this.trackingInteraction}}`)
+
+A common issue this rule will catch is declaring the modifier's name conditionally like `{{(modifier (unless this.hasBeenClicked "track-interaction") "click" customizeData=this.customizeClickData)}}`. This is technically correct since the helper `modifier` simply ignores `null` and `undefined` values producing a no-op modifier but it is confusing. Ideally the we should instead do `{{(unless this.hasBeenClicked (modifier "track-interaction" "click" customizeData=this.customizeClickData))}}` where it's clear the modifier will not get called.
+
+## Examples
+
+This rule **forbids** the following:
+
+```hbs
+<div
+  {{(modifier
+    (unless this.hasBeenClicked 'track-interaction') 'click' customizeData=this.customizeClickData
+  )}}
+></div>
+```
+
+This rule **allows** the following:
+
+```hbs
+<div
+  {{(unless
+    this.hasBeenClicked (modifier 'track-interaction' 'click' customizeData=this.customizeClickData)
+  )}}
+></div>
+```
+
+## References
+
+- [Documentation](https://guides.emberjs.com/release/components/template-lifecycle-dom-and-modifiers/#toc_event-handlers) for modifiers

lib/rules/index.js

@@ -111,6 +111,7 @@ import requiresplattributes from './require-splattributes.js';
 import requirevalidalttext from './require-valid-alt-text.js';
 import requirevalidnamedblocknamingformat from './require-valid-named-block-naming-format.js';
 import selfclosingvoidelements from './self-closing-void-elements.js';
+import simplemodifiers from './simple-modifiers.js';
 import simpleunless from './simple-unless.js';
 import splatattributesonly from './splat-attributes-only.js';
 import styleconcatenation from './style-concatenation.js';
@@ -229,6 +230,7 @@ export default {
   'require-valid-alt-text': requirevalidalttext,
   'require-valid-named-block-naming-format': requirevalidnamedblocknamingformat,
   'self-closing-void-elements': selfclosingvoidelements,
+  'simple-modifiers': simplemodifiers,
   'simple-unless': simpleunless,
   'splat-attributes-only': splatattributesonly,
   'style-concatenation': styleconcatenation,

lib/rules/simple-modifiers.js

@@ -0,0 +1,36 @@
+import Rule from './_base.js';
+import { match } from '../helpers/node-matcher.js';
+
+export default class SimpleModifiers extends Rule {
+  visitor() {
+    return {
+      SubExpression(node) {
+        if (!this._isModifier(node)) {
+          return;
+        }
+
+        const firstModifierParam = node.params[0];
+        if (firstModifierParam) {
+          this._validateModifier(firstModifierParam);
+        }
+      },
+    };
+  }
+
+  _validateModifier(node) {
+    // First argument of the modifier must be a string
+    if (node.type === 'StringLiteral' || node.type === 'PathExpression') {
+      return;
+    }
+
+    this.log({
+      message:
+        'The modifier helper should have a string or a variable name containing the modifier name as a first argument.',
+      node,
+    });
+  }
+
+  _isModifier(node) {
+    return match(node.path, { original: 'modifier', type: 'PathExpression' });
+  }
+}

test/unit/rules/simple-modifiers-test.js

@@ -0,0 +1,41 @@
+import generateRuleTests from '../../helpers/rule-test-harness.js';
+
+generateRuleTests({
+  name: 'simple-modifiers',
+
+  config: true,
+
+  good: [
+    '<div {{(modifier "track-interaction" @controlName)}}></div>',
+    '<div {{(modifier this.trackInteraction @controlName)}}></div>',
+    '<div {{(modifier @trackInteraction @controlName)}}></div>',
+    '<div {{(if @isActionVisible (modifier "track-interaction" eventName=myEventName eventBody=myEventbody))}}></div>',
+    '<div {{(my-modifier (unless this.hasBeenClicked "track-interaction") "click" customizeData=this.customizeClickData)}}></div>',
+    '<MyComponent @people={{array "Tom Dale" "Yehuda Katz" this.myOtherPerson}} />',
+    '<div {{(if this.foo (modifier "foo-bar"))}}></div>',
+  ],
+
+  bad: [
+    {
+      template:
+        '<div {{(modifier (unless this.hasBeenClicked "track-interaction") "click" customizeData=this.customizeClickData)}}></div>',
+      verifyResults(results) {
+        expect(results).toMatchInlineSnapshot(`
+          [
+            {
+              "column": 17,
+              "endColumn": 65,
+              "endLine": 1,
+              "filePath": "layout.hbs",
+              "line": 1,
+              "message": "The modifier helper should have a string or a variable name containing the modifier name as a first argument.",
+              "rule": "simple-modifiers",
+              "severity": 2,
+              "source": "(unless this.hasBeenClicked \\"track-interaction\\")",
+            },
+          ]
+        `);
+      },
+    },
+  ],
+});