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

Rule proposition: No async in computed property functions #53

Closed
chrisvfritz opened this issue Jun 26, 2017 · 10 comments
Closed

Rule proposition: No async in computed property functions #53

chrisvfritz opened this issue Jun 26, 2017 · 10 comments

Comments

@chrisvfritz
Copy link
Contributor

A common issue that beginners run into is relying on async code in a computed property function, even though we warn about it in the docs. This might not be possible to identify 100% of the time, but it would be nice for the linter to warn about:

  • async/await
  • passing a function as a function argument (covers promises and callbacks)

Am I missing any cases where either of the above would actually be valid inside a computed property function?

@michalsnik
Copy link
Member

Sounds nice! I think this one will definitely fit in Best practices category.

Could you please add few examples of code that in your opinion should cause warnings @chrisvfritz ?
This way it will be easier to get anyone's head around this and later to define proper test scenarios.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Jun 27, 2017

Test scenarios:

computed: {
  foo: async function () {
    return await someFunc()
  }
}
computed: {
  foo () {
    return new Promise((resolve, reject) => {})
  }
}
computed: {
  foo () {
    return fetch(url).then(response => {})
  }
}
computed: {
  foo () {
    return funcWithCallback(function () {})
  }
}

@mysticatea
Copy link
Member

I have a concern about callbacks. Can it distinguish sync or async? For example, return this.users.map(u => u.name). It may be Array#map or Promise#map (Bluebird).

@chrisvfritz
Copy link
Contributor Author

@mysticatea I think it'd be better to err on the side of undermatching than overmatching, but I'm thinking about a variety of strategies that could be combined for pretty good detection. For example, if there's a then method or any argument called response or res.

@michalsnik
Copy link
Member

michalsnik commented Jun 28, 2017

I think we should cover first 3 examples, and the last one would be to detect presence of any of then, catch or finally in all CallExpressions inside computed property's body. Bur I'm against checking arguments response and res as they might be used even if the body has nothing to do with async computing. For example application has a module that gathers users' responses for a given topic, than we could have a computed property activeResponse in which we'd like to call a utility function with response argument, or short res. I had similar case with route in other plugin. I thought it's enough to check for that name and then someone had an app with generating routes on google maps :D

@chrisvfritz
Copy link
Contributor Author

@michalsnik Good thinking. 🙂

@armano2
Copy link
Collaborator

armano2 commented Jul 4, 2017

It's all or did i miss something?

  invalid: [
    {
      filename: 'test.vue',
      code: `export default {
        computed: {
          foo: async function () {
            return await someFunc()
          }
        }
      }`,
      errors: ['Computed properies cannot have side effect.']
    },
    {
      filename: 'test.vue',
      code: `export default {
        computed: {
          foo: async function () {
            return new Promise((resolve, reject) => {}) }
          }
        }
      }`,
      errors: ['Computed properies cannot have side effect.']
    },
    {
      filename: 'test.vue',
      code: `export default {
        computed: {
          foo: function () {
            return bar.then(response => {})
          }
        }
      }`,
      errors: ['Computed properies cannot have side effect.']
    }
    {
      filename: 'test.vue',
      code: `export default {
        computed: {
          foo: function () {
            return bar.catch(e => {})
          }
        }
      }`,
      errors: ['Computed properies cannot have side effect.']
    },
    {
      filename: 'test.vue',
      code: `export default {
        computed: {
          foo: function () {
            return Promise.all([])
          }
        }
      }`,
      errors: ['Computed properies cannot have side effect.']
    },
    {
      filename: 'test.vue',
      code: `export default {
        computed: {
          foo: function () {
            return Promise.race([])
          }
        }
      }`,
      errors: ['Computed properies cannot have side effect.']
    },
    {
      filename: 'test.vue',
      code: `export default {
        computed: {
          foo: function () {
            return Promise.reject([])
          }
        }
      }`,
      errors: ['Computed properies cannot have side effect.']
    },
    {
      filename: 'test.vue',
      code: `export default {
        computed: {
          foo: function () {
            return Promise.resolve([])
          }
        }
      }`,
      errors: ['Computed properies cannot have side effect.']
    }
  ]

@michalsnik
Copy link
Member

Almost there @armano2, I have few things to mention:

  1. You missed the case with .finally
  2. Message should be: Unexpected asynchronous action in computed property "xxx" or something similar.
  3. We should also test cases with explicit getters and setters:
computed: {
  foo: {
    get() {},
    set() {}
  }
}
  1. We should wait with implementation of this rule, as I'm introducing logic that will help implementing this scenario in no-side-effects-in-computed-property rule.

@armano2
Copy link
Collaborator

armano2 commented Jul 5, 2017

i partially implemented those rules yesterday, but i will have a look into your helpers 🍡

thank you for feedback

@michalsnik
Copy link
Member

Done in v3.8.0 🚀 Thanks @armano2 for the hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants