Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/eslint-plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ The same set in `.oxlintrc.json`:
"js-toolkit/prefer-instance-scheduler": "warn",
"js-toolkit/option-default-factory": "error",
"js-toolkit/no-conflicting-negated-option": "error",
"js-toolkit/no-destroy-lifecycle": "error",
"js-toolkit/no-deprecated-properties": ["error", { "version": "v4" }],
"js-toolkit/no-dispatch-event": "warn",
"js-toolkit/no-shadow-dom": "error",
Expand Down Expand Up @@ -201,6 +202,21 @@ These rules describe v4 only, and ship in `configs.v4` rather than in `configs.r
| `js-toolkit/option-default-factory` | Disallows a literal object or array as an option `default` — every instance would share it. Core warns at runtime, but only once a component mounts. | error | |
| `js-toolkit/no-conflicting-negated-option` | Disallows declaring both a boolean option `x` and an option named `noX`, which would make `data-option-no-x` mean two things. Core deliberately does not check this on every mount. | error | |
| `js-toolkit/no-deprecated-properties` | With `{ "version": "v4" }`: reports `$parent`, `$root`, `$children`, `$update`, `$warn`, `$log`, `$terminate`, `$services.enable()`/`.disable()`, `updated()`, `terminated()`, `config.emits`. | error | |
| `js-toolkit/no-destroy-lifecycle` | Renames `$destroy()` to `$unmount()` and the `destroyed()` hook to `unmounted()` — the v4 names. See below. | error | 🔧 |

#### `no-destroy-lifecycle`

v4 has no destroyed state. `$mount()` and `$unmount()` are one boolean with two values, and the method that used to be called `$destroy()` documented itself as the reversible inverse of `$mount()`. `$mount()`/`$unmount()` and `mounted()`/`unmounted()` are symmetric pairs where `$mount()`/`$destroy()` was not. Vue 3 made the same rename to `beforeDestroy`/`destroyed`.

The rename is not an entry in `no-deprecated-properties`, for three reasons. That rule only flags a member expression whose object is `this`, deliberately, so that `someLibrary.$parent` is not noise — but most `$destroy()` calls are `instance.$destroy()`, so folding the rename in would miss the common case, and lifting the guard for one entry would lift it for all of them. That rule is also not fixable and should not become so: `$parent` → `$closest()` is not a safe textual rewrite, while a rename is exactly the case a fixer fits. And the two rules say different things — `no-deprecated-properties` reports v3 names v4 dropped, whereas `$destroy` is v4's own name, renamed while v4 is unreleased.

The two halves are asymmetric, because the two names are.

`$destroy` is flagged on **any** receiver — `this.$destroy()`, `instance.$destroy()`, `super.$destroy()` — and on a `$destroy()` method definition, which is what a service mixin overriding the framework's teardown writes. The `$` prefix is the toolkit's own namespace, and the rule ships in `configs.v4` only; that opt-in is the guard against rewriting an unrelated library's API.

`destroyed` has no prefix to lean on — `emitter.destroyed`, `record.destroyed` and a plain `destroyed` boolean are ordinary code — so that half is narrowed to the two places where it can only be the hook: a non-static `destroyed()` method definition in a class that reads as a v4 component, and a `super.destroyed()` call inside one. A bare `foo.destroyed()` is left alone.

Computed access (`instance['$destroy']()`) is not flagged: it is rare, and the fixer would have to guess the quote style of a string it did not write.

#### `no-write-in-read-phase`

Expand Down
3 changes: 3 additions & 0 deletions packages/eslint-plugin/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
preferInstanceScheduler,
optionDefaultFactory,
noConflictingNegatedOption,
noDestroyLifecycle,
} from './rules/index.ts';

const PLUGIN_NAME = 'js-toolkit';
Expand Down Expand Up @@ -75,6 +76,7 @@ const rules = {
'prefer-instance-scheduler': preferInstanceScheduler,
'option-default-factory': optionDefaultFactory,
'no-conflicting-negated-option': noConflictingNegatedOption,
'no-destroy-lifecycle': noDestroyLifecycle,
};

const recommendedRules: Record<string, string> = {
Expand Down Expand Up @@ -124,6 +126,7 @@ const v4Rules: Record<string, unknown> = {
[`${PLUGIN_NAME}/prefer-instance-scheduler`]: 'warn',
[`${PLUGIN_NAME}/option-default-factory`]: 'error',
[`${PLUGIN_NAME}/no-conflicting-negated-option`]: 'error',
[`${PLUGIN_NAME}/no-destroy-lifecycle`]: 'error',
[`${PLUGIN_NAME}/no-deprecated-properties`]: ['error', { version: 'v4' }],
[`${PLUGIN_NAME}/no-dispatch-event`]: 'warn',
[`${PLUGIN_NAME}/no-shadow-dom`]: 'error',
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,4 @@ export { noOptionsAssignment } from './no-options-assignment.ts';
export { preferInstanceScheduler } from './prefer-instance-scheduler.ts';
export { optionDefaultFactory } from './option-default-factory.ts';
export { noConflictingNegatedOption } from './no-conflicting-negated-option.ts';
export { noDestroyLifecycle } from './no-destroy-lifecycle.ts';
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ describe('no-deprecated-properties', () => {
{
code: `class Slider extends Base {
mounted() { this.$watchChildren(Item, () => {}); }
destroyed() {}
unmounted() {}
static config = { name: 'Slider' };
}`,
options: v4,
Expand Down
4 changes: 2 additions & 2 deletions packages/eslint-plugin/src/rules/no-deprecated-properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ const V4_REMOVED = new Map([
// the component name and element, rather than writing to the console. See
// `$error` for the counterpart that carries a cause.
['$log', 'console.log()'],
['$terminate', '$destroy()'],
['$terminate', '$unmount()'],
]);

/** Methods v4 no longer calls. Defining one is dead code. */
const V4_REMOVED_METHODS = new Map([
['updated', '$watchChildren(), or an option<Name>Changed() hook'],
['terminated', 'destroyed()'],
['terminated', 'unmounted()'],
]);

/** `$services` survives, but its two switches do not. */
Expand Down
142 changes: 142 additions & 0 deletions packages/eslint-plugin/src/rules/no-destroy-lifecycle.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
import { describe, it } from 'vitest';
import { tester } from '../utils/rule-tester.ts';
import { noDestroyLifecycle } from './no-destroy-lifecycle.ts';

describe('no-destroy-lifecycle', () => {
it('passes and fails correctly', () => {
tester.run('no-destroy-lifecycle', noDestroyLifecycle as any, {
valid: [
// The v4 names themselves.
`class Slider extends Base {
static config = { name: 'Slider' };
unmounted() {}
}`,
`class Slider extends Base {
static config = { name: 'Slider' };
onClick() { this.$unmount(); }
}`,
`instance.$unmount();`,
// `destroyed` outside a component is an ordinary name.
`class Store { destroyed() {} }`,
`class Store extends Error { destroyed() {} }`,
`emitter.destroyed();`,
`if (record.destroyed) { retry(); }`,
`const { destroyed } = state;`,
// A static member is not the hook.
`class Slider extends Base {
static config = { name: 'Slider' };
static destroyed() {}
}`,
// Computed access is left alone: the fixer would have to guess the
// quote style of a string it did not write.
`instance['$destroy']();`,
],
invalid: [
// `this.$destroy()` — the shape `no-deprecated-properties` could reach.
{
code: `class Slider extends Base {
static config = { name: 'Slider' };
onClick() { this.$destroy(); }
}`,
errors: [{ messageId: 'renamedMethod' }],
output: `class Slider extends Base {
static config = { name: 'Slider' };
onClick() { this.$unmount(); }
}`,
},
// `instance.$destroy()` — the shape it could not, and the common one.
{
code: `const slider = new Slider(el);
slider.$destroy();`,
errors: [{ messageId: 'renamedMethod' }],
output: `const slider = new Slider(el);
slider.$unmount();`,
},
// Any receiver, including one reached through a chain.
{
code: `el[INSTANCES].get(name).$destroy();`,
errors: [{ messageId: 'renamedMethod' }],
output: `el[INSTANCES].get(name).$unmount();`,
},
// The hook definition.
{
code: `class Dialog extends Base {
static config = { name: 'Dialog' };
destroyed() {
this.close();
}
}`,
errors: [{ messageId: 'renamedHook' }],
output: `class Dialog extends Base {
static config = { name: 'Dialog' };
unmounted() {
this.close();
}
}`,
},
// The hook definition plus `super.destroyed()`, which
// `withScrolledInView` writes.
{
code: `class Animation extends withScroll(Base) {
static config = { name: 'Animation' };
destroyed() {
super.destroyed();
this.snap();
}
}`,
errors: [{ messageId: 'renamedHook' }, { messageId: 'renamedHook' }],
output: `class Animation extends withScroll(Base) {
static config = { name: 'Animation' };
unmounted() {
super.unmounted();
this.snap();
}
}`,
},
// `super.destroyed()` from the already-renamed hook.
{
code: `class Animation extends Base {
static config = { name: 'Animation' };
unmounted() { super.destroyed(); }
}`,
errors: [{ messageId: 'renamedHook' }],
output: `class Animation extends Base {
static config = { name: 'Animation' };
unmounted() { super.unmounted(); }
}`,
},
// A service mixin overrides the framework's own teardown.
{
code: `const withThing = (BaseClass) =>
class extends BaseClass {
$destroy() {
this.$services.scrolled.stop();
return super.$destroy();
}
};`,
errors: [{ messageId: 'renamedMethod' }, { messageId: 'renamedMethod' }],
output: `const withThing = (BaseClass) =>
class extends BaseClass {
$unmount() {
this.$services.scrolled.stop();
return super.$unmount();
}
};`,
},
// A component recognised through the framework surface rather than a
// `static config`.
{
code: `class Item extends AbstractItem {
mounted() { this.$el.hidden = false; }
destroyed() {}
}`,
errors: [{ messageId: 'renamedHook' }],
output: `class Item extends AbstractItem {
mounted() { this.$el.hidden = false; }
unmounted() {}
}`,
},
],
});
});
});
129 changes: 129 additions & 0 deletions packages/eslint-plugin/src/rules/no-destroy-lifecycle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import {
createRule,
findEnclosingClass,
getAncestors,
getKeyName,
isComponentClass,
type Node,
type RuleContext,
} from '../utils/ast.ts';

/**
* v4 renamed `$destroy()` to `$unmount()` and `destroyed()` to `unmounted()`.
*
* There is no destroyed state in v4: `#isMounted` is one boolean with two
* values, and the method documented itself as "the reversible inverse of
* `$mount()`". `$mount()`/`$unmount()` and `mounted()`/`unmounted()` are
* symmetric pairs where `$mount()`/`$destroy()` was not, which is the same
* rename Vue 3 made to `beforeDestroy`/`destroyed`.
*
* ## Why this is its own rule and not an entry in `no-deprecated-properties`
*
* Three reasons, and the first is the one that decides it.
*
* **`no-deprecated-properties` only flags a member expression whose object is
* `this`.** That guard is deliberate — `$parent` and `$root` are ordinary
* enough words that flagging `someLibrary.$parent` would be noise. But most
* `$destroy()` calls are `instance.$destroy()`: from a registry, from a test,
* from application code holding an instance. Folding the rename in would
* therefore miss the common case, and lifting the guard for one entry would
* lift it for every entry, which is exactly the false positive that guard
* exists to prevent.
*
* **`no-deprecated-properties` is not fixable, and should not become so.**
* `$parent` → `$closest()` and `$children` → `$watchChildren()` are not safe
* textual rewrites; they change the shape of the call and often the logic
* around it. A rename is the one case where a fixer is exactly right, and a
* rule that is fixable for one of its eleven reports is worse than two rules.
*
* **The two rules say different things.** `no-deprecated-properties` reports
* v3 names that v4 dropped. `$destroy` was never a v3-only name: it is v4's
* own, renamed while v4 is unreleased. This rule's whole lifetime is that
* rename window, and it is meant to be deleted, not grown.
*
* ## What is flagged, and what is not
*
* The two halves are deliberately asymmetric, because the two names are.
*
* `$destroy` is flagged on **any** receiver — `this.$destroy()`,
* `instance.$destroy()`, `super.$destroy()` — and on a `$destroy()` method
* definition, which is the shape a service mixin overriding the framework's
* teardown writes. The `$` prefix is the toolkit's own namespace, and the rule
* ships in `configs.v4` only, which a project turns on when it is on v4. That
* opt-in is the guard against rewriting an unrelated library's API.
*
* `destroyed` has no such prefix — `emitter.destroyed`, `record.destroyed`
* and a `destroyed` boolean are all ordinary code — so that half is narrowed
* to the two places where it can only mean the hook: a non-static
* `destroyed()` method definition in a class that reads as a v4 component,
* and a `super.destroyed()` call inside one. A plain `foo.destroyed()` call is
* left alone.
*
* Computed access (`instance['$destroy']()`) is not flagged. It is rare, and a
* fixer would have to guess the quote style of a string it did not write.
*/
export const noDestroyLifecycle = createRule({
meta: {
type: 'problem',
fixable: 'code',
docs: {
description:
'Require the v4 names `$unmount()` and `unmounted()` over `$destroy()` and `destroyed()`',
},
messages: {
renamedMethod: '`$destroy()` is named `$unmount()` in v4.',
renamedHook: 'The `destroyed()` hook is named `unmounted()` in v4.',
},
},
createOnce(context: RuleContext) {
function inComponent(node: Node): boolean {
const enclosing = findEnclosingClass(getAncestors(node, context));
return Boolean(enclosing) && isComponentClass(enclosing as Node);
}

function rename(target: Node, messageId: string, to: string) {
context.report({
node: target,
messageId,
fix: (fixer: any) => fixer.replaceText(target, to),
});
}

return {
MemberExpression(node: Node) {
if (node.computed) return;

const name = node.property?.name;

if (name === '$destroy') {
rename(node.property, 'renamedMethod', '$unmount');
return;
}

// `super.destroyed()` chains the hook of a mixin or an abstract
// component, so the receiver settles what a bare name cannot.
if (name === 'destroyed' && node.object?.type === 'Super' && inComponent(node)) {
rename(node.property, 'renamedHook', 'unmounted');
}
},

MethodDefinition(node: Node) {
if (node.computed || !node.key) return;

const name = getKeyName(node);

// A mixin overrides the framework's own teardown, which is why this is
// not restricted to a class that reads as a component.
if (name === '$destroy') {
rename(node.key, 'renamedMethod', '$unmount');
return;
}

if (name !== 'destroyed' || node.static === true) return;
if (!inComponent(node)) return;

rename(node.key, 'renamedHook', 'unmounted');
},
};
},
});
4 changes: 4 additions & 0 deletions packages/eslint-plugin/src/utils/ast.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,10 @@ const COMPONENT_MEMBERS = new Set([
'$read',
'$write',
'$mount',
'$unmount',
// v4 renamed `$destroy()` to `$unmount()`. The old name stays in this set
// while unmigrated code exists, so that `no-destroy-lifecycle` still reads a
// class whose only framework signal is the call it is there to rewrite.
'$destroy',
'$services',
]);
Expand Down
Loading
Loading