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.

Sign up for GitHub

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

Jump to bottom

Add composition api's computed function support to vue/no-side-effects-in-computed-properties close #1393 #1407

Merged
merged 4 commits into from Jan 21, 2021
Merged

Add composition api's computed function support to vue/no-side-effects-in-computed-properties close #1393 #1407

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
adaad90
Add composition api's computed function support to vue/no-side-effect…
sapphi-red Dec 30, 2020
f1cd89f
check targetBody
sapphi-red Jan 5, 2021
a057276
fix false negative with arr.reverse()
sapphi-red Jan 5, 2021
6fbcb73
only report variables declared inside setup
sapphi-red Jan 5, 2021
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
53 changes: 49 additions & 4 deletions docs/rules/no-side-effects-in-computed-properties.md
View file
Open in desktop
Expand Up @@ -2,20 +2,20 @@
pageClass: rule-details
sidebarDepth: 0
title: vue/no-side-effects-in-computed-properties
description: disallow side effects in computed properties
description: disallow side effects in computed properties and functions
since: v3.6.0
---
# vue/no-side-effects-in-computed-properties

> disallow side effects in computed properties
> disallow side effects in computed properties and functions

- :gear: This rule is included in all of `"plugin:vue/vue3-essential"`, `"plugin:vue/essential"`, `"plugin:vue/vue3-strongly-recommended"`, `"plugin:vue/strongly-recommended"`, `"plugin:vue/vue3-recommended"` and `"plugin:vue/recommended"`.

## :book: Rule Details

This rule is aimed at preventing the code which makes side effects in computed properties.
This rule is aimed at preventing the code which makes side effects in computed properties and functions.

It is considered a very bad practice to introduce side effects inside computed properties. It makes the code not predictable and hard to understand.
It is considered a very bad practice to introduce side effects inside computed properties and functions. It makes the code not predictable and hard to understand.

<eslint-code-block :rules="{'vue/no-side-effects-in-computed-properties': ['error']}">

Expand Down Expand Up @@ -58,6 +58,51 @@ export default {

</eslint-code-block>

<eslint-code-block :rules="{'vue/no-side-effects-in-computed-properties': ['error']}">

```vue
<script>
import {computed} from 'vue'
/* ✓ GOOD */
export default {
setup() {
const foo = useFoo()

const fullName = computed(() => `${foo.firstName} ${foo.lastName}`)
const reversedArray = computed(() => {
return foo.array.slice(0).reverse() // .slice makes a copy of the array, instead of mutating the orginal
})
}
}
</script>
```

</eslint-code-block>

<eslint-code-block :rules="{'vue/no-side-effects-in-computed-properties': ['error']}">

```vue
<script>
import {computed} from 'vue'
/* ✗ BAD */
export default {
setup() {
const foo = useFoo()

const fullName = computed(() => {
foo.firstName = 'lorem' // <- side effect
return `${foo.firstName} ${foo.lastName}`
})
const reversedArray = computed(() => {
return foo.array.reverse() // <- side effect - orginal array is being mutated
})
}
}
</script>
```

</eslint-code-block>

## :wrench: Options

Nothing.
Expand Down
170 changes: 123 additions & 47 deletions lib/rules/no-side-effects-in-computed-properties.js
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@
* @author Michał Sajnóg
*/
'use strict'

const { ReferenceTracker, findVariable } = require('eslint-utils')
const utils = require('../utils')

/**
Expand Down Expand Up @@ -31,6 +31,8 @@ module.exports = {
create(context) {
/** @type {Map<ObjectExpression, ComponentComputedProperty[]>} */
const computedPropertiesMap = new Map()
/** @type {Array<FunctionExpression | ArrowFunctionExpression>} */
const computedCallNodes = []

/**
* @typedef {object} ScopeStack
Expand All @@ -54,56 +56,130 @@ module.exports = {
scopeStack = scopeStack && scopeStack.upper
}

return utils.defineVueVisitor(context, {
onVueObjectEnter(node) {
computedPropertiesMap.set(node, utils.getComputedProperties(node))
},
':function': onFunctionEnter,
':function:exit': onFunctionExit,

/**
* @param {(Identifier | ThisExpression) & {parent: MemberExpression}} node
* @param {VueObjectData} data
*/
'MemberExpression > :matches(Identifier, ThisExpression)'(
node,
{ node: vueNode }
) {
if (!scopeStack) {
return
}
const targetBody = scopeStack.body
const computedProperty = /** @type {ComponentComputedProperty[]} */ (computedPropertiesMap.get(
vueNode
)).find((cp) => {
return (
cp.value &&
node.loc.start.line >= cp.value.loc.start.line &&
node.loc.end.line <= cp.value.loc.end.line &&
targetBody === cp.value
)
})
if (!computedProperty) {
return
}
return Object.assign(
{
Program() {
const tracker = new ReferenceTracker(context.getScope())
const traceMap = utils.createCompositionApiTraceMap({
[ReferenceTracker.ESM]: true,
computed: {
[ReferenceTracker.CALL]: true
}
})

if (!utils.isThis(node, context)) {
return
}
const mem = node.parent
if (mem.object !== node) {
return
for (const { node } of tracker.iterateEsmReferences(traceMap)) {
if (node.type !== 'CallExpression') {
continue
}

const getterBody = utils.getGetterBodyFromComputedFunction(node)
if (getterBody) {
computedCallNodes.push(getterBody)
}
}
}
},
utils.defineVueVisitor(context, {
onVueObjectEnter(node) {
computedPropertiesMap.set(node, utils.getComputedProperties(node))
},
':function': onFunctionEnter,
':function:exit': onFunctionExit,

/**
* @param {(Identifier | ThisExpression) & {parent: MemberExpression}} node
* @param {VueObjectData} data
*/
'MemberExpression > :matches(Identifier, ThisExpression)'(
node,
{ node: vueNode }
) {
if (!scopeStack) {
return
}
const targetBody = scopeStack.body

const invalid = utils.findMutating(mem)
if (invalid) {
context.report({
node: invalid.node,
message: 'Unexpected side effect in "{{key}}" computed property.',
data: { key: computedProperty.key || 'Unknown' }
const computedProperty = /** @type {ComponentComputedProperty[]} */ (computedPropertiesMap.get(
vueNode
)).find((cp) => {
return (
cp.value &&
node.loc.start.line >= cp.value.loc.start.line &&
node.loc.end.line <= cp.value.loc.end.line &&
targetBody === cp.value
)
})
if (computedProperty) {
if (!utils.isThis(node, context)) {
return
}
const mem = node.parent
if (mem.object !== node) {
return
}

const invalid = utils.findMutating(mem)
if (invalid) {
context.report({
node: invalid.node,
message:
'Unexpected side effect in "{{key}}" computed property.',
data: { key: computedProperty.key || 'Unknown' }
})
}
return
}

// ignore `this` for computed functions
if (node.type === 'ThisExpression') {
ota-meshi marked this conversation as resolved.
Show resolved Hide resolved
return
}

const computedFunction = computedCallNodes.find(
(c) =>
node.loc.start.line >= c.loc.start.line &&
node.loc.end.line <= c.loc.end.line
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved
)
if (!computedFunction) {
return
}

const mem = node.parent
if (mem.object !== node) {
return
}

const variable = findVariable(context.getScope(), node)
if (variable) {
if (variable.defs.length !== 1) {
return
}

const def = variable.defs[0]
if (
Copy link
Member

@ota-meshi ota-meshi Jan 5, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to report only the variables defined in the setup() function scope. There can be many false negatives, but I think there are many false positives in the current process.

e.g.

const myUtils = { ... } // My utilities
export default {
  setup(props) {
    const array = reactive([])
    const foo = computed(() => myUtils.reverse(array)) // OK Build an array in reverse order.
    const bar = computed(() => array.reverse()) // NG
    const baz = computed(() => myUtils.reverse(props.array)) // OK Build an array in reverse order.
    const qux = computed(() => props.array.reverse()) // NG
  }
}

Copy link
Contributor Author

@sapphi-red sapphi-red Jan 5, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem only lies with the CallExpressions.
I changed the code to only report variables which are declared inside setup functions.
But maybe it is possible to report mutations that are not by CallExpressions even if it is declared outside.

const myUtils = {}

myUtils.reverse() // this is ok, since it is unknown whether it is destructive
myUtils = null // this is a mutation
myUtils.count++ // this is a mutation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main purpose of this rule is to prevent a reactive loop due to mutation. Therefore, I don't think variables that are unlikely to be reactive should be reported.
I think it is possible to add options to check for other mutations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thank you for answering!

def.type !== 'ImplicitGlobalVariable' &&
def.type !== 'TDZ' &&
def.type !== 'ImportBinding'
) {
if (
def.node.loc.start.line >= computedFunction.loc.start.line &&
def.node.loc.end.line <= computedFunction.loc.end.line
) {
// mutating local variables are accepted
return
}
}
}

const invalid = utils.findMutating(mem)
if (invalid) {
context.report({
node: invalid.node,
message: 'Unexpected side effect in computed function.'
})
}
}
}
})
})
)
}
}