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

added support for watch as array and moved all the watch logic to seprate module. #66

Merged
merged 7 commits into from
Feb 16, 2019

Conversation

eladFrizi
Copy link
Contributor

@eladFrizi eladFrizi commented Feb 12, 2019

I added an option to use the watch option as array fo properties instead of function.
In this way, the watch is closer to the Vue original watch.
@foxbenjaminfox

export default {
  data() {
    return {
      x: 2,
      y: 3,
      z: 0,
      obj: {
        v:{
          j: 0
        },
        m: 0
      }
    }
  }
  ,
  asyncComputed: {
    sum: {
      get() {
        const total = this.x + this.y
        return new Promise(resolve =>
          setTimeout(() => resolve(total), 1000)
        )
      },
      watch: ['z','obj.m','obj.v.j']
    }
  }
}

@foxbenjaminfox
Copy link
Owner

Thanks. I'll review and merge this, but just three things about the tests:

  1. You seem to have unintentionally made some unrelated whitespace changes across all of test/index.js. Could you undo that?
  2. More importantly, it's important that we have tests asserting that both styles of watcher work. (Both arrays, and functions for backwards compatibly.)
  3. Let's also have tests for each relevant code path.
  • For both simple property names (without dots) and nested properties (with dots).
  • For both the case when the array has one element, and the case when it has multiple.

Also, if the user passes something that's neither an array nor a function, let's throw an error instead of silently ignoring it. Similarly, let's check that the array elements are strings, and throw an error if not.

@eladFrizi
Copy link
Contributor Author

Hey Benjamin.
Added test for watch as a function, watch as array, nested path, and multiple proptires.
Also, undo the spaces and added the error when watch value is invalid.
Thanks!

@foxbenjaminfox foxbenjaminfox merged commit fba83d3 into foxbenjaminfox:master Feb 16, 2019
@foxbenjaminfox
Copy link
Owner

Thanks! I've merged the PR. I'm glad you contributed this feature, it is something that definitely needed doing. I've now released a new version which includes this feature, so users can start using it right away. Sooner or later I plan to deprecate using a function watcher, and in the next major version bump I'll probably remove it.

And once again, thanks for contributing. If you find something else you want to send another PR about, feel free.

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

Successfully merging this pull request may close these issues.

2 participants