Skip to content

Added test combining watch and shouldUpdate #38

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

Merged
merged 1 commit into from
Aug 5, 2018
Merged

Added test combining watch and shouldUpdate #38

merged 1 commit into from
Aug 5, 2018

Conversation

tjallingt
Copy link
Contributor

Small thought i had while working on this: // eslint-disable-next-line no-unused-expressions these are necesairy in watch because its kind of strange, should watch not just be an array of properties to watch and be implemented as watch.map(prop => this[prop])?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f2fa3d5 on tjallingt:master into 99ff44c on foxbenjaminfox:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f2fa3d5 on tjallingt:master into 99ff44c on foxbenjaminfox:master.

@foxbenjaminfox foxbenjaminfox merged commit f2fa3d5 into foxbenjaminfox:master Aug 5, 2018
@foxbenjaminfox
Copy link
Owner

I've merged this PR, and I've published these changes as v3.4.0 to npm.

You're right about the fact that the way watch is implemented is not the best. It's the kind of thing that sounds great at the time, but in retrospect could be a lot cleaner. The way it currently works takes advantage of Vue's reactivity system in (what seems like) a neat way, but it's not really worth the hassle, because it doesn't really add much beyond doing it the simple way (with an array of properties.)

An array of properties can't do this:

const vm1 = new Vue({ data: { x: 0 } })
const vm2 = new Vue({
  data: { x: 1 },
  asyncComputed: {
    y: {
      get () { return this.x },
      watch () { vm1.x }
    }
  }
})

Ultimately though, that's an extreme edge case, and you probably shouldn't be doing that anyway. So I do consider the current way watch works to be a mistake, and if I did it again I would do it the simpler and better way, as an array of properties instead of a function.

Well, redoing watch is certainly something I'll think about for v4, when I decide that it's worth releasing a major version bump, with backwards-compatibility breaking changes. That will happen eventually, and when it comes I may well do an update to the watch interface as part of that.

@foxbenjaminfox
Copy link
Owner

Thanks for the PRs. I appreciate it.

If you ever feel like making some more, feel free to do so anytime. I'm always happy to receive good contributions to my projects.

@foxbenjaminfox foxbenjaminfox mentioned this pull request Aug 5, 2018
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.

3 participants