From 4b8b7e8ea87331f4910132fc5148d206289fcbac Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:07:44 -0400 Subject: [PATCH 1/2] Add no-modifier-without-element-usage rule Reports modifiers that never reference their element, for both `modifier()` callbacks and class modifiers (`modify(element)` or `this.element`). Any reference counts as usage, including destructuring and passing the element along. Not in `recommended`; that is proposed for v14 in emberjs/rfcs#1217. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 1 + .../no-modifier-without-element-usage.md | 101 ++++++ .../no-modifier-without-element-usage.js | 202 +++++++++++ .../no-modifier-without-element-usage.js | 317 ++++++++++++++++++ 4 files changed, 621 insertions(+) create mode 100644 docs/rules/no-modifier-without-element-usage.md create mode 100644 lib/rules/no-modifier-without-element-usage.js create mode 100644 tests/lib/rules/no-modifier-without-element-usage.js diff --git a/README.md b/README.md index a62ffd2983..2354e1f034 100644 --- a/README.md +++ b/README.md @@ -297,6 +297,7 @@ To disable a rule for an entire `.gjs`/`.gts` file, use a regular ESLint file-le | Name                                             | Description | 💼 | 🔧 | 💡 | | :----------------------------------------------------------------------------------------------------------------- | :----------------------------------------------------------------------------------- | :- | :- | :- | +| [no-modifier-without-element-usage](docs/rules/no-modifier-without-element-usage.md) | disallow modifiers that never use their element | | | | | [template-builtin-component-arguments](docs/rules/template-builtin-component-arguments.md) | disallow setting certain attributes on builtin components | 📋 | | | | [template-no-action-modifiers](docs/rules/template-no-action-modifiers.md) | disallow usage of {{action}} modifiers | | 🔧 | | | [template-no-action-on-submit-button](docs/rules/template-no-action-on-submit-button.md) | disallow action attribute on submit buttons | 📋 | | | diff --git a/docs/rules/no-modifier-without-element-usage.md b/docs/rules/no-modifier-without-element-usage.md new file mode 100644 index 0000000000..1148a60812 --- /dev/null +++ b/docs/rules/no-modifier-without-element-usage.md @@ -0,0 +1,101 @@ +# ember/no-modifier-without-element-usage + + + +Disallow modifiers that never use their element. + +A modifier exists to give an element behavior that only the DOM node can provide: event listeners, focus, measurement, or handing the node to a third-party library. A modifier that ignores its element is an effect keyed to render timing, which brings back the problems of `{{did-insert}}` and `{{did-update}}`: extra renders, render loops, and behavior that no longer lives next to the data it depends on. + +## Rule Details + +This rule reports a modifier whose element is never referenced. + +Both modifier styles are checked: + +- function modifiers created with `modifier()` from `ember-modifier`, where the element is the first parameter of the callback +- class modifiers extending the `ember-modifier` default export or `ClassBasedModifier`, where the element is the first parameter of `modify()` or, in the legacy hook API, `this.element` + +Any reference counts as usage, including destructuring the element and passing it to another function. A modifier with no element parameter at all is reported. + +## Examples + +Examples of **incorrect** code for this rule: + +```js +import { modifier } from 'ember-modifier'; + +// The element is never used +modifier((element, positional) => { + trackEvent(positional[0]); +}); +``` + +```js +import { modifier } from 'ember-modifier'; + +// No element parameter at all +modifier(() => { + trackEvent('rendered'); +}); +``` + +```js +import Modifier from 'ember-modifier'; + +// A class modifier that ignores its element +export default class Track extends Modifier { + modify(element, [name]) { + trackEvent(name); + } +} +``` + +Examples of **correct** code for this rule: + +```js +import { modifier } from 'ember-modifier'; + +modifier((element) => { + element.focus(); +}); +``` + +```js +import { modifier } from 'ember-modifier'; + +// Passing the element along counts as usage +modifier((element, positional) => { + const chart = new Chart(element, positional[0]); + + return () => chart.destroy(); +}); +``` + +```js +import Modifier from 'ember-modifier'; + +export default class Track extends Modifier { + modify(element, [name]) { + element.dataset.trackedAs = name; + } +} +``` + +## Migration + +A modifier that does not use its element usually wants one of these instead: + +- derived state, so the value is computed where it is read rather than pushed on render +- a resource, for behavior with setup and teardown that is not tied to an element +- an event handler on the element that already triggers the behavior + +## Related Rules + +- [no-at-ember-render-modifiers](no-at-ember-render-modifiers.md) +- [template-no-at-ember-render-modifiers](template-no-at-ember-render-modifiers.md) +- [no-modifier-argument-destructuring](no-modifier-argument-destructuring.md) + +## References + +- [ember-modifier](https://github.com/ember-modifier/ember-modifier) +- [Ember Autotracking](https://guides.emberjs.com/release/in-depth-topics/autotracking-in-depth/) diff --git a/lib/rules/no-modifier-without-element-usage.js b/lib/rules/no-modifier-without-element-usage.js new file mode 100644 index 0000000000..6665552b38 --- /dev/null +++ b/lib/rules/no-modifier-without-element-usage.js @@ -0,0 +1,202 @@ +'use strict'; + +const { getImportIdentifier } = require('../utils/import'); + +const ERROR_MESSAGE = + 'This modifier never uses its element. Modifiers exist to add behavior to an element, so element-free logic belongs somewhere else.'; + +/** + * Unwraps a parameter to the node that binds a name, so that `...element` and + * `element = fallback` are treated the same as `element`. + */ +function unwrapParam(param) { + if (!param) { + return null; + } + + if (param.type === 'RestElement') { + return unwrapParam(param.argument); + } + + if (param.type === 'AssignmentPattern') { + return unwrapParam(param.left); + } + + return param; +} + +function isFunction(node) { + return ( + Boolean(node) && + (node.type === 'ArrowFunctionExpression' || + node.type === 'FunctionExpression' || + node.type === 'FunctionDeclaration') + ); +} + +/** + * A destructuring pattern reads the element to build its bindings, so it counts + * as usage without any reference to look up. + */ +function isElementUsed(sourceCode, fnNode) { + const param = unwrapParam(fnNode.params[0]); + + if (!param) { + return false; + } + + if (param.type !== 'Identifier') { + return true; + } + + const variable = sourceCode + .getDeclaredVariables(fnNode) + .find((candidate) => candidate.defs.some((def) => def.name === param)); + + return Boolean(variable) && variable.references.length > 0; +} + +function isThisElement(node) { + if (node.object.type !== 'ThisExpression') { + return false; + } + + return node.computed + ? node.property.type === 'Literal' && node.property.value === 'element' + : node.property.type === 'Identifier' && node.property.name === 'element'; +} + +function isModifyMember(node) { + return ( + (node.type === 'MethodDefinition' || node.type === 'PropertyDefinition') && + !node.static && + !node.computed && + node.key.type === 'Identifier' && + node.key.name === 'modify' + ); +} + +function getModifyFunction(modifyMember) { + if (modifyMember.type === 'MethodDefinition') { + return modifyMember.value; + } + + return isFunction(modifyMember.value) ? modifyMember.value : null; +} + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + type: 'suggestion', + docs: { + description: 'disallow modifiers that never use their element', + category: 'Best Practices', + recommended: false, + url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-modifier-without-element-usage.md', + }, + fixable: null, + schema: [], + messages: { + main: ERROR_MESSAGE, + }, + }, + + ERROR_MESSAGE, + + create(context) { + const { sourceCode } = context; + + let functionModifierName; + const classModifierNames = new Set(); + const classStack = []; + + function enterClass(node) { + const isModifier = + node.superClass?.type === 'Identifier' && classModifierNames.has(node.superClass.name); + const modifyMember = isModifier ? node.body.body.find(isModifyMember) : undefined; + + classStack.push({ + node, + isModifier, + modifyMember, + modifyFn: modifyMember && getModifyFunction(modifyMember), + usesThisElement: false, + }); + } + + function exitClass() { + const { node, isModifier, modifyMember, modifyFn, usesThisElement } = classStack.pop(); + + if (!isModifier || usesThisElement) { + return; + } + + // `modify` assigned from elsewhere cannot be checked here. + if (modifyMember && !modifyFn) { + return; + } + + if (modifyFn && isElementUsed(sourceCode, modifyFn)) { + return; + } + + context.report({ + node: modifyFn?.params[0] ?? modifyMember?.key ?? node.id ?? node.superClass, + messageId: 'main', + }); + } + + return { + ImportDeclaration(node) { + if (node.source.value !== 'ember-modifier') { + return; + } + + functionModifierName ??= getImportIdentifier(node, 'ember-modifier', 'modifier'); + + for (const name of [ + getImportIdentifier(node, 'ember-modifier'), + getImportIdentifier(node, 'ember-modifier', 'ClassBasedModifier'), + ]) { + if (name) { + classModifierNames.add(name); + } + } + }, + + CallExpression(node) { + if (!functionModifierName) { + return; + } + + if (node.callee.type !== 'Identifier' || node.callee.name !== functionModifierName) { + return; + } + + const callback = node.arguments[0]; + + if (!isFunction(callback) || isElementUsed(sourceCode, callback)) { + return; + } + + context.report({ + node: callback.params[0] ?? node.callee, + messageId: 'main', + }); + }, + + ClassDeclaration: enterClass, + ClassExpression: enterClass, + 'ClassDeclaration:exit': exitClass, + 'ClassExpression:exit': exitClass, + + MemberExpression(node) { + const classInfo = classStack.at(-1); + + if (classInfo?.isModifier && isThisElement(node)) { + classInfo.usesThisElement = true; + } + }, + }; + }, +}; diff --git a/tests/lib/rules/no-modifier-without-element-usage.js b/tests/lib/rules/no-modifier-without-element-usage.js new file mode 100644 index 0000000000..9ff017f130 --- /dev/null +++ b/tests/lib/rules/no-modifier-without-element-usage.js @@ -0,0 +1,317 @@ +const rule = require('../../../lib/rules/no-modifier-without-element-usage'); +const RuleTester = require('eslint').RuleTester; + +const { ERROR_MESSAGE } = rule; + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 2022, + sourceType: 'module', + }, +}); + +ruleTester.run('no-modifier-without-element-usage', rule, { + valid: [ + // Not importing from ember-modifier + ` + function modifier(fn) { return fn; } + modifier(() => {}); + `, + + // Function modifier using the element + ` + import { modifier } from 'ember-modifier'; + modifier((element) => { + element.focus(); + }); + `, + + // Element used inside a nested function + ` + import { modifier } from 'ember-modifier'; + modifier((element, positional) => { + registerCleanup(() => element.remove()); + }); + `, + + // Element passed along to another function + ` + import { modifier } from 'ember-modifier'; + modifier((element, positional) => { + setup(element, positional[0]); + }); + `, + + // Destructuring the element reads it + ` + import { modifier } from 'ember-modifier'; + modifier(({ dataset }) => { + console.log(dataset.id); + }); + `, + + // Renamed function modifier import + ` + import { modifier as createModifier } from 'ember-modifier'; + createModifier((element) => element.focus()); + `, + + // Function expression using the element + ` + import { modifier } from 'ember-modifier'; + modifier(function (element) { + element.focus(); + }); + `, + + // Callback declared elsewhere cannot be checked + ` + import { modifier } from 'ember-modifier'; + modifier(myCallback); + `, + + // A different export of ember-modifier + ` + import { something } from 'ember-modifier'; + something(() => {}); + `, + + // Class modifier using the element in modify() + ` + import Modifier from 'ember-modifier'; + export default class Autofocus extends Modifier { + modify(element) { + element.focus(); + } + } + `, + + // Class modifier using the element via a rest param + ` + import Modifier from 'ember-modifier'; + export default class Autofocus extends Modifier { + modify(...args) { + args[0].focus(); + } + } + `, + + // Class modifier using the element in a class property arrow function + ` + import Modifier from 'ember-modifier'; + export default class Autofocus extends Modifier { + modify = (element) => { + element.focus(); + }; + } + `, + + // Legacy class modifier using this.element + ` + import Modifier from 'ember-modifier'; + export default class Autofocus extends Modifier { + didInstall() { + this.element.focus(); + } + } + `, + + // ClassBasedModifier named import + ` + import { ClassBasedModifier } from 'ember-modifier'; + export default class Autofocus extends ClassBasedModifier { + modify(element) { + element.focus(); + } + } + `, + + // Not an ember-modifier class + ` + import Component from '@glimmer/component'; + export default class Foo extends Component { + modify() {} + } + `, + + // `modify` assigned from elsewhere cannot be checked + ` + import Modifier from 'ember-modifier'; + export default class Autofocus extends Modifier { + modify = someHelper; + } + `, + + // Element usage inside a nested class does not hide the outer usage + ` + import Modifier from 'ember-modifier'; + export default class Autofocus extends Modifier { + modify(element) { + class Inner { + run() { + return this.element; + } + } + element.append(new Inner().run()); + } + } + `, + ], + + invalid: [ + // Function modifier with no parameters + { + code: ` + import { modifier } from 'ember-modifier'; + modifier(() => { + doSomething(); + }); + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Function modifier that ignores the element + { + code: ` + import { modifier } from 'ember-modifier'; + modifier((element, positional) => { + doSomething(positional[0]); + }); + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Function expression that ignores the element + { + code: ` + import { modifier } from 'ember-modifier'; + modifier(function (element) { + doSomething(); + }); + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Renamed import + { + code: ` + import { modifier as createModifier } from 'ember-modifier'; + createModifier((element) => doSomething()); + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Shadowed element name inside the callback is not a reference to the param + { + code: ` + import { modifier } from 'ember-modifier'; + modifier((element) => { + const inner = (element) => element.focus(); + inner(document.body); + }); + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Class modifier whose modify() ignores the element + { + code: ` + import Modifier from 'ember-modifier'; + export default class Logger extends Modifier { + modify(element, positional) { + console.log(positional[0]); + } + } + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Class modifier whose modify() takes no parameters + { + code: ` + import Modifier from 'ember-modifier'; + export default class Logger extends Modifier { + modify() { + console.log('rendered'); + } + } + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Class property arrow function that ignores the element + { + code: ` + import Modifier from 'ember-modifier'; + export default class Logger extends Modifier { + modify = (element) => { + console.log('rendered'); + }; + } + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Class modifier with no modify() and no this.element + { + code: ` + import Modifier from 'ember-modifier'; + export default class Logger extends Modifier { + didInstall() { + console.log('installed'); + } + } + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // ClassBasedModifier named import + { + code: ` + import { ClassBasedModifier } from 'ember-modifier'; + export default class Logger extends ClassBasedModifier { + modify() {} + } + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Class expression + { + code: ` + import Modifier from 'ember-modifier'; + export default class extends Modifier { + modify() {} + } + `, + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }], + }, + + // Both a function modifier and a class modifier in one file + { + code: ` + import Modifier, { modifier } from 'ember-modifier'; + modifier(() => doSomething()); + export default class Logger extends Modifier { + modify() {} + } + `, + output: null, + errors: [ + { message: ERROR_MESSAGE, type: 'Identifier' }, + { message: ERROR_MESSAGE, type: 'Identifier' }, + ], + }, + ], +}); From e2d4314db6a157bbcb96dafd55a68b86fb350d6e Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Mon, 17 Aug 2026 15:47:15 -0400 Subject: [PATCH 2/2] Fix docs --- docs/rules/no-modifier-without-element-usage.md | 9 ++------- eslint.config.js | 1 + 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/docs/rules/no-modifier-without-element-usage.md b/docs/rules/no-modifier-without-element-usage.md index 1148a60812..4ee2544e44 100644 --- a/docs/rules/no-modifier-without-element-usage.md +++ b/docs/rules/no-modifier-without-element-usage.md @@ -4,18 +4,13 @@ Disallow modifiers that never use their element. -A modifier exists to give an element behavior that only the DOM node can provide: event listeners, focus, measurement, or handing the node to a third-party library. A modifier that ignores its element is an effect keyed to render timing, which brings back the problems of `{{did-insert}}` and `{{did-update}}`: extra renders, render loops, and behavior that no longer lives next to the data it depends on. +A modifier exists to give an element behavior that only the DOM node can provide: event listeners, focus, measurement, or handing the node to a third-party library. A modifier that ignores its element has historically caused infinite render loops, and lead to confusion. ## Rule Details This rule reports a modifier whose element is never referenced. -Both modifier styles are checked: - -- function modifiers created with `modifier()` from `ember-modifier`, where the element is the first parameter of the callback -- class modifiers extending the `ember-modifier` default export or `ClassBasedModifier`, where the element is the first parameter of `modify()` or, in the legacy hook API, `this.element` - -Any reference counts as usage, including destructuring the element and passing it to another function. A modifier with no element parameter at all is reported. +Both modifier function and class styles from `ember-modifier` are checked. ## Examples diff --git a/eslint.config.js b/eslint.config.js index 8950e757cd..a52c1ccceb 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -194,6 +194,7 @@ module.exports = [ 'n/no-unsupported-features/es-syntax': 'off', 'no-console': 'off', 'no-undef': 'off', + 'no-param-reassign': 'off', 'no-unused-expressions': 'off', 'no-unused-labels': 'off', 'no-unused-vars': 'off',