Skip to content

Commit

Permalink
Check against this usage in computed functions
Browse files Browse the repository at this point in the history
  • Loading branch information
emilos committed Dec 19, 2017
1 parent c13b6ad commit d561c93
Show file tree
Hide file tree
Showing 9 changed files with 105 additions and 39 deletions.
8 changes: 8 additions & 0 deletions src/utils/isThisGetCallExpression.ts
Original file line number Diff line number Diff line change
@@ -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';
}
21 changes: 21 additions & 0 deletions src/utils/walkThroughTopFunctionScope.ts
Original file line number Diff line number Diff line change
@@ -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;
}
},
});
}
20 changes: 19 additions & 1 deletion src/validate/js/propValidators/computed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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(
Expand Down
55 changes: 18 additions & 37 deletions src/validate/js/propValidators/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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
}
}]
11 changes: 11 additions & 0 deletions test/validator/samples/computed-purify-check-no-this/input.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<button>{{foo}}</button>

<script>
export default {
computed: {
foo () {
this.set({ foo: true });
}
}
}
</script>
Original file line number Diff line number Diff line change
@@ -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
}
}]
11 changes: 11 additions & 0 deletions test/validator/samples/computed-purify-check-this-get/input.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{{foo}}

<script>
export default {
computed: {
foo () {
return this.get( 'bar' );
}
}
}
</script>
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@
}
}
}
</script>
</script>

0 comments on commit d561c93

Please sign in to comment.