Add `no-async-in-computed-properties` rule #72

armano2 · 2017-07-08T13:58:40Z

This PR implements rules proposed in #53

DONE:

Create documtation
Add tests
async await
new Promise
.then .catch .finally
Promise.all() Promise.race() Promise.reject() Promise.resolve()
setTimeout() setInterval() setImmediate() requestAnimationFrame()

armano2 · 2017-07-13T23:56:48Z

@michalsnik There is one commit there with you as author (i took some of your changes) to align with it 🗡

I cleaned up commits from merges and all tests are passing now 🔢

michalsnik · 2017-07-14T06:39:31Z

Great @armano2, thank you so much for all the hard work! I'll review it in details later today, but so far it looks really good! :) I'm glad you figured out more cases, these all make perfect sense to me :)

michalsnik · 2017-07-16T17:59:50Z

docs/rules/no-async-in-computed-properties.md

@@ -0,0 +1,56 @@
+# Check if there are no asynchronous action inside computed properties (no-async-in-computed-properties)
+
+Vue.js has a basic construct which lets computed properties collects dependencies and refreshes them thats why its really important to not use any of asynchronous action inside of them.


I'd go with simple:

Computed properties should be synchronous. Asynchronous actions inside them may not work as expected and can lead to an unexpected behaviour, that's why you should avoid them.
If you need async computed properties you might want to consider using additional plugin https://github.com/foxbenjaminfox/vue-async-computed

michalsnik · 2017-07-16T18:02:46Z

docs/rules/no-async-in-computed-properties.md

+export default {
+  computed: {
+    pro () {
+      setTimeout(() => { }, 0)


Let's put each method causing errors in separate property

michalsnik · 2017-07-16T18:03:15Z

lib/rules/no-async-in-computed-properties.js

@@ -0,0 +1,178 @@
+/**
+ * @fileoverview Check if there are no async inside computed properties.


asynchronous actions

michalsnik · 2017-07-16T18:04:47Z

lib/rules/no-async-in-computed-properties.js

+
+function isTimedFunction (node) {
+  return (
+    node.callee.type === 'Identifier' &&


Add additional node.type === 'CallExpression'. It's good to always check the given node too, and not depend on implementation that is outside this function.

michalsnik · 2017-07-16T18:08:01Z

lib/rules/no-async-in-computed-properties.js

+    TIMED_FUNCTIONS.indexOf(node.callee.name) !== -1
+  ) || (
+    node.callee.type === 'MemberExpression' &&
+    node.callee.object.name === 'window' && (


There is a missing check: node.callee.object.type === 'Identifier' &&

michalsnik · 2017-07-16T18:16:09Z

lib/rules/no-async-in-computed-properties.js

+      node.callee.object.name === 'Promise' &&
+      PROMISE_METHODS.indexOf(node.callee.property.name) !== -1
+    ) || ( // somePromise.ANYTHING()
+      node.callee.object && isPromise(node.callee.object)


Ok this recursion is for checking chained promises, right?

@michalsnik there can be expression like

el.foo.bar.catch()

I don't think so @armano2, in your example it would call isPromise with memberExpression -> el.foo.bar which will automatically return false. What you're checking is:

something.crazy.then().test()

But I don't see a real case with this kind of node. I don't think you'll ever end up with custom method after promise method so I think we can even remove this last condition. Add more tests if you want, delete it and check if they all pass.

@michalsnik you are completely right :D
thank you for explanation

michalsnik · 2017-07-16T18:17:56Z

lib/rules/no-async-in-computed-properties.js

+    if (node.generator) {
+      forbiddenNodes.push({
+        node: node,
+        type: 'yield*'


I think we should call this type simply generator

michalsnik · 2017-07-16T18:22:45Z

lib/rules/no-async-in-computed-properties.js

+            el.node.loc.end.line <= cp.value.loc.end.line
+          ) {
+            let message = `Unexpected asynchronous action in "{{name}}" computed property.`
+            if (el.type === 'await') {


I see a lot of repetition here. Please think about creating a map with all types and corresponding names. Then return the message without repeating it :)

michalsnik · 2017-07-16T18:23:57Z

lib/rules/no-async-in-computed-properties.js

+  create,
+  meta: {
+    docs: {
+      description: 'Check if there are no async inside computed properties.',


asynchronous actions

michalsnik · 2017-07-16T18:28:55Z

tests/lib/rules/no-async-in-computed-properties.js

+      `,
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Unexpected yield* expression in "foo" computed property.',


generator function expression would be more correct

armano2 · 2017-07-16T18:48:21Z

Thank you for code review i adapted all requests into code except chained promises

michalsnik · 2017-07-16T20:48:44Z

docs/rules/no-async-in-computed-properties.md

@@ -0,0 +1,64 @@
+# Check if there are no asynchronous action inside computed properties (no-async-in-computed-properties)


michalsnik · 2017-07-16T20:57:02Z

lib/rules/no-async-in-computed-properties.js

+  const forbiddenNodes = []
+
+  const messages = {
+    promise: 'Unexpected asynchronous action in "{{name}}" computed property.',


I was thinking about:

const expressionTypes = { promise: 'asynchronous action', await: 'await operator', yield: 'yield keyword', generator: 'generator function expression', async: 'async function declaration', new: 'Promise object', timed: 'timed function' }

and then you could pass message as:

Unexpected {{expressionName}} in "{{propertyName}}" computed property.

with:

data: { propertyName: cp.key, expressionName: expressionTypes[cp.key] }

What do you think? That would prevent you from duplicating the message.

I was not sure if those messages are correct at all. I'm terrible at describing stuff 🥇
if they are going to stay as they are i have no objections

@michalsnik updated

michalsnik · 2017-07-16T21:46:07Z

I put do not merge label on all of your rules. And I'll start merging/finishing reviewing them after #69 is merged, to have a better picture of your changes.

armano2 · 2017-07-16T21:54:07Z

ok

michalsnik · 2017-07-19T14:30:42Z

Alright, you can fix conflicts now :)

armano2 · 2017-07-19T15:59:05Z

@michalsnik cleaned up and merged

mysticatea · 2017-07-19T21:51:21Z

I'm not sure this rule should disallow yield expressions.
This is not related to async. But it might be dangerous since the value is cached...

export default {
    computed: {
        *numbers() {
            yield 1
            yield 2
            yield 3
        }
    }
}

armano2 · 2017-07-19T22:06:08Z

await nodes are YieldExpression's with babel-eslint < 7.0.0

@mysticatea i don't like this to but babel-eslint < 7.0.0 👎

babel/babel-eslint#350

mysticatea · 2017-07-19T22:43:18Z

@armano2 I see. Hmm, how about disallowing async functions instead of await expressions? babel-eslint@6 looks to have node.async === true for async functions correctly. The await expressions on outside of async functions are syntax error.

armano2 · 2017-07-19T23:09:25Z

@mysticatea yes its true and we node.async === true is already supported

Doc: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/yield

The yield keyword causes generator function execution to pause and the value of the expression following the yield keyword is returned to the generator's caller. It can be thought of as a generator-based version of the return keyword.

For me yield can be called async-like.

A yield, which causes the generator to once again pause and return the generator's new value. The next time next() is called, execution resumes with the statement immediately after the yield.

but if you want i can remote it from support and add it as separate rule.

michalsnik · 2017-07-21T09:07:11Z

Hmm, I'm ok with both versions but to be honest, who would ever want to set generator as computed property? It doesn't make sense at all. I think we can even remote this check and maybe add another rule for that but personally I don't think it's even necessary.. What do you think guys?

armano2 · 2017-07-21T13:49:46Z

@michalsnik yield/generator removed 🍡

michalsnik · 2017-07-22T11:37:03Z

Okay, one last thing @armano2 let's change the await check to async function detection and it's good to go.

armano2 · 2017-07-22T13:20:45Z

@michalsnik but we are checking async function declaration already here: https://github.com/vuejs/eslint-plugin-vue/pull/72/files#diff-ceb2d093cf4ac93cc581e9af530bfebaR70

michalsnik added the work in progress label Jul 9, 2017

michalsnik self-requested a review July 9, 2017 00:21

armano2 force-pushed the patch-2-no-async-in-computed-properties branch 5 times, most recently from 39016d1 to d4c4742 Compare July 13, 2017 23:54

armano2 changed the title ~~No async in computed property functions~~ WIP: No async in computed property functions Jul 14, 2017

armano2 changed the title ~~WIP: No async in computed property functions~~ No async in computed property functions Jul 14, 2017

armano2 changed the title ~~No async in computed property functions~~ Add no-async-in-computed-properties rule Jul 14, 2017

armano2 mentioned this pull request Jul 15, 2017

Add return-in-computed-property rule. #78

Merged

3 tasks

armano2 force-pushed the patch-2-no-async-in-computed-properties branch from f60b332 to 4692050 Compare July 16, 2017 13:35

michalsnik suggested changes Jul 16, 2017

View reviewed changes

michalsnik added Ready for review do not merge and removed work in progress labels Jul 16, 2017

michalsnik removed the do not merge label Jul 19, 2017

armano2 force-pushed the patch-2-no-async-in-computed-properties branch from 597085f to c0cdedd Compare July 19, 2017 15:58

michalsnik added the semver-minor label Jul 21, 2017

michalsnik approved these changes Jul 22, 2017

View reviewed changes

armano2 and others added 3 commits July 22, 2017 18:45

Add no-async-in-computed-properties rule.

aa17e77

Remove yield/generator from this rule.

76695d0

Remove yield from example code

67c1086

armano2 force-pushed the patch-2-no-async-in-computed-properties branch from 3fafcbb to 67c1086 Compare July 22, 2017 16:46

michalsnik merged commit 5ebdf71 into vuejs:master Jul 23, 2017

michalsnik removed the Ready for review label Jul 23, 2017

armano2 deleted the patch-2-no-async-in-computed-properties branch July 23, 2017 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `no-async-in-computed-properties` rule #72

Add `no-async-in-computed-properties` rule #72

armano2 commented Jul 8, 2017 •

edited by michalsnik

Loading

armano2 commented Jul 13, 2017 •

edited

Loading

michalsnik commented Jul 14, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

armano2 Jul 16, 2017

armano2 Jul 16, 2017 •

edited

Loading

michalsnik Jul 16, 2017

armano2 Jul 16, 2017

armano2 Jul 16, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

armano2 commented Jul 16, 2017

michalsnik Jul 16, 2017

michalsnik Jul 16, 2017

armano2 Jul 16, 2017 •

edited

Loading

armano2 Jul 16, 2017

michalsnik commented Jul 16, 2017

armano2 commented Jul 16, 2017

michalsnik commented Jul 19, 2017

armano2 commented Jul 19, 2017

mysticatea commented Jul 19, 2017

armano2 commented Jul 19, 2017 •

edited

Loading

mysticatea commented Jul 19, 2017

armano2 commented Jul 19, 2017 •

edited

Loading

michalsnik commented Jul 21, 2017

armano2 commented Jul 21, 2017

michalsnik commented Jul 22, 2017

armano2 commented Jul 22, 2017

		@@ -0,0 +1,56 @@
		# Check if there are no asynchronous action inside computed properties (no-async-in-computed-properties)

		Vue.js has a basic construct which lets computed properties collects dependencies and refreshes them thats why its really important to not use any of asynchronous action inside of them.

		@@ -0,0 +1,178 @@
		/**
		* @fileoverview Check if there are no async inside computed properties.

		@@ -0,0 +1,64 @@
		# Check if there are no asynchronous action inside computed properties (no-async-in-computed-properties)

Add no-async-in-computed-properties rule #72

Add no-async-in-computed-properties rule #72

Conversation

armano2 commented Jul 8, 2017 • edited by michalsnik Loading

armano2 commented Jul 13, 2017 • edited Loading

michalsnik commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 commented Jul 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalsnik commented Jul 16, 2017

armano2 commented Jul 16, 2017

michalsnik commented Jul 19, 2017

armano2 commented Jul 19, 2017

mysticatea commented Jul 19, 2017

armano2 commented Jul 19, 2017 • edited Loading

mysticatea commented Jul 19, 2017

armano2 commented Jul 19, 2017 • edited Loading

michalsnik commented Jul 21, 2017

armano2 commented Jul 21, 2017

michalsnik commented Jul 22, 2017

armano2 commented Jul 22, 2017

Add `no-async-in-computed-properties` rule #72

Add `no-async-in-computed-properties` rule #72

armano2 commented Jul 8, 2017 •

edited by michalsnik

Loading

armano2 commented Jul 13, 2017 •

edited

Loading

armano2 Jul 16, 2017 •

edited

Loading

armano2 Jul 16, 2017 •

edited

Loading

armano2 commented Jul 19, 2017 •

edited

Loading

armano2 commented Jul 19, 2017 •

edited

Loading