Skip to content

Commit f0d3e68

Browse files
ota-meshiota-meshi
authored
feat: add svelte/no-dom-manipulating rule (#302)
* feat: add `svelte/no-dom-manipulating` rule * Create thick-boxes-warn.md * chore: update doc
1 parent 1349cf7 commit f0d3e68

31 files changed

+675
-11
lines changed

‎.changeset/thick-boxes-warn.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"eslint-plugin-svelte": minor
3+
---
4+
5+
feat: add `svelte/no-dom-manipulating` rule

‎.stylelintignore

+1
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,4 @@ LICENSE
1414
*.md
1515
/docs-svelte-kit/
1616
/coverage
17+
/build

‎README.md

+1
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,7 @@ These rules relate to possible syntax or logic errors in Svelte code:
298298

299299
| Rule ID | Description | |
300300
|:--------|:------------|:---|
301+
| [svelte/no-dom-manipulating](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-dom-manipulating/) | disallow DOM manipulating | |
301302
| [svelte/no-dupe-else-if-blocks](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-dupe-else-if-blocks/) | disallow duplicate conditions in `{#if}` / `{:else if}` chains | :star: |
302303
| [svelte/no-dupe-style-properties](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-dupe-style-properties/) | disallow duplicate style properties | :star: |
303304
| [svelte/no-dynamic-slot-name](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-dynamic-slot-name/) | disallow dynamic slot name | :star::wrench: |

‎docs/rules.md

+1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ These rules relate to possible syntax or logic errors in Svelte code:
1616

1717
| Rule ID | Description | |
1818
|:--------|:------------|:---|
19+
| [svelte/no-dom-manipulating](./rules/no-dom-manipulating.md) | disallow DOM manipulating | |
1920
| [svelte/no-dupe-else-if-blocks](./rules/no-dupe-else-if-blocks.md) | disallow duplicate conditions in `{#if}` / `{:else if}` chains | :star: |
2021
| [svelte/no-dupe-style-properties](./rules/no-dupe-style-properties.md) | disallow duplicate style properties | :star: |
2122
| [svelte/no-dynamic-slot-name](./rules/no-dynamic-slot-name.md) | disallow dynamic slot name | :star::wrench: |

‎docs/rules/no-dom-manipulating.md

+109
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
---
2+
pageClass: "rule-details"
3+
sidebarDepth: 0
4+
title: "svelte/no-dom-manipulating"
5+
description: "disallow DOM manipulating"
6+
---
7+
8+
# svelte/no-dom-manipulating
9+
10+
> disallow DOM manipulating
11+
12+
- :exclamation: <badge text="This rule has not been released yet." vertical="middle" type="error"> **_This rule has not been released yet._** </badge>
13+
14+
## :book: Rule Details
15+
16+
In general, DOM manipulating should delegate to Svelte runtime. If you manipulate the DOM directly, the Svelte runtime may confuse because there is a difference between the actual DOM and the Svelte runtime's expected DOM.
17+
Therefore this rule reports where you use DOM manipulating function.
18+
We don't recommend but If you intentionally manipulate the DOM, simply you can ignore this ESLint report.
19+
20+
<ESLintCodeBlock>
21+
22+
<!--eslint-skip-->
23+
24+
```svelte
25+
<script>
26+
/* eslint svelte/no-dom-manipulating: "error" */
27+
let foo, bar, show
28+
29+
/* ✓ GOOD */
30+
const toggle = () => (show = !show)
31+
32+
/* ✗ BAD */
33+
const remove = () => foo.remove()
34+
const update = () => (bar.textContent = "Update!")
35+
</script>
36+
37+
{#if show}
38+
<div bind:this={foo}>Foo</div>
39+
{/if}
40+
<div bind:this={bar}>
41+
{#if show}
42+
Bar
43+
{/if}
44+
</div>
45+
46+
<button on:click={() => toggle()}>Click Me (Good)</button>
47+
<button on:click={() => remove()}>Click Me (Bad)</button>
48+
<button on:click={() => update()}>Click Me (Bad)</button>
49+
```
50+
51+
</ESLintCodeBlock>
52+
53+
This rule only tracks and checks variables given with `bind:this={}`. In other words, it doesn't track things like function arguments given to `transition:` directives. These functions have been well tested and are often used more carefully.
54+
55+
<ESLintCodeBlock>
56+
57+
<!--eslint-skip-->
58+
59+
```svelte
60+
<script>
61+
/* eslint svelte/no-dom-manipulating: "error" */
62+
let visible = false
63+
64+
function typewriter(node, { speed = 1 }) {
65+
const valid =
66+
node.childNodes.length === 1 &&
67+
node.childNodes[0].nodeType === Node.TEXT_NODE
68+
69+
if (!valid) {
70+
throw new Error(
71+
`This transition only works on elements with a single text node child`,
72+
)
73+
}
74+
75+
const text = node.textContent
76+
const duration = text.length / (speed * 0.01)
77+
78+
return {
79+
duration,
80+
tick: (t) => {
81+
const i = Math.trunc(text.length * t)
82+
node.textContent = text.slice(0, i) // It does not report.
83+
},
84+
}
85+
}
86+
</script>
87+
88+
<label>
89+
<input type="checkbox" bind:checked={visible} />
90+
visible
91+
</label>
92+
93+
{#if visible}
94+
<p transition:typewriter>The quick brown fox jumps over the lazy dog</p>
95+
{/if}
96+
```
97+
98+
</ESLintCodeBlock>
99+
100+
See also <https://svelte.jp/examples/custom-js-transitions>.
101+
102+
## :wrench: Options
103+
104+
Nothing.
105+
106+
## :mag: Implementation
107+
108+
- [Rule source](https://github.com/ota-meshi/eslint-plugin-svelte/blob/main/src/rules/no-dom-manipulating.ts)
109+
- [Test source](https://github.com/ota-meshi/eslint-plugin-svelte/blob/main/tests/src/rules/no-dom-manipulating.ts)

‎src/rules/no-dom-manipulating.ts

+131
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
import type { AST } from "svelte-eslint-parser"
2+
import type { TSESTree } from "@typescript-eslint/types"
3+
import { createRule } from "../utils"
4+
import { findVariable, getNodeName } from "../utils/ast-utils"
5+
import type { Variable } from "@typescript-eslint/scope-manager"
6+
import { getPropertyName } from "eslint-utils"
7+
8+
const DOM_MANIPULATING_METHODS = new Set([
9+
"appendChild", // https://developer.mozilla.org/en-US/docs/Web/API/Node/appendChild
10+
"insertBefore", // https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore
11+
"normalize", // https://developer.mozilla.org/en-US/docs/Web/API/Node/normalize
12+
"removeChild", // https://developer.mozilla.org/en-US/docs/Web/API/Node/removeChild
13+
"replaceChild", // https://developer.mozilla.org/en-US/docs/Web/API/Node/replaceChild
14+
"after", // https://developer.mozilla.org/en-US/docs/Web/API/Element/after
15+
"append", // https://developer.mozilla.org/en-US/docs/Web/API/Element/append
16+
"before", // https://developer.mozilla.org/en-US/docs/Web/API/Element/before
17+
"insertAdjacentElement", // https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentElement
18+
"insertAdjacentHTML", // https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentHTML
19+
"insertAdjacentText", // https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentText
20+
"prepend", // https://developer.mozilla.org/en-US/docs/Web/API/Element/prepend
21+
"remove", // https://developer.mozilla.org/en-US/docs/Web/API/Element/remove
22+
"replaceChildren", // https://developer.mozilla.org/en-US/docs/Web/API/Element/replaceChildren
23+
"replaceWith", // https://developer.mozilla.org/en-US/docs/Web/API/Element/replaceWith
24+
])
25+
const DOM_MANIPULATING_PROPERTIES = new Set([
26+
"textContent", // https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent
27+
"innerHTML", // https://developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML
28+
"outerHTML", // https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
29+
"innerText", // https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText
30+
"outerText", // https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/outerText
31+
])
32+
33+
export default createRule("no-dom-manipulating", {
34+
meta: {
35+
docs: {
36+
description: "disallow DOM manipulating",
37+
category: "Possible Errors",
38+
recommended: false,
39+
},
40+
schema: [],
41+
messages: {
42+
disallowManipulateDOM:
43+
"Don't manipulate the DOM directly. The Svelte runtime can get confused if there is a difference between the actual DOM and the DOM expected by the Svelte runtime.",
44+
},
45+
type: "problem",
46+
},
47+
create(context) {
48+
const domVariables = new Set<Variable>()
49+
50+
/**
51+
* Verify DOM variable identifier node
52+
*/
53+
function verifyIdentifier(
54+
node: TSESTree.Identifier | TSESTree.JSXIdentifier,
55+
) {
56+
const member = node.parent
57+
if (member?.type !== "MemberExpression" || member.object !== node) {
58+
return
59+
}
60+
const name = getPropertyName(member)
61+
if (!name) {
62+
return
63+
}
64+
let target: TSESTree.Expression = member
65+
let parent = target.parent
66+
while (parent?.type === "ChainExpression") {
67+
target = parent
68+
parent = parent.parent
69+
}
70+
if (!parent) {
71+
return
72+
}
73+
if (parent.type === "CallExpression") {
74+
if (parent.callee !== target || !DOM_MANIPULATING_METHODS.has(name)) {
75+
return
76+
}
77+
} else if (parent.type === "AssignmentExpression") {
78+
if (parent.left !== target || !DOM_MANIPULATING_PROPERTIES.has(name)) {
79+
return
80+
}
81+
}
82+
context.report({
83+
node: member,
84+
messageId: "disallowManipulateDOM",
85+
})
86+
}
87+
88+
return {
89+
"SvelteDirective[kind='Binding']"(node: AST.SvelteBindingDirective) {
90+
if (
91+
node.key.name.name !== "this" ||
92+
!node.expression ||
93+
node.expression.type !== "Identifier"
94+
) {
95+
// not bind:this={id}
96+
return
97+
}
98+
const element = node.parent.parent
99+
if (element.type !== "SvelteElement" || !isHTMLElement(element)) {
100+
// not HTML element
101+
return
102+
}
103+
const variable = findVariable(context, node.expression)
104+
if (
105+
!variable ||
106+
(variable.scope.type !== "module" && variable.scope.type !== "global")
107+
) {
108+
return
109+
}
110+
domVariables.add(variable)
111+
},
112+
"Program:exit"() {
113+
for (const variable of domVariables) {
114+
for (const reference of variable.references) {
115+
verifyIdentifier(reference.identifier)
116+
}
117+
}
118+
},
119+
}
120+
121+
/**
122+
* Checks whether the given node is a HTML element or not.
123+
*/
124+
function isHTMLElement(node: AST.SvelteElement) {
125+
return (
126+
node.kind === "html" ||
127+
(node.kind === "special" && getNodeName(node) === "svelte:element")
128+
)
129+
}
130+
},
131+
})

‎src/utils/ast-utils.ts

+6-8
Original file line numberDiff line numberDiff line change
@@ -518,19 +518,17 @@ function getAttributeValueRangeTokens(
518518
* Returns name of SvelteElement
519519
*/
520520
export function getNodeName(node: SvAST.SvelteElement): string {
521-
if ("name" in node.name) {
521+
if (node.name.type === "Identifier" || node.name.type === "SvelteName") {
522522
return node.name.name
523523
}
524-
let object = ""
524+
const memberPath = [node.name.property.name]
525525
let currentObject = node.name.object
526-
while ("object" in currentObject) {
527-
object = `${currentObject.property.name}.${object}`
526+
while (currentObject.type === "SvelteMemberExpressionName") {
527+
memberPath.unshift(currentObject.property.name)
528528
currentObject = currentObject.object
529529
}
530-
if ("name" in currentObject) {
531-
object = `${currentObject.name}.${object}`
532-
}
533-
return object + node.name.property.name
530+
memberPath.unshift(currentObject.name)
531+
return memberPath.join(".")
534532
}
535533

536534
/**

‎src/utils/rules.ts

+2
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import maxAttributesPerLine from "../rules/max-attributes-per-line"
1212
import mustacheSpacing from "../rules/mustache-spacing"
1313
import noAtDebugTags from "../rules/no-at-debug-tags"
1414
import noAtHtmlTags from "../rules/no-at-html-tags"
15+
import noDomManipulating from "../rules/no-dom-manipulating"
1516
import noDupeElseIfBlocks from "../rules/no-dupe-else-if-blocks"
1617
import noDupeStyleProperties from "../rules/no-dupe-style-properties"
1718
import noDynamicSlotName from "../rules/no-dynamic-slot-name"
@@ -59,6 +60,7 @@ export const rules = [
5960
mustacheSpacing,
6061
noAtDebugTags,
6162
noAtHtmlTags,
63+
noDomManipulating,
6264
noDupeElseIfBlocks,
6365
noDupeStyleProperties,
6466
noDynamicSlotName,

‎tests/fixtures/rules/no-dom-manipulating/invalid/chain01-errors.yaml

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
- message:
2+
Don't manipulate the DOM directly. The Svelte runtime can get confused
3+
if there is a difference between the actual DOM and the DOM expected by the
4+
Svelte runtime.
5+
line: 5
6+
column: 5
7+
suggestions: null
8+
- message:
9+
Don't manipulate the DOM directly. The Svelte runtime can get confused
10+
if there is a difference between the actual DOM and the DOM expected by the
11+
Svelte runtime.
12+
line: 9
13+
column: 7
14+
suggestions: null

‎tests/fixtures/rules/no-dom-manipulating/invalid/chain01-input.svelte

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<script>
2+
let foo
3+
4+
const remove1 = () => {
5+
foo?.remove()
6+
}
7+
const remove2 = () => {
8+
// eslint-disable-next-line no-unsafe-optional-chaining -- ignore
9+
;(foo?.remove)()
10+
}
11+
</script>
12+
13+
<p bind:this={foo}>div</p>
14+
15+
<button on:click={() => remove1()}>Click Me</button>
16+
<button on:click={() => remove2()}>Click Me</button>

‎tests/fixtures/rules/no-dom-manipulating/invalid/remove-text01-errors.yaml

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
- message:
2+
Don't manipulate the DOM directly. The Svelte runtime can get confused
3+
if there is a difference between the actual DOM and the DOM expected by the
4+
Svelte runtime.
5+
line: 9
6+
column: 25
7+
suggestions: null

‎tests/fixtures/rules/no-dom-manipulating/invalid/remove-text01-input.svelte

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<script>
2+
let div
3+
let show
4+
5+
// ✓ GOOD
6+
const toggle = () => (show = !show)
7+
8+
// ✗ BAD
9+
const update = () => (div.textContent = "foo")
10+
</script>
11+
12+
<div bind:this={div}>
13+
{#if show}
14+
div
15+
{/if}
16+
</div>
17+
18+
<button on:click={() => toggle()}>Click Me (Good)</button>
19+
<button on:click={() => update()}>Click Me (Bad)</button>

‎tests/fixtures/rules/no-dom-manipulating/invalid/remove01-errors.yaml

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
- message:
2+
Don't manipulate the DOM directly. The Svelte runtime can get confused
3+
if there is a difference between the actual DOM and the DOM expected by the
4+
Svelte runtime.
5+
line: 9
6+
column: 24
7+
suggestions: null

‎tests/fixtures/rules/no-dom-manipulating/invalid/remove01-input.svelte

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<script>
2+
let div
3+
let show
4+
5+
// ✓ GOOD
6+
const toggle = () => (show = !show)
7+
8+
// ✗ BAD
9+
const remove = () => div.remove()
10+
</script>
11+
12+
{#if show}
13+
<div bind:this={div}>div</div>
14+
{/if}
15+
16+
<button on:click={() => toggle()}>Click Me (Good)</button>
17+
<button on:click={() => remove()}>Click Me (Bad)</button>

0 commit comments

Comments
 (0)