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

subscriptions can add dependencies to computed #341

Closed
mbest opened this issue Feb 23, 2012 · 6 comments
Closed

subscriptions can add dependencies to computed #341

mbest opened this issue Feb 23, 2012 · 6 comments
Milestone

Comments

@mbest
Copy link
Member

mbest commented Feb 23, 2012

The beforeChange subscriptions are run within a computed's dependency block. Thus any observables that they access become subscriptions of the computed observable.

Here is a fiddle to demonstrate: http://jsfiddle.net/mbest/RdFYC/

change subscriptions can also cause subscriptions, but it's a little more complicated:

http://jsfiddle.net/mbest/f7YtZ/

@mbest
Copy link
Member Author

mbest commented Apr 28, 2012

I've included a fix for this bug in my deferred updates plugin: https://github.com/mbest/knockout-deferred-updates.

Here are the two examples with the plugin included (but with deferred updates turned off):

http://jsfiddle.net/mbest/JAg4c/

http://jsfiddle.net/mbest/7N2Kg/

@SteveSanderson
Copy link
Contributor

I see. This is a good justification for an internal instance of the ignoreDependencies facility that you came up with a while back! (There could be other solutions, but I agree that ignoreDependencies is the most straightforward.)

I'll convert this into some specs and a pull request tomorrow.

@mbest
Copy link
Member Author

mbest commented Jul 9, 2012

Great. The specs that I made are in branch 341-ignore-dependencies-from-callbacks.

@SteveSanderson
Copy link
Contributor

Excellent - thanks for the specs. Pull request added: #563

@SteveSanderson
Copy link
Contributor

(Reopening until merged - didn't mean to close this!)

@mbest
Copy link
Member Author

mbest commented Jul 11, 2012

Looks good. Merged.

@mbest mbest closed this as completed Jul 26, 2012
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

No branches or pull requests

2 participants