From adaad908f887894cf47eff798030acb8c1f545bb Mon Sep 17 00:00:00 2001 From: sapphi-red Date: Wed, 30 Dec 2020 14:11:39 +0900 Subject: [PATCH 1/4] Add composition api's computed function support to vue/no-side-effects-in-computed-properties close #1393 --- .../no-side-effects-in-computed-properties.md | 53 +++- .../no-side-effects-in-computed-properties.js | 170 ++++++++---- .../no-side-effects-in-computed-properties.js | 250 ++++++++++++++++-- 3 files changed, 400 insertions(+), 73 deletions(-) diff --git a/docs/rules/no-side-effects-in-computed-properties.md b/docs/rules/no-side-effects-in-computed-properties.md index 7a3df6871..221be32d9 100644 --- a/docs/rules/no-side-effects-in-computed-properties.md +++ b/docs/rules/no-side-effects-in-computed-properties.md @@ -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. @@ -58,6 +58,51 @@ export default { + + +```vue + +``` + + + + + +```vue + +``` + + + ## :wrench: Options Nothing. diff --git a/lib/rules/no-side-effects-in-computed-properties.js b/lib/rules/no-side-effects-in-computed-properties.js index 4d5c79e49..28d5ec21a 100644 --- a/lib/rules/no-side-effects-in-computed-properties.js +++ b/lib/rules/no-side-effects-in-computed-properties.js @@ -3,7 +3,7 @@ * @author Michał Sajnóg */ 'use strict' - +const { ReferenceTracker, findVariable } = require('eslint-utils') const utils = require('../utils') /** @@ -31,6 +31,8 @@ module.exports = { create(context) { /** @type {Map} */ const computedPropertiesMap = new Map() + /** @type {Array} */ + const computedCallNodes = [] /** * @typedef {object} ScopeStack @@ -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') { + return + } + + const computedFunction = computedCallNodes.find( + (c) => + node.loc.start.line >= c.loc.start.line && + node.loc.end.line <= c.loc.end.line + ) + 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 ( + 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.' + }) + } } - } - }) + }) + ) } } diff --git a/tests/lib/rules/no-side-effects-in-computed-properties.js b/tests/lib/rules/no-side-effects-in-computed-properties.js index ae5ff7512..ee2cf4206 100644 --- a/tests/lib/rules/no-side-effects-in-computed-properties.js +++ b/tests/lib/rules/no-side-effects-in-computed-properties.js @@ -20,7 +20,11 @@ const parserOptions = { // Tests // ------------------------------------------------------------------------------ -const ruleTester = new RuleTester() +const ruleTester = new RuleTester({ + parser: require.resolve('vue-eslint-parser'), + parserOptions +}) + ruleTester.run('no-side-effects-in-computed-properties', rule, { valid: [ { @@ -101,8 +105,7 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { this.someArray.forEach(arr => console.log(arr)) } } - })`, - parserOptions + })` }, { code: `Vue.component('test', { @@ -120,8 +123,7 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { return something.b } } - })`, - parserOptions + })` }, { code: `Vue.component('test', { @@ -129,8 +131,7 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { data() { return {} } - })`, - parserOptions + })` }, { code: `Vue.component('test', { @@ -141,8 +142,7 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { return a }, } - })`, - parserOptions + })` }, { code: `Vue.component('test', { @@ -161,8 +161,7 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { } }, } - })`, - parserOptions + })` }, { code: `Vue.component('test', { @@ -171,15 +170,13 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { return this.something['a']().reverse() }, } - })`, - parserOptions + })` }, { code: `const test = { el: '#app' } Vue.component('test', { el: test.el - })`, - parserOptions + })` }, { code: `Vue.component('test', { @@ -188,8 +185,90 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { return [...this.items].reverse() }, } - })`, - parserOptions + })` + }, + { + filename: 'test.vue', + code: ` + ` } ], invalid: [ @@ -222,7 +301,6 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { } } })`, - parserOptions, errors: [ { line: 4, @@ -286,7 +364,6 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { }, } })`, - parserOptions, errors: [ { line: 5, @@ -321,7 +398,6 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { } }); `, - parserOptions, errors: [ { line: 5, @@ -341,7 +417,6 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { }, } })`, - parserOptions, errors: [ { line: 4, @@ -363,12 +438,143 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { }, } })`, - parserOptions, errors: [ 'Unexpected side effect in "test1" computed property.', 'Unexpected side effect in "test2" computed property.', 'Unexpected side effect in "test3" computed property.' ] + }, + { + filename: 'test.vue', + code: ` + + `, + errors: [ + { + line: 12, + message: 'Unexpected side effect in computed function.' + }, + { + line: 13, + message: 'Unexpected side effect in computed function.' + }, + { + line: 17, + message: 'Unexpected side effect in computed function.' + }, + { + line: 18, + message: 'Unexpected side effect in computed function.' + }, + { + line: 21, + message: 'Unexpected side effect in computed function.' + }, + { + line: 23, + message: 'Unexpected side effect in computed function.' + }, + { + line: 27, + message: 'Unexpected side effect in computed function.' + }, + { + line: 30, + message: 'Unexpected side effect in computed function.' + }, + { + line: 33, + message: 'Unexpected side effect in computed function.' + }, + { + line: 37, + message: 'Unexpected side effect in computed function.' + }, + { + line: 40, + message: 'Unexpected side effect in computed function.' + }, + { + line: 43, + message: 'Unexpected side effect in computed function.' + }, + { + line: 45, + message: 'Unexpected side effect in computed function.' + } + ] + }, + { + filename: 'test.vue', + code: ` + + `, + errors: [ + { + line: 8, + message: 'Unexpected side effect in computed function.' + } + ] } ] }) From f1cd89fdd2b48612d63651b0e12a8de7627e0e4e Mon Sep 17 00:00:00 2001 From: sapphi-red Date: Wed, 6 Jan 2021 02:08:51 +0900 Subject: [PATCH 2/4] check targetBody --- lib/rules/no-side-effects-in-computed-properties.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/rules/no-side-effects-in-computed-properties.js b/lib/rules/no-side-effects-in-computed-properties.js index 28d5ec21a..d73770e26 100644 --- a/lib/rules/no-side-effects-in-computed-properties.js +++ b/lib/rules/no-side-effects-in-computed-properties.js @@ -138,7 +138,8 @@ module.exports = { const computedFunction = computedCallNodes.find( (c) => node.loc.start.line >= c.loc.start.line && - node.loc.end.line <= c.loc.end.line + node.loc.end.line <= c.loc.end.line && + targetBody === c.body ) if (!computedFunction) { return From a0572763a018bf80e45712c207f75fee43691c1b Mon Sep 17 00:00:00 2001 From: sapphi-red Date: Wed, 6 Jan 2021 02:09:43 +0900 Subject: [PATCH 3/4] fix false negative with arr.reverse() --- .../no-side-effects-in-computed-properties.js | 2 +- .../no-side-effects-in-computed-properties.js | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/rules/no-side-effects-in-computed-properties.js b/lib/rules/no-side-effects-in-computed-properties.js index d73770e26..0380a129e 100644 --- a/lib/rules/no-side-effects-in-computed-properties.js +++ b/lib/rules/no-side-effects-in-computed-properties.js @@ -172,7 +172,7 @@ module.exports = { } } - const invalid = utils.findMutating(mem) + const invalid = utils.findMutating(node) if (invalid) { context.report({ node: invalid.node, diff --git a/tests/lib/rules/no-side-effects-in-computed-properties.js b/tests/lib/rules/no-side-effects-in-computed-properties.js index ee2cf4206..06d64ffe2 100644 --- a/tests/lib/rules/no-side-effects-in-computed-properties.js +++ b/tests/lib/rules/no-side-effects-in-computed-properties.js @@ -555,6 +555,27 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { } ] }, + { + filename: 'test.vue', + code: ` + + `, + errors: [ + { + line: 8, + message: 'Unexpected side effect in computed function.' + } + ] + }, { filename: 'test.vue', code: ` From 6fbcb734a5b4d30b3e3d802a0940affeffd6a399 Mon Sep 17 00:00:00 2001 From: sapphi-red Date: Wed, 6 Jan 2021 02:24:21 +0900 Subject: [PATCH 4/4] only report variables declared inside setup --- .../no-side-effects-in-computed-properties.js | 50 ++++++++++++------- .../no-side-effects-in-computed-properties.js | 13 ++--- 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/lib/rules/no-side-effects-in-computed-properties.js b/lib/rules/no-side-effects-in-computed-properties.js index 0380a129e..7c0639b95 100644 --- a/lib/rules/no-side-effects-in-computed-properties.js +++ b/lib/rules/no-side-effects-in-computed-properties.js @@ -33,6 +33,8 @@ module.exports = { const computedPropertiesMap = new Map() /** @type {Array} */ const computedCallNodes = [] + /** @type {Array} */ + const setupFunctions = [] /** * @typedef {object} ScopeStack @@ -85,6 +87,9 @@ module.exports = { }, ':function': onFunctionEnter, ':function:exit': onFunctionExit, + onSetupFunctionEnter(node) { + setupFunctions.push(node) + }, /** * @param {(Identifier | ThisExpression) & {parent: MemberExpression}} node @@ -151,25 +156,34 @@ module.exports = { } const variable = findVariable(context.getScope(), node) - if (variable) { - if (variable.defs.length !== 1) { - return - } + if (!variable || variable.defs.length !== 1) { + return + } - const def = variable.defs[0] - if ( - 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 def = variable.defs[0] + if ( + def.type === 'ImplicitGlobalVariable' || + def.type === 'TDZ' || + def.type === 'ImportBinding' + ) { + return + } + + const isDeclaredInsideSetup = setupFunctions.some( + (setupFn) => + def.node.loc.start.line >= setupFn.loc.start.line && + def.node.loc.end.line <= setupFn.loc.end.line + ) + if (!isDeclaredInsideSetup) { + return + } + + 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(node) diff --git a/tests/lib/rules/no-side-effects-in-computed-properties.js b/tests/lib/rules/no-side-effects-in-computed-properties.js index 06d64ffe2..647910d4a 100644 --- a/tests/lib/rules/no-side-effects-in-computed-properties.js +++ b/tests/lib/rules/no-side-effects-in-computed-properties.js @@ -192,6 +192,7 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { code: ` ` @@ -487,10 +489,7 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { const test9 = computed(() => { A.name = '' }) - const test10 = computed(() => { - console.log = () => {} - }) - const test11 = computed(() => (foo.a = '', true)) + const test10 = computed(() => (foo.a = '', true)) const test100 = computed(() => { const a = foo @@ -546,11 +545,7 @@ ruleTester.run('no-side-effects-in-computed-properties', rule, { message: 'Unexpected side effect in computed function.' }, { - line: 43, - message: 'Unexpected side effect in computed function.' - }, - { - line: 45, + line: 42, message: 'Unexpected side effect in computed function.' } ]