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.

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

Check against this usage in computed functions #1033

Merged
merged 1 commit into from
Dec 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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>