Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add svelte/no-dom-manipulating rule #302

Merged
merged 4 commits into from
Nov 13, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/thick-boxes-warn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"eslint-plugin-svelte": minor
---

feat: add `svelte/no-dom-manipulating` rule
1 change: 1 addition & 0 deletions .stylelintignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ LICENSE
*.md
/docs-svelte-kit/
/coverage
/build
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ These rules relate to possible syntax or logic errors in Svelte code:

| Rule ID | Description | |
|:--------|:------------|:---|
| [svelte/no-dom-manipulating](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-dom-manipulating/) | disallow DOM manipulating | |
| [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: |
| [svelte/no-dupe-style-properties](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-dupe-style-properties/) | disallow duplicate style properties | :star: |
| [svelte/no-dynamic-slot-name](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-dynamic-slot-name/) | disallow dynamic slot name | :star::wrench: |
Expand Down
1 change: 1 addition & 0 deletions docs/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ These rules relate to possible syntax or logic errors in Svelte code:

| Rule ID | Description | |
|:--------|:------------|:---|
| [svelte/no-dom-manipulating](./rules/no-dom-manipulating.md) | disallow DOM manipulating | |
| [svelte/no-dupe-else-if-blocks](./rules/no-dupe-else-if-blocks.md) | disallow duplicate conditions in `{#if}` / `{:else if}` chains | :star: |
| [svelte/no-dupe-style-properties](./rules/no-dupe-style-properties.md) | disallow duplicate style properties | :star: |
| [svelte/no-dynamic-slot-name](./rules/no-dynamic-slot-name.md) | disallow dynamic slot name | :star::wrench: |
Expand Down
109 changes: 109 additions & 0 deletions docs/rules/no-dom-manipulating.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
---
pageClass: "rule-details"
sidebarDepth: 0
title: "svelte/no-dom-manipulating"
description: "disallow DOM manipulating"
---

# svelte/no-dom-manipulating

> disallow DOM manipulating

- :exclamation: <badge text="This rule has not been released yet." vertical="middle" type="error"> **_This rule has not been released yet._** </badge>

## :book: Rule Details

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.
Therefore this rule reports where you use DOM manipulating function.
We don't recommend but If you intentionally manipulate the DOM, simply you can ignore this ESLint report.

<ESLintCodeBlock>

<!--eslint-skip-->

```svelte
<script>
/* eslint svelte/no-dom-manipulating: "error" */
let foo, bar, show

/* ✓ GOOD */
const toggle = () => (show = !show)

/* ✗ BAD */
const remove = () => foo.remove()
const update = () => (bar.textContent = "Update!")
</script>

{#if show}
<div bind:this={foo}>Foo</div>
{/if}
<div bind:this={bar}>
{#if show}
Bar
{/if}
</div>

<button on:click={() => toggle()}>Click Me (Good)</button>
<button on:click={() => remove()}>Click Me (Bad)</button>
<button on:click={() => update()}>Click Me (Bad)</button>
```

</ESLintCodeBlock>

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.

<ESLintCodeBlock>

<!--eslint-skip-->

```svelte
<script>
/* eslint svelte/no-dom-manipulating: "error" */
let visible = false

function typewriter(node, { speed = 1 }) {
const valid =
node.childNodes.length === 1 &&
node.childNodes[0].nodeType === Node.TEXT_NODE

if (!valid) {
throw new Error(
`This transition only works on elements with a single text node child`,
)
}

const text = node.textContent
const duration = text.length / (speed * 0.01)

return {
duration,
tick: (t) => {
const i = Math.trunc(text.length * t)
node.textContent = text.slice(0, i) // It does not report.
},
}
}
</script>

<label>
<input type="checkbox" bind:checked={visible} />
visible
</label>

{#if visible}
<p transition:typewriter>The quick brown fox jumps over the lazy dog</p>
{/if}
```

</ESLintCodeBlock>

See also <https://svelte.jp/examples/custom-js-transitions>.

## :wrench: Options

Nothing.

## :mag: Implementation

- [Rule source](https://github.com/ota-meshi/eslint-plugin-svelte/blob/main/src/rules/no-dom-manipulating.ts)
- [Test source](https://github.com/ota-meshi/eslint-plugin-svelte/blob/main/tests/src/rules/no-dom-manipulating.ts)
131 changes: 131 additions & 0 deletions src/rules/no-dom-manipulating.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import type { AST } from "svelte-eslint-parser"
import type { TSESTree } from "@typescript-eslint/types"
import { createRule } from "../utils"
import { findVariable, getNodeName } from "../utils/ast-utils"
import type { Variable } from "@typescript-eslint/scope-manager"
import { getPropertyName } from "eslint-utils"

const DOM_MANIPULATING_METHODS = new Set([
"appendChild", // https://developer.mozilla.org/en-US/docs/Web/API/Node/appendChild
"insertBefore", // https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore
"normalize", // https://developer.mozilla.org/en-US/docs/Web/API/Node/normalize
"removeChild", // https://developer.mozilla.org/en-US/docs/Web/API/Node/removeChild
"replaceChild", // https://developer.mozilla.org/en-US/docs/Web/API/Node/replaceChild
"after", // https://developer.mozilla.org/en-US/docs/Web/API/Element/after
"append", // https://developer.mozilla.org/en-US/docs/Web/API/Element/append
"before", // https://developer.mozilla.org/en-US/docs/Web/API/Element/before
"insertAdjacentElement", // https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentElement
"insertAdjacentHTML", // https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentHTML
"insertAdjacentText", // https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentText
"prepend", // https://developer.mozilla.org/en-US/docs/Web/API/Element/prepend
"remove", // https://developer.mozilla.org/en-US/docs/Web/API/Element/remove
"replaceChildren", // https://developer.mozilla.org/en-US/docs/Web/API/Element/replaceChildren
"replaceWith", // https://developer.mozilla.org/en-US/docs/Web/API/Element/replaceWith
])
const DOM_MANIPULATING_PROPERTIES = new Set([
"textContent", // https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent
"innerHTML", // https://developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML
"outerHTML", // https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML
"innerText", // https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText
"outerText", // https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/outerText
])

export default createRule("no-dom-manipulating", {
meta: {
docs: {
description: "disallow DOM manipulating",
category: "Possible Errors",
recommended: false,
},
schema: [],
messages: {
disallowManipulateDOM:
"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.",
},
type: "problem",
},
create(context) {
const domVariables = new Set<Variable>()

/**
* Verify DOM variable identifier node
*/
function verifyIdentifier(
node: TSESTree.Identifier | TSESTree.JSXIdentifier,
) {
const member = node.parent
if (member?.type !== "MemberExpression" || member.object !== node) {
return
}
const name = getPropertyName(member)
if (!name) {
return
}
let target: TSESTree.Expression = member
let parent = target.parent
while (parent?.type === "ChainExpression") {
target = parent
parent = parent.parent
}
if (!parent) {
return
}
if (parent.type === "CallExpression") {
if (parent.callee !== target || !DOM_MANIPULATING_METHODS.has(name)) {
return
}
} else if (parent.type === "AssignmentExpression") {
if (parent.left !== target || !DOM_MANIPULATING_PROPERTIES.has(name)) {
return
}
}
context.report({
node: member,
messageId: "disallowManipulateDOM",
})
}

return {
"SvelteDirective[kind='Binding']"(node: AST.SvelteBindingDirective) {
if (
node.key.name.name !== "this" ||
!node.expression ||
node.expression.type !== "Identifier"
) {
// not bind:this={id}
return
}
const element = node.parent.parent
if (element.type !== "SvelteElement" || !isHTMLElement(element)) {
// not HTML element
return
}
const variable = findVariable(context, node.expression)
if (
!variable ||
(variable.scope.type !== "module" && variable.scope.type !== "global")
) {
return
}
domVariables.add(variable)
},
"Program:exit"() {
for (const variable of domVariables) {
for (const reference of variable.references) {
verifyIdentifier(reference.identifier)
}
}
},
}

/**
* Checks whether the given node is a HTML element or not.
*/
function isHTMLElement(node: AST.SvelteElement) {
return (
node.kind === "html" ||
(node.kind === "special" && getNodeName(node) === "svelte:element")
)
}
},
})
14 changes: 6 additions & 8 deletions src/utils/ast-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -518,19 +518,17 @@ function getAttributeValueRangeTokens(
* Returns name of SvelteElement
*/
export function getNodeName(node: SvAST.SvelteElement): string {
if ("name" in node.name) {
if (node.name.type === "Identifier" || node.name.type === "SvelteName") {
return node.name.name
}
let object = ""
const memberPath = [node.name.property.name]
let currentObject = node.name.object
while ("object" in currentObject) {
object = `${currentObject.property.name}.${object}`
while (currentObject.type === "SvelteMemberExpressionName") {
memberPath.unshift(currentObject.property.name)
currentObject = currentObject.object
}
if ("name" in currentObject) {
object = `${currentObject.name}.${object}`
}
return object + node.name.property.name
memberPath.unshift(currentObject.name)
return memberPath.join(".")
}

/**
Expand Down
2 changes: 2 additions & 0 deletions src/utils/rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import maxAttributesPerLine from "../rules/max-attributes-per-line"
import mustacheSpacing from "../rules/mustache-spacing"
import noAtDebugTags from "../rules/no-at-debug-tags"
import noAtHtmlTags from "../rules/no-at-html-tags"
import noDomManipulating from "../rules/no-dom-manipulating"
import noDupeElseIfBlocks from "../rules/no-dupe-else-if-blocks"
import noDupeStyleProperties from "../rules/no-dupe-style-properties"
import noDynamicSlotName from "../rules/no-dynamic-slot-name"
Expand Down Expand Up @@ -59,6 +60,7 @@ export const rules = [
mustacheSpacing,
noAtDebugTags,
noAtHtmlTags,
noDomManipulating,
noDupeElseIfBlocks,
noDupeStyleProperties,
noDynamicSlotName,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
- message:
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.
line: 5
column: 5
suggestions: null
- message:
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.
line: 9
column: 7
suggestions: null
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<script>
let foo

const remove1 = () => {
foo?.remove()
}
const remove2 = () => {
// eslint-disable-next-line no-unsafe-optional-chaining -- ignore
;(foo?.remove)()
}
</script>

<p bind:this={foo}>div</p>

<button on:click={() => remove1()}>Click Me</button>
<button on:click={() => remove2()}>Click Me</button>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
- message:
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.
line: 9
column: 25
suggestions: null
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<script>
let div
let show

// ✓ GOOD
const toggle = () => (show = !show)

// ✗ BAD
const update = () => (div.textContent = "foo")
</script>

<div bind:this={div}>
{#if show}
div
{/if}
</div>

<button on:click={() => toggle()}>Click Me (Good)</button>
<button on:click={() => update()}>Click Me (Bad)</button>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
- message:
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.
line: 9
column: 24
suggestions: null
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<script>
let div
let show

// ✓ GOOD
const toggle = () => (show = !show)

// ✗ BAD
const remove = () => div.remove()
</script>

{#if show}
<div bind:this={div}>div</div>
{/if}

<button on:click={() => toggle()}>Click Me (Good)</button>
<button on:click={() => remove()}>Click Me (Bad)</button>