diff --git a/docs/rules/no-side-effects-in-computed-properties.md b/docs/rules/no-side-effects-in-computed-properties.md new file mode 100644 index 000000000..d908435f0 --- /dev/null +++ b/docs/rules/no-side-effects-in-computed-properties.md @@ -0,0 +1,41 @@ +# Do not introduce side effects in computed properties (no-side-effects-in-computed-properties) + +It is considered a very bad practice to introduce side effects inside computed properties. It makes the code not predictable and hard to understand. + + +## Rule Details + +Examples of **incorrect** code for this rule: + +```js + +export default { + computed: { + fullName() { + this.firstName = 'lorem' // <- side effect + return `${this.firstName} ${this.lastName}` + }, + somethingReversed() { + return this.something.reverse() // <- side effect + } + } +} + +``` + +Examples of **correct** code for this rule: + +```js + +export default { + computed: { + fullName() { + return `${this.firstName} ${this.lastName}` + }, + somethingReversed() { + return this.something.slice(0).reverse() + } + } +} + +``` diff --git a/lib/rules/no-side-effects-in-computed-properties.js b/lib/rules/no-side-effects-in-computed-properties.js new file mode 100644 index 000000000..968f7a814 --- /dev/null +++ b/lib/rules/no-side-effects-in-computed-properties.js @@ -0,0 +1,71 @@ +/** + * @fileoverview Don't introduce side effects in computed properties + * @author Michał Sajnóg + */ +'use strict' + +const utils = require('../utils') + +function create (context) { + const forbiddenNodes = [] + + return Object.assign({}, + { + // this.xxx <=|+=|-=> + 'AssignmentExpression > MemberExpression' (node) { + if (utils.parseMemberExpression(node)[0] === 'this') { + forbiddenNodes.push(node) + } + }, + // this.xxx <++|--> + 'UpdateExpression > MemberExpression' (node) { + if (utils.parseMemberExpression(node)[0] === 'this') { + forbiddenNodes.push(node) + } + }, + // this.xxx.func() + 'CallExpression' (node) { + const code = context.getSourceCode().getText(node) + const MUTATION_REGEX = /(this.)((?!(concat|slice|map|filter)\().)*((push|pop|shift|unshift|reverse|splice|sort|copyWithin|fill)\()/g + + if (MUTATION_REGEX.test(code)) { + forbiddenNodes.push(node) + } + } + }, + utils.executeOnVueComponent(context, (obj) => { + const computedProperties = utils.getComputedProperties(obj) + + computedProperties.forEach(cp => { + forbiddenNodes.forEach(node => { + if ( + node.loc.start.line >= cp.value.loc.start.line && + node.loc.end.line <= cp.value.loc.end.line + ) { + context.report({ + node: node, + message: `Unexpected side effect in "${cp.key}" computed property.` + }) + } + }) + }) + }) + ) +} + +// ------------------------------------------------------------------------------ +// Rule Definition +// ------------------------------------------------------------------------------ + +module.exports = { + create, + meta: { + docs: { + description: 'Don\'t introduce side effects in computed properties', + category: 'Best Practices', + recommended: false + }, + fixable: null, + schema: [] + } +} diff --git a/lib/rules/order-in-components.js b/lib/rules/order-in-components.js index 0c4d23bab..aed795106 100644 --- a/lib/rules/order-in-components.js +++ b/lib/rules/order-in-components.js @@ -4,6 +4,8 @@ */ 'use strict' +const utils = require('../utils') + const defaultOrder = [ ['name', 'delimiters', 'functional', 'model'], ['components', 'directives', 'filters'], @@ -36,38 +38,6 @@ const groups = { ] } -function isComponentFile (node, path) { - const isVueFile = path.endsWith('.vue') || path.endsWith('.jsx') - return isVueFile && node.declaration.type === 'ObjectExpression' -} - -function isVueComponent (node) { - const callee = node.callee - - const isFullVueComponent = node.type === 'CallExpression' && - callee.type === 'MemberExpression' && - callee.object.type === 'Identifier' && - callee.object.name === 'Vue' && - callee.property.type === 'Identifier' && - callee.property.name === 'component' && - node.arguments.length && - node.arguments.slice(-1)[0].type === 'ObjectExpression' - - const isDestructedVueComponent = callee.type === 'Identifier' && - callee.name === 'component' - - return isFullVueComponent || isDestructedVueComponent -} - -function isVueInstance (node) { - const callee = node.callee - return node.type === 'NewExpression' && - callee.type === 'Identifier' && - callee.name === 'Vue' && - node.arguments.length && - node.arguments[0].type === 'ObjectExpression' -} - function getOrderMap (order) { const orderMap = new Map() @@ -106,28 +76,13 @@ function checkOrder (propertiesNodes, orderMap, context) { function create (context) { const options = context.options[0] || {} const order = options.order || defaultOrder - const filePath = context.getFilename() const extendedOrder = order.map(property => groups[property] || property) const orderMap = getOrderMap(extendedOrder) - return { - ExportDefaultDeclaration (node) { - // export default {} in .vue || .jsx - if (!isComponentFile(node, filePath)) return - checkOrder(node.declaration.properties, orderMap, context) - }, - CallExpression (node) { - // Vue.component('xxx', {}) || component('xxx', {}) - if (!isVueComponent(node)) return - checkOrder(node.arguments.slice(-1)[0].properties, orderMap, context) - }, - NewExpression (node) { - // new Vue({}) - if (!isVueInstance(node)) return - checkOrder(node.arguments[0].properties, orderMap, context) - } - } + return utils.executeOnVueComponent(context, (obj) => { + checkOrder(obj.properties, orderMap, context) + }) } // ------------------------------------------------------------------------------ diff --git a/lib/utils/index.js b/lib/utils/index.js index cd01df744..4dd38d790 100644 --- a/lib/utils/index.js +++ b/lib/utils/index.js @@ -241,5 +241,148 @@ module.exports = { assert(typeof name === 'string') return VOID_ELEMENT_NAMES.has(name.toLowerCase()) + }, + + /** + * Parse member expression node to get array with all of it's parts + * @param {ASTNode} MemberExpression + * @returns {Array} + */ + parseMemberExpression (node) { + const members = [] + let memberExpression + + if (node.type === 'MemberExpression') { + memberExpression = node + + while (memberExpression.type === 'MemberExpression') { + if (memberExpression.property.type === 'Identifier') { + members.push(memberExpression.property.name) + } + memberExpression = memberExpression.object + } + + if (memberExpression.type === 'ThisExpression') { + members.push('this') + } else if (memberExpression.type === 'Identifier') { + members.push(memberExpression.name) + } + } + + return members.reverse() + }, + + /** + * Get all computed properties by looking at all component's properties + * @param {ObjectExpression} Object with component definition + * @return {Array} Array of computed properties in format: [{key: String, value: ASTNode}] + */ + getComputedProperties (componentObject) { + const computedPropertiesNode = componentObject.properties + .filter(p => + p.key.type === 'Identifier' && + p.key.name === 'computed' && + p.value.type === 'ObjectExpression' + )[0] + + if (!computedPropertiesNode) { return [] } + + return computedPropertiesNode.value.properties + .filter(cp => cp.type === 'Property') + .map(cp => { + const key = cp.key.name + let value + + if (cp.value.type === 'FunctionExpression') { + value = cp.value.body + } else if (cp.value.type === 'ObjectExpression') { + value = cp.value.properties + .filter(p => + p.key.type === 'Identifier' && + p.key.name === 'get' && + p.value.type === 'FunctionExpression' + ) + .map(p => p.value.body)[0] + } + + return { key, value } + }) + }, + + /** + * Check whether the given node is a Vue component based + * on the filename and default export type + * export default {} in .vue || .jsx + * @param {ASTNode} node Node to check + * @param {string} path File name with extension + * @returns {boolean} + */ + isVueComponentFile (node, path) { + const isVueFile = path.endsWith('.vue') || path.endsWith('.jsx') + return isVueFile && + node.type === 'ExportDefaultDeclaration' && + node.declaration.type === 'ObjectExpression' + }, + + /** + * Check whether given node is Vue component + * Vue.component('xxx', {}) || component('xxx', {}) + * @param {ASTNode} node Node to check + * @returns {boolean} + */ + isVueComponent (node) { + const callee = node.callee + + const isFullVueComponent = node.type === 'CallExpression' && + callee.type === 'MemberExpression' && + callee.object.type === 'Identifier' && + callee.object.name === 'Vue' && + callee.property.type === 'Identifier' && + callee.property.name === 'component' && + node.arguments.length && + node.arguments.slice(-1)[0].type === 'ObjectExpression' + + const isDestructedVueComponent = callee.type === 'Identifier' && + callee.name === 'component' + + return isFullVueComponent || isDestructedVueComponent + }, + + /** + * Check whether given node is new Vue instance + * new Vue({}) + * @param {ASTNode} node Node to check + * @returns {boolean} + */ + isVueInstance (node) { + const callee = node.callee + return node.type === 'NewExpression' && + callee.type === 'Identifier' && + callee.name === 'Vue' && + node.arguments.length && + node.arguments[0].type === 'ObjectExpression' + }, + + executeOnVueComponent (context, cb) { + const filePath = context.getFilename() + const _this = this + + return { + 'ExportDefaultDeclaration:exit' (node) { + // export default {} in .vue || .jsx + if (!_this.isVueComponentFile(node, filePath)) return + cb(node.declaration) + }, + 'CallExpression:exit' (node) { + // Vue.component('xxx', {}) || component('xxx', {}) + if (!_this.isVueComponent(node)) return + cb(node.arguments.slice(-1)[0]) + }, + 'NewExpression:exit' (node) { + // new Vue({}) + if (!_this.isVueInstance(node)) return + cb(node.arguments[0]) + } + } } } diff --git a/package.json b/package.json index f5d6c8d97..4bc869aa1 100644 --- a/package.json +++ b/package.json @@ -49,6 +49,8 @@ }, "devDependencies": { "@types/node": "^4.2.6", + "babel-eslint": "^7.2.3", + "chai": "^4.1.0", "eslint": "^3.18.0", "eslint-plugin-eslint-plugin": "^0.7.1", "eslint-plugin-vue-libs": "^1.2.0", diff --git a/tests/lib/rules/no-side-effects-in-computed-properties.js b/tests/lib/rules/no-side-effects-in-computed-properties.js new file mode 100644 index 000000000..5a58de081 --- /dev/null +++ b/tests/lib/rules/no-side-effects-in-computed-properties.js @@ -0,0 +1,200 @@ +/** + * @fileoverview Don't introduce side effects in computed properties + * @author Michał Sajnóg + */ +'use strict' + +// ------------------------------------------------------------------------------ +// Requirements +// ------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/no-side-effects-in-computed-properties') +const RuleTester = require('eslint').RuleTester + +const parserOptions = { + ecmaVersion: 6, + sourceType: 'module', + ecmaFeatures: { experimentalObjectRestSpread: true } +} + +// ------------------------------------------------------------------------------ +// Tests +// ------------------------------------------------------------------------------ + +const ruleTester = new RuleTester() +ruleTester.run('no-side-effects-in-computed-properties', rule, { + valid: [ + { + code: `Vue.component('test', { + computed: { + test1() { + return this.firstName + ' ' + this.lastName + }, + test2() { + return this.something.slice(0).reverse() + }, + test3() { + const example = this.something * 2 + return example + 'test' + }, + test4() { + return { + ...this.something, + test: 'example' + } + }, + test5: { + get() { + return this.firstName + ' ' + this.lastName + }, + set(newValue) { + const names = newValue.split(' ') + this.firstName = names[0] + this.lastName = names[names.length - 1] + } + }, + test6: { + get() { + return this.something.slice(0).reverse() + } + }, + test7: { + get() { + const example = this.something * 2 + return example + 'test' + } + }, + test8: { + get() { + return { + ...this.something, + test: 'example' + } + } + } + } + })`, + parserOptions + }, + { + code: `Vue.component('test', { + computed: { + ...mapGetters(['example']), + test1() { + const num = 0 + const something = { + a: 'val', + b: ['1', '2'] + } + num++ + something.a = 'something' + something.b.reverse() + return something.b + } + } + })`, + parserOptions + }, + { + code: `Vue.component('test', { + name: 'something', + data() { + return {} + } + })`, + parserOptions + } + ], + invalid: [ + { + code: `Vue.component('test', { + computed: { + test1() { + this.firstName = 'lorem' + asd.qwe.zxc = 'lorem' + return this.firstName + ' ' + this.lastName + }, + test2() { + this.count += 2; + this.count++; + return this.count; + }, + test3() { + return this.something.reverse() + }, + test4() { + const test = this.another.something.push('example') + return 'something' + }, + } + })`, + parserOptions, + errors: [{ + line: 4, + message: 'Unexpected side effect in "test1" computed property.' + }, { + line: 9, + message: 'Unexpected side effect in "test2" computed property.' + }, { + line: 10, + message: 'Unexpected side effect in "test2" computed property.' + }, { + line: 14, + message: 'Unexpected side effect in "test3" computed property.' + }, { + line: 17, + message: 'Unexpected side effect in "test4" computed property.' + }] + }, + { + code: `Vue.component('test', { + computed: { + test1: { + get() { + this.firstName = 'lorem' + return this.firstName + ' ' + this.lastName + } + }, + test2: { + get() { + this.count += 2; + this.count++; + return this.count; + } + }, + test3: { + get() { + return this.something.reverse() + } + }, + test4: { + get() { + const test = this.another.something.push('example') + return 'something' + }, + set(newValue) { + this.something = newValue + } + }, + } + })`, + parserOptions, + errors: [{ + line: 5, + message: 'Unexpected side effect in "test1" computed property.' + }, { + line: 11, + message: 'Unexpected side effect in "test2" computed property.' + }, { + line: 12, + message: 'Unexpected side effect in "test2" computed property.' + }, { + line: 18, + message: 'Unexpected side effect in "test3" computed property.' + }, { + line: 23, + message: 'Unexpected side effect in "test4" computed property.' + }] + } + ] +}) diff --git a/tests/lib/utils/index.js b/tests/lib/utils/index.js new file mode 100644 index 000000000..13293fb3a --- /dev/null +++ b/tests/lib/utils/index.js @@ -0,0 +1,123 @@ +'use strict' + +const babelEslint = require('babel-eslint') +const utils = require('../../../lib/utils/index') +const chai = require('chai') + +const assert = chai.assert + +describe('parseMemberExpression', () => { + let node + + const parse = function (code) { + return babelEslint.parse(code).body[0].expression + } + + it('should parse member expression', () => { + node = parse('this.some.nested.property') + assert.deepEqual( + utils.parseMemberExpression(node), + ['this', 'some', 'nested', 'property'] + ) + + node = parse('another.property') + assert.deepEqual( + utils.parseMemberExpression(node), + ['another', 'property'] + ) + + node = parse('this.something') + assert.deepEqual( + utils.parseMemberExpression(node), + ['this', 'something'] + ) + }) +}) + +describe('getComputedProperties', () => { + let node + + const parse = function (code) { + return babelEslint.parse(code).body[0].declarations[0].init + } + + it('should return empty array when there is no computed property', () => { + node = parse(`const test = { + name: 'test', + data() { + return {} + } + }`) + + assert.equal( + utils.getComputedProperties(node).length, + 0 + ) + }) + + it('should return computed properties', () => { + node = parse(`const test = { + name: 'test', + data() { + return {} + }, + computed: { + a: 'bad', + b: function () { + return 'b' + }, + c() { + return 'c' + }, + d: {}, + e: { + set(val) { + this.something = val + } + }, + f: { + get() { + return 'f' + } + } + } + }`) + + const computedProperties = utils.getComputedProperties(node) + + assert.equal( + computedProperties.length, + 6, + 'it detects all computed properties' + ) + + assert.notOk(computedProperties[0].value) + assert.ok(computedProperties[1].value) + assert.ok(computedProperties[2].value) + assert.notOk(computedProperties[3].value) + assert.notOk(computedProperties[4].value) + assert.ok(computedProperties[5].value) + }) + + it('should not collide with object spread operator', () => { + node = parse(`const test = { + name: 'test', + computed: { + ...mapGetters(['test']), + a() { + return 'a' + } + } + }`) + + const computedProperties = utils.getComputedProperties(node) + + assert.equal( + computedProperties.length, + 1, + 'it detects all computed properties' + ) + + assert.ok(computedProperties[0].value) + }) +})