From d561c93841f51c89a0228804f91ae9ea8f536d82 Mon Sep 17 00:00:00 2001 From: Emil Ajdyna Date: Tue, 19 Dec 2017 02:53:21 +0100 Subject: [PATCH] Check against this usage in computed functions --- src/utils/isThisGetCallExpression.ts | 8 +++ src/utils/walkThroughTopFunctionScope.ts | 21 +++++++ src/validate/js/propValidators/computed.ts | 20 ++++++- src/validate/js/propValidators/helpers.ts | 55 ++++++------------- .../computed-purify-check-no-this/errors.json | 8 +++ .../computed-purify-check-no-this/input.html | 11 ++++ .../errors.json | 8 +++ .../computed-purify-check-this-get/input.html | 11 ++++ .../helper-purity-check-this-get/input.html | 2 +- 9 files changed, 105 insertions(+), 39 deletions(-) create mode 100644 src/utils/isThisGetCallExpression.ts create mode 100644 src/utils/walkThroughTopFunctionScope.ts create mode 100644 test/validator/samples/computed-purify-check-no-this/errors.json create mode 100644 test/validator/samples/computed-purify-check-no-this/input.html create mode 100644 test/validator/samples/computed-purify-check-this-get/errors.json create mode 100644 test/validator/samples/computed-purify-check-this-get/input.html diff --git a/src/utils/isThisGetCallExpression.ts b/src/utils/isThisGetCallExpression.ts new file mode 100644 index 000000000000..81642a389d86 --- /dev/null +++ b/src/utils/isThisGetCallExpression.ts @@ -0,0 +1,8 @@ +import { Node } from '../interfaces'; + +export default function isThisGetCallExpression(node: Node): boolean { + return node.type === 'CallExpression' && + node.callee.type === 'MemberExpression' && + node.callee.object.type === 'ThisExpression' && + node.callee.property.name === 'get'; +} diff --git a/src/utils/walkThroughTopFunctionScope.ts b/src/utils/walkThroughTopFunctionScope.ts new file mode 100644 index 000000000000..b773bf9a97ba --- /dev/null +++ b/src/utils/walkThroughTopFunctionScope.ts @@ -0,0 +1,21 @@ +import { Node } from '../interfaces'; +import { walk } from 'estree-walker'; + +export default function walkThroughTopFunctionScope(body: Node, callback: Function) { + let lexicalDepth = 0; + walk(body, { + enter(node: Node) { + if (/^Function/.test(node.type)) { + lexicalDepth += 1; + } else if (lexicalDepth === 0) { + callback(node) + } + }, + + leave(node: Node) { + if (/^Function/.test(node.type)) { + lexicalDepth -= 1; + } + }, + }); +} diff --git a/src/validate/js/propValidators/computed.ts b/src/validate/js/propValidators/computed.ts index a54d7856b0fb..a1b9afa93769 100644 --- a/src/validate/js/propValidators/computed.ts +++ b/src/validate/js/propValidators/computed.ts @@ -2,6 +2,8 @@ import checkForDupes from '../utils/checkForDupes'; import checkForComputedKeys from '../utils/checkForComputedKeys'; import { Validator } from '../../'; import { Node } from '../../../interfaces'; +import walkThroughTopFunctionScope from '../../../utils/walkThroughTopFunctionScope'; +import isThisGetCallExpression from '../../../utils/isThisGetCallExpression'; const isFunctionExpression = new Set([ 'FunctionExpression', @@ -27,7 +29,23 @@ export default function computed(validator: Validator, prop: Node) { ); } - const params = computation.value.params; + const { body, params } = computation.value; + + walkThroughTopFunctionScope(body, (node: Node) => { + if (isThisGetCallExpression(node) && !node.callee.property.computed) { + validator.error( + `Cannot use this.get(...) — it must be passed into the computed function as an argument`, + node.start + ); + } + + if (node.type === 'ThisExpression') { + validator.error( + `Computed should be pure functions — they do not have access to the component instance and cannot use 'this'. Did you mean to put this in 'methods'?`, + node.start + ); + } + }); if (params.length === 0) { validator.error( diff --git a/src/validate/js/propValidators/helpers.ts b/src/validate/js/propValidators/helpers.ts index 4345257e555b..d4960d9e1ebe 100644 --- a/src/validate/js/propValidators/helpers.ts +++ b/src/validate/js/propValidators/helpers.ts @@ -3,6 +3,8 @@ import checkForComputedKeys from '../utils/checkForComputedKeys'; import { walk } from 'estree-walker'; import { Validator } from '../../'; import { Node } from '../../../interfaces'; +import walkThroughTopFunctionScope from '../../../utils/walkThroughTopFunctionScope'; +import isThisGetCallExpression from '../../../utils/isThisGetCallExpression'; export default function helpers(validator: Validator, prop: Node) { if (prop.value.type !== 'ObjectExpression') { @@ -18,45 +20,24 @@ export default function helpers(validator: Validator, prop: Node) { prop.value.properties.forEach((prop: Node) => { if (!/FunctionExpression/.test(prop.value.type)) return; - let lexicalDepth = 0; let usesArguments = false; - walk(prop.value.body, { - enter(node: Node) { - if (/^Function/.test(node.type)) { - lexicalDepth += 1; - } else if (lexicalDepth === 0) { - // handle special case that's caused some people confusion — using `this.get(...)` instead of passing argument - // TODO do the same thing for computed values? - if ( - node.type === 'CallExpression' && - node.callee.type === 'MemberExpression' && - node.callee.object.type === 'ThisExpression' && - node.callee.property.name === 'get' && - !node.callee.property.computed - ) { - validator.error( - `Cannot use this.get(...) — it must be passed into the helper function as an argument`, - node.start - ); - } - - if (node.type === 'ThisExpression') { - validator.error( - `Helpers should be pure functions — they do not have access to the component instance and cannot use 'this'. Did you mean to put this in 'methods'?`, - node.start - ); - } else if (node.type === 'Identifier' && node.name === 'arguments') { - usesArguments = true; - } - } - }, - - leave(node: Node) { - if (/^Function/.test(node.type)) { - lexicalDepth -= 1; - } - }, + walkThroughTopFunctionScope(prop.value.body, (node: Node) => { + if (isThisGetCallExpression(node) && !node.callee.property.computed) { + validator.error( + `Cannot use this.get(...) — it must be passed into the helper function as an argument`, + node.start + ); + } + + if (node.type === 'ThisExpression') { + validator.error( + `Helpers should be pure functions — they do not have access to the component instance and cannot use 'this'. Did you mean to put this in 'methods'?`, + node.start + ); + } else if (node.type === 'Identifier' && node.name === 'arguments') { + usesArguments = true; + } }); if (prop.value.params.length === 0 && !usesArguments) { diff --git a/test/validator/samples/computed-purify-check-no-this/errors.json b/test/validator/samples/computed-purify-check-no-this/errors.json new file mode 100644 index 000000000000..e1a5e392defa --- /dev/null +++ b/test/validator/samples/computed-purify-check-no-this/errors.json @@ -0,0 +1,8 @@ +[{ + "message": "Computed should be pure functions — they do not have access to the component instance and cannot use 'this'. Did you mean to put this in 'methods'?", + "pos": 83, + "loc": { + "line": 7, + "column": 4 + } +}] diff --git a/test/validator/samples/computed-purify-check-no-this/input.html b/test/validator/samples/computed-purify-check-no-this/input.html new file mode 100644 index 000000000000..a7e4130e9e5a --- /dev/null +++ b/test/validator/samples/computed-purify-check-no-this/input.html @@ -0,0 +1,11 @@ + + + diff --git a/test/validator/samples/computed-purify-check-this-get/errors.json b/test/validator/samples/computed-purify-check-this-get/errors.json new file mode 100644 index 000000000000..f5cb473ed282 --- /dev/null +++ b/test/validator/samples/computed-purify-check-this-get/errors.json @@ -0,0 +1,8 @@ +[{ + "message": "Cannot use this.get(...) — it must be passed into the computed function as an argument", + "pos": 73, + "loc": { + "line": 7, + "column": 11 + } +}] diff --git a/test/validator/samples/computed-purify-check-this-get/input.html b/test/validator/samples/computed-purify-check-this-get/input.html new file mode 100644 index 000000000000..1188c94d9739 --- /dev/null +++ b/test/validator/samples/computed-purify-check-this-get/input.html @@ -0,0 +1,11 @@ +{{foo}} + + diff --git a/test/validator/samples/helper-purity-check-this-get/input.html b/test/validator/samples/helper-purity-check-this-get/input.html index e950152de876..8807be05f6dd 100644 --- a/test/validator/samples/helper-purity-check-this-get/input.html +++ b/test/validator/samples/helper-purity-check-this-get/input.html @@ -8,4 +8,4 @@ } } } - \ No newline at end of file +