diff --git a/CHANGELOG.md b/CHANGELOG.md index 85ec35f07..9c34bb51b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format ## [Unreleased] +### Added + +- **ESLint Plugin:** add `prefer-destructured-lookups` rule — warns when `this.$refs`, `this.$options` or `this.$children` members are accessed more than once in the same method without being destructured ([#729](https://github.com/studiometa/js-toolkit/pull/729)) +- **ESLint Plugin:** add `no-dollar-prefix` rule — disallows user-defined instance methods and properties prefixed with `$` in `Base` subclasses, as `$` is reserved for framework members ([#729](https://github.com/studiometa/js-toolkit/pull/729)) +- **ESLint Plugin:** add `require-destroyed-cleanup` rule — requires a `destroyed()` method in `Base` subclasses that use `setTimeout`, `setInterval` or `requestAnimationFrame` to prevent memory leaks ([#729](https://github.com/studiometa/js-toolkit/pull/729)) + ## [v3.6.0-beta.2](https://github.com/studiometa/js-toolkit/compare/3.6.0-beta.1..3.6.0-beta.2) (2026-05-12) ### Added diff --git a/packages/docs/guide/going-further/linting.md b/packages/docs/guide/going-further/linting.md index 63d3ad36a..bf1f4a283 100644 --- a/packages/docs/guide/going-further/linting.md +++ b/packages/docs/guide/going-further/linting.md @@ -48,7 +48,10 @@ Add the plugin to your `.oxlintrc.json`: "js-toolkit/no-redundant-with-mount-when-in-view": "warn", "js-toolkit/no-manual-intersection-observer": "warn", "js-toolkit/no-manual-mutation-observer": "warn", - "js-toolkit/prefer-ref-over-query-selector": "warn" + "js-toolkit/prefer-ref-over-query-selector": "warn", + "js-toolkit/prefer-destructured-lookups": "warn", + "js-toolkit/no-dollar-prefix": "error", + "js-toolkit/require-destroyed-cleanup": "warn" } } ``` @@ -254,3 +257,70 @@ Disallows `new IntersectionObserver()` inside `Base` subclasses. Use `withInters **Recommended:** warn Disallows `new MutationObserver()` inside `Base` subclasses. Use the `withMutation` decorator instead. + +### Best practices + +#### `js-toolkit/prefer-destructured-lookups` + +**Recommended:** warn + +Warns when `this.$refs`, `this.$options` or `this.$children` members are accessed more than once in the same method body. Destructure them into local variables at the top of the method to avoid repeated lookups. + +```js +// ❌ Bad +async mounted() { + this.$refs.input.focus(); + this.$refs.input.value = ''; +} + +// ✅ Good +async mounted() { + const { input } = this.$refs; + input.focus(); + input.value = ''; +} +``` + +#### `js-toolkit/no-dollar-prefix` + +**Recommended:** error + +Disallows user-defined instance methods and properties prefixed with `$` in `Base` subclasses. The `$` prefix is reserved for framework-provided members (`$el`, `$refs`, `$emit`, `$options`, etc.). + +```js +// ❌ Bad +class Foo extends Base { + $helper() {} +} + +// ✅ Good +class Foo extends Base { + helper() {} +} +``` + +#### `js-toolkit/require-destroyed-cleanup` + +**Recommended:** warn + +Requires a `destroyed()` lifecycle method in `Base` subclasses that call `setTimeout`, `setInterval` or `requestAnimationFrame`. Failing to clean up timers when a component is destroyed can cause memory leaks and hard-to-debug bugs. + +```js +// ❌ Bad +class Foo extends Base { + async mounted() { + this._timer = setTimeout(() => {}, 1000); + } +} + +// ✅ Good +class Foo extends Base { + async mounted() { + this._timer = setTimeout(() => {}, 1000); + } + + async destroyed() { + clearTimeout(this._timer); + } +} +``` diff --git a/packages/eslint-plugin-js-toolkit/README.md b/packages/eslint-plugin-js-toolkit/README.md index cd06d7c3c..7a52f3b2c 100644 --- a/packages/eslint-plugin-js-toolkit/README.md +++ b/packages/eslint-plugin-js-toolkit/README.md @@ -49,7 +49,10 @@ Add the plugin to your `.oxlintrc.json`: "js-toolkit/components-pascal-case": "error", "js-toolkit/require-emit-declared-in-config": "error", "js-toolkit/require-children-declared-in-config": "error", - "js-toolkit/require-options-declared-in-config": "error" + "js-toolkit/require-options-declared-in-config": "error", + "js-toolkit/prefer-destructured-lookups": "warn", + "js-toolkit/no-dollar-prefix": "error", + "js-toolkit/require-destroyed-cleanup": "warn" } } ``` @@ -144,3 +147,11 @@ export default [ | `js-toolkit/refs-no-bracket-access` | Disallows bracket access with a `[]` suffix on `this.$refs` (e.g. `this.$refs['items[]']`). Rewrites to dot notation camelCase. | error | 🔧 | | `js-toolkit/prefer-ref-over-query-selector` | Warns when `this.$el.querySelector()` or `this.$el.querySelectorAll()` is used inside a `Base` subclass. Declare a ref in `static config` and use `this.$refs` instead. | warn | | | `js-toolkit/require-refs-declared-in-config` | Requires all `this.$refs.` accesses to be declared in `static config.refs`. | error | | + +### Best practices + +| Rule | Description | Recommended | Fixable | +| ---------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | ------- | +| `js-toolkit/prefer-destructured-lookups` | Warns when `this.$refs`, `this.$options` or `this.$children` members are accessed more than once in the same method. Destructure into a local variable instead. | warn | | +| `js-toolkit/no-dollar-prefix` | Disallows user-defined instance methods and properties prefixed with `$` in `Base` subclasses. The `$` prefix is reserved for framework-provided members. | error | | +| `js-toolkit/require-destroyed-cleanup` | Requires a `destroyed()` method in `Base` subclasses that call `setTimeout`, `setInterval` or `requestAnimationFrame`, to clear timers and prevent memory leaks. | warn | | diff --git a/packages/eslint-plugin-js-toolkit/src/index.ts b/packages/eslint-plugin-js-toolkit/src/index.ts index 6a8acf8a4..6645bf94e 100644 --- a/packages/eslint-plugin-js-toolkit/src/index.ts +++ b/packages/eslint-plugin-js-toolkit/src/index.ts @@ -26,6 +26,9 @@ import { requireEmitDeclaredInConfig, requireChildrenDeclaredInConfig, requireOptionsDeclaredInConfig, + preferDestructuredLookups, + noDollarPrefix, + requireDestroyedCleanup, } from './rules/index.ts'; const PLUGIN_NAME = 'js-toolkit'; @@ -57,6 +60,9 @@ const rules = { 'require-emit-declared-in-config': requireEmitDeclaredInConfig, 'require-children-declared-in-config': requireChildrenDeclaredInConfig, 'require-options-declared-in-config': requireOptionsDeclaredInConfig, + 'prefer-destructured-lookups': preferDestructuredLookups, + 'no-dollar-prefix': noDollarPrefix, + 'require-destroyed-cleanup': requireDestroyedCleanup, }; const recommendedRules: Record = { @@ -86,6 +92,9 @@ const recommendedRules: Record = { [`${PLUGIN_NAME}/require-emit-declared-in-config`]: 'error', [`${PLUGIN_NAME}/require-children-declared-in-config`]: 'error', [`${PLUGIN_NAME}/require-options-declared-in-config`]: 'error', + [`${PLUGIN_NAME}/prefer-destructured-lookups`]: 'warn', + [`${PLUGIN_NAME}/no-dollar-prefix`]: 'error', + [`${PLUGIN_NAME}/require-destroyed-cleanup`]: 'warn', }; const base = eslintCompatPlugin({ diff --git a/packages/eslint-plugin-js-toolkit/src/rules/index.ts b/packages/eslint-plugin-js-toolkit/src/rules/index.ts index beac8eec7..4604cad96 100644 --- a/packages/eslint-plugin-js-toolkit/src/rules/index.ts +++ b/packages/eslint-plugin-js-toolkit/src/rules/index.ts @@ -24,3 +24,6 @@ export { componentsPascalCase } from './components-pascal-case.ts'; export { requireEmitDeclaredInConfig } from './require-emit-declared-in-config.ts'; export { requireChildrenDeclaredInConfig } from './require-children-declared-in-config.ts'; export { requireOptionsDeclaredInConfig } from './require-options-declared-in-config.ts'; +export { preferDestructuredLookups } from './prefer-destructured-lookups.ts'; +export { noDollarPrefix } from './no-dollar-prefix.ts'; +export { requireDestroyedCleanup } from './require-destroyed-cleanup.ts'; diff --git a/packages/eslint-plugin-js-toolkit/src/rules/no-dollar-prefix.test.ts b/packages/eslint-plugin-js-toolkit/src/rules/no-dollar-prefix.test.ts new file mode 100644 index 000000000..b1d67e17d --- /dev/null +++ b/packages/eslint-plugin-js-toolkit/src/rules/no-dollar-prefix.test.ts @@ -0,0 +1,63 @@ +import { describe, it } from 'vitest'; +import { tester } from '../utils/rule-tester.ts'; +import { noDollarPrefix } from './no-dollar-prefix.ts'; + +describe('no-dollar-prefix', () => { + it('passes and fails correctly', () => { + tester.run('no-dollar-prefix', noDollarPrefix as any, { + valid: [ + // Not a Base subclass + `class Foo { $method() {} }`, + // Normal method names + `import { Base } from '@studiometa/js-toolkit'; + class Foo extends Base { + mounted() {} + myMethod() {} + myProperty = 'foo'; + }`, + // Static members are allowed (e.g. static config) + `import { Base } from '@studiometa/js-toolkit'; + class Foo extends Base { + static $config = {}; + }`, + // ClassExpression variant + `import { Base } from '@studiometa/js-toolkit'; + const Foo = class extends Base { + mounted() {} + };`, + ], + invalid: [ + { + code: `import { Base } from '@studiometa/js-toolkit'; +class Foo extends Base { + $myMethod() {} +}`, + errors: [{ messageId: 'noDollarPrefix' }], + }, + { + code: `import { Base } from '@studiometa/js-toolkit'; +class Foo extends Base { + $myProperty = 'foo'; +}`, + errors: [{ messageId: 'noDollarPrefix' }], + }, + { + code: `import { Base } from '@studiometa/js-toolkit'; +class Foo extends Base { + $onScroll() {} + $helper() {} +}`, + errors: [{ messageId: 'noDollarPrefix' }, { messageId: 'noDollarPrefix' }], + }, + // ClassExpression variant + { + code: `import { Base } from '@studiometa/js-toolkit'; +const Foo = class extends Base { + $myMethod() {} +};`, + errors: [{ messageId: 'noDollarPrefix' }], + }, + ], + }); + }); +}); diff --git a/packages/eslint-plugin-js-toolkit/src/rules/no-dollar-prefix.ts b/packages/eslint-plugin-js-toolkit/src/rules/no-dollar-prefix.ts new file mode 100644 index 000000000..99c8f5692 --- /dev/null +++ b/packages/eslint-plugin-js-toolkit/src/rules/no-dollar-prefix.ts @@ -0,0 +1,42 @@ +import { isBaseSubclass, type Node, type RuleContext, createRule } from '../utils/ast.ts'; + +export const noDollarPrefix = createRule({ + meta: { + type: 'problem', + docs: { + description: + 'Disallow user-defined methods and properties prefixed with "$" in Base subclasses — "$" is reserved for js-toolkit framework members', + }, + messages: { + noDollarPrefix: + '"${{name}}" uses the "$" prefix, which is reserved for js-toolkit framework members. Rename it without the "$" prefix.', + }, + }, + createOnce(context: RuleContext) { + return { + ClassDeclaration(node: Node) { + check(node, context); + }, + ClassExpression(node: Node) { + check(node, context); + }, + }; + }, +}); + +function check(node: Node, context: RuleContext) { + if (!isBaseSubclass(node, context)) return; + + for (const member of node.body?.body ?? []) { + if (member.static) continue; + if (member.type !== 'MethodDefinition' && member.type !== 'PropertyDefinition') continue; + const name: string = member.key?.name ?? ''; + if (name.startsWith('$')) { + context.report({ + node: member.key, + messageId: 'noDollarPrefix', + data: { name: name.slice(1) }, + }); + } + } +} diff --git a/packages/eslint-plugin-js-toolkit/src/rules/prefer-destructured-lookups.test.ts b/packages/eslint-plugin-js-toolkit/src/rules/prefer-destructured-lookups.test.ts new file mode 100644 index 000000000..f9a4e33ad --- /dev/null +++ b/packages/eslint-plugin-js-toolkit/src/rules/prefer-destructured-lookups.test.ts @@ -0,0 +1,67 @@ +import { describe, it } from 'vitest'; +import { tester } from '../utils/rule-tester.ts'; +import { preferDestructuredLookups } from './prefer-destructured-lookups.ts'; + +describe('prefer-destructured-lookups', () => { + it('passes and fails correctly', () => { + tester.run('prefer-destructured-lookups', preferDestructuredLookups as any, { + valid: [ + // Not a Base subclass + `class Foo { method() { this.$refs.btn; this.$refs.btn; } }`, + // Single access — no need to destructure + `import { Base } from '@studiometa/js-toolkit'; + class Foo extends Base { + mounted() { this.$refs.btn.focus(); } + }`, + // Already destructured + `import { Base } from '@studiometa/js-toolkit'; + class Foo extends Base { + mounted() { const { btn } = this.$refs; btn.focus(); btn.blur(); } + }`, + // Two different refs — each accessed once + `import { Base } from '@studiometa/js-toolkit'; + class Foo extends Base { + mounted() { this.$refs.btn.focus(); this.$refs.input.value = ''; } + }`, + // Lookup outside a method (e.g. in a property initializer) — no enclosing method, should not warn + `import { Base } from '@studiometa/js-toolkit'; + class Foo extends Base { + x = this.$refs.btn; + y = this.$refs.btn; + }`, + ], + invalid: [ + { + code: `import { Base } from '@studiometa/js-toolkit'; +class Foo extends Base { + mounted() { + this.$refs.input.focus(); + this.$refs.input.value = ''; + } +}`, + errors: [{ messageId: 'preferDestructured' }], + }, + { + code: `import { Base } from '@studiometa/js-toolkit'; +class Foo extends Base { + mounted() { + this.$options.foo; + this.$options.foo; + } +}`, + errors: [{ messageId: 'preferDestructured' }], + }, + { + code: `import { Base } from '@studiometa/js-toolkit'; +class Foo extends Base { + mounted() { + this.$children.modal.open(); + this.$children.modal.close(); + } +}`, + errors: [{ messageId: 'preferDestructured' }], + }, + ], + }); + }); +}); diff --git a/packages/eslint-plugin-js-toolkit/src/rules/prefer-destructured-lookups.ts b/packages/eslint-plugin-js-toolkit/src/rules/prefer-destructured-lookups.ts new file mode 100644 index 000000000..8799f94e2 --- /dev/null +++ b/packages/eslint-plugin-js-toolkit/src/rules/prefer-destructured-lookups.ts @@ -0,0 +1,88 @@ +import { + findEnclosingClass, + isBaseSubclass, + type Node, + type RuleContext, + createRule, +} from '../utils/ast.ts'; + +const TRACKED_PROPERTIES = new Set(['$refs', '$options', '$children']); + +function isTrackedLookup(node: Node): { prop: string; name: string } | null { + if ( + node.type === 'MemberExpression' && + node.object?.type === 'MemberExpression' && + node.object.object?.type === 'ThisExpression' && + node.object.property?.type === 'Identifier' && + TRACKED_PROPERTIES.has(node.object.property.name) && + node.property?.type === 'Identifier' && + !node.computed + ) { + return { prop: node.object.property.name, name: node.property.name }; + } + return null; +} + +function findEnclosingMethod(ancestors: Node[]): Node | null { + for (let i = ancestors.length - 1; i >= 0; i--) { + const node = ancestors[i]; + if (node.type === 'MethodDefinition') return node; + } + return null; +} + +export const preferDestructuredLookups = createRule({ + meta: { + type: 'suggestion', + docs: { + description: + 'Prefer destructuring this.$refs, this.$options and this.$children into local variables when accessed multiple times in the same method', + }, + messages: { + preferDestructured: + '"this.{{prop}}.{{name}}" is accessed {{count}} times. Destructure it into a local variable to avoid repeated lookups.', + }, + }, + createOnce(context: RuleContext) { + // methodKey -> lookup key -> list of nodes + const methodLookups = new Map>(); + + return { + MemberExpression(node: Node) { + const match = isTrackedLookup(node); + if (!match) return; + + const ancestors = + context.getAncestors?.() ?? context.sourceCode?.getAncestors?.(node) ?? []; + const method = findEnclosingMethod(ancestors); + if (!method) return; + + const cls = findEnclosingClass(ancestors); + if (!cls || !isBaseSubclass(cls, context)) return; + + if (!methodLookups.has(method)) methodLookups.set(method, new Map()); + const map = methodLookups.get(method)!; + const key = `${match.prop}.${match.name}`; + const list = map.get(key) ?? []; + list.push(node); + map.set(key, list); + }, + + 'MethodDefinition:exit'(node: Node) { + const map = methodLookups.get(node); + if (!map) return; + methodLookups.delete(node); + + for (const [key, nodes] of map) { + if (nodes.length < 2) continue; + const [prop, name] = key.split('.'); + context.report({ + node: nodes[0], + messageId: 'preferDestructured', + data: { prop, name, count: String(nodes.length) }, + }); + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin-js-toolkit/src/rules/require-destroyed-cleanup.test.ts b/packages/eslint-plugin-js-toolkit/src/rules/require-destroyed-cleanup.test.ts new file mode 100644 index 000000000..9bb854474 --- /dev/null +++ b/packages/eslint-plugin-js-toolkit/src/rules/require-destroyed-cleanup.test.ts @@ -0,0 +1,72 @@ +import { describe, it } from 'vitest'; +import { tester } from '../utils/rule-tester.ts'; +import { requireDestroyedCleanup } from './require-destroyed-cleanup.ts'; + +describe('require-destroyed-cleanup', () => { + it('passes and fails correctly', () => { + tester.run('require-destroyed-cleanup', requireDestroyedCleanup as any, { + valid: [ + // Not a Base subclass + `class Foo { mounted() { setTimeout(() => {}, 1000); } }`, + // No timers + `import { Base } from '@studiometa/js-toolkit'; + class Foo extends Base { + mounted() { this.$refs.btn.focus(); } + }`, + // Has timers and has destroyed() + `import { Base } from '@studiometa/js-toolkit'; + class Foo extends Base { + mounted() { this._timer = setTimeout(() => {}, 1000); } + destroyed() { clearTimeout(this._timer); } + }`, + `import { Base } from '@studiometa/js-toolkit'; + class Foo extends Base { + mounted() { this._raf = requestAnimationFrame(() => {}); } + destroyed() { cancelAnimationFrame(this._raf); } + }`, + // ClassExpression with timers and destroyed — should pass + `import { Base } from '@studiometa/js-toolkit'; + const Foo = class extends Base { + mounted() { this._timer = setTimeout(() => {}, 1000); } + destroyed() { clearTimeout(this._timer); } + };`, + // ClassExpression without timers — should pass + `import { Base } from '@studiometa/js-toolkit'; + const Foo = class extends Base { + mounted() { this.$refs.btn.focus(); } + };`, + ], + invalid: [ + { + code: `import { Base } from '@studiometa/js-toolkit'; +class Foo extends Base { + mounted() { this._timer = setTimeout(() => {}, 1000); } +}`, + errors: [{ messageId: 'requireDestroyed' }], + }, + { + code: `import { Base } from '@studiometa/js-toolkit'; +class Foo extends Base { + mounted() { this._interval = setInterval(() => {}, 500); } +}`, + errors: [{ messageId: 'requireDestroyed' }], + }, + { + code: `import { Base } from '@studiometa/js-toolkit'; +class Foo extends Base { + mounted() { this._raf = requestAnimationFrame(() => {}); } +}`, + errors: [{ messageId: 'requireDestroyed' }], + }, + // ClassExpression variant + { + code: `import { Base } from '@studiometa/js-toolkit'; +const Foo = class extends Base { + mounted() { this._timer = setTimeout(() => {}, 1000); } +};`, + errors: [{ messageId: 'requireDestroyed' }], + }, + ], + }); + }); +}); diff --git a/packages/eslint-plugin-js-toolkit/src/rules/require-destroyed-cleanup.ts b/packages/eslint-plugin-js-toolkit/src/rules/require-destroyed-cleanup.ts new file mode 100644 index 000000000..043906a22 --- /dev/null +++ b/packages/eslint-plugin-js-toolkit/src/rules/require-destroyed-cleanup.ts @@ -0,0 +1,96 @@ +import { isBaseSubclass, type Node, type RuleContext, createRule } from '../utils/ast.ts'; + +const SETUP_CALLS = new Set(['setTimeout', 'setInterval', 'requestAnimationFrame']); + +export const requireDestroyedCleanup = createRule({ + meta: { + type: 'suggestion', + docs: { + description: + 'Require a destroyed() method in Base subclasses that use setTimeout, setInterval or requestAnimationFrame', + }, + messages: { + requireDestroyed: + 'This component uses {{calls}} but has no destroyed() method. Add destroyed() to clear timers and avoid memory leaks.', + }, + }, + createOnce(context: RuleContext) { + // class node -> set of timer call names found + const classTimers = new Map>(); + // class node -> whether destroyed() exists + const classHasDestroyed = new Map(); + + return { + ClassDeclaration(node: Node) { + classTimers.set(node, new Set()); + classHasDestroyed.set(node, false); + }, + + ClassExpression(node: Node) { + classTimers.set(node, new Set()); + classHasDestroyed.set(node, false); + }, + + MethodDefinition(node: Node) { + if (node.key?.name === 'destroyed') { + // Find the enclosing class + const sourceCode = context.sourceCode ?? context.getSourceCode?.(); + const ancestors = context.getAncestors?.() ?? sourceCode?.getAncestors?.(node) ?? []; + for (let i = ancestors.length - 1; i >= 0; i--) { + const a = ancestors[i]; + if (a.type === 'ClassDeclaration' || a.type === 'ClassExpression') { + classHasDestroyed.set(a, true); + break; + } + } + } + }, + + CallExpression(node: Node) { + if (node.callee?.type !== 'Identifier') return; + if (!SETUP_CALLS.has(node.callee.name)) return; + + const sourceCode = context.sourceCode ?? context.getSourceCode?.(); + const ancestors = context.getAncestors?.() ?? sourceCode?.getAncestors?.(node) ?? []; + + for (let i = ancestors.length - 1; i >= 0; i--) { + const a = ancestors[i]; + if (a.type === 'ClassDeclaration' || a.type === 'ClassExpression') { + if (!isBaseSubclass(a, context)) break; + classTimers.get(a)?.add(node.callee.name); + break; + } + } + }, + + 'ClassDeclaration:exit'(node: Node) { + reportIfNeeded(node, context, classTimers, classHasDestroyed); + }, + + 'ClassExpression:exit'(node: Node) { + reportIfNeeded(node, context, classTimers, classHasDestroyed); + }, + }; + }, +}); + +function reportIfNeeded( + node: Node, + context: RuleContext, + classTimers: Map>, + classHasDestroyed: Map, +) { + const timers = classTimers.get(node); + const hasDestroyed = classHasDestroyed.get(node); + classTimers.delete(node); + classHasDestroyed.delete(node); + + if (!timers || timers.size === 0) return; + if (hasDestroyed) return; + + context.report({ + node: node.id ?? node, + messageId: 'requireDestroyed', + data: { calls: [...timers].join(', ') }, + }); +}