computed can be attached to multiple nodes and dispose when all are removed #484

Closed
mbest opened this Issue May 15, 2012 · 7 comments

Comments

Projects
None yet
3 participants
Owner

mbest commented May 15, 2012

Many times, a computed is used to track dependencies that affect bindings of multiple nodes. Currently there's not a good way to handle this in Knockout. A single node is tracked directly (usually a parent node, or the first of sibling nodes) and the others are handled by a disposeWhen function.

This change will improve the existing code that needs to track multiple nodes and will provide a way to handle observable view models as part of #321.

@ghost ghost assigned mbest May 31, 2012

@mbest mbest referenced this issue Jun 6, 2012

Merged

Templating fixes #144

Owner

mbest commented Jun 6, 2012

I first thought of this feature as a way to support observable view models that are tied to a binding context. If the view model is updated, so is the binding context and any dependents (child contexts and bindings). The binding context itself will need a subscription to the view model to be notified of updates and will do so using a computed observable. Since a binding context will generally be shared among many different nodes in the DOM, I needed a way to track multiple nodes and only dispose the computed object once all of the nodes were removed.

First I implemented this specifically for the binding context. Basically I added a dispose callback for each node that used the binding context and maintained an array of nodes. The dispose callback would remove the node from the array, and if the array was empty, finally dispose the computed.

I then realized that other areas of Knockout could use this same functionality (templates specifically) and so generalized it within ko.computed. disposeWhenNodeIsRemoved could accept an array of nodes and ko.computed included functions to add or replace the watched nodes.

Later, I decided to take it a step further and generalize it within domNodeDisposal. So my current implementation has computed using an updated domNodeDisposal that supports attaching multiple nodes to a single disposal callback.

Contributor

SteveSanderson commented Jul 27, 2012

Could you clarify what the benefit of this change would be?

Looking through the code, I've only noticed one place where we make a semi-arbitrary choice about which node to select for disposeWhenNodeIsRemoved, i.e., during ko.renderTemplate when we're in replaceNode mode we watch for removal of the parentNode of the first entry in the target set (and this only applies to KO 1.0-style string-based templating). In all other cases it seems that there is one unambiguous natural candidate to use for disposeWhenNodeIsRemoved.

I may be missing some point, so could you give an example to show what problems arise with this arrangement?

Owner

mbest commented Jul 30, 2012

In addition to what you mentioned (string-based templates), this would also be useful for arrayToDomNodeChildren. Currently it actively tracks the "container" node and passively tracks the first node in the array. Having it track all of the "mapped" nodes would make it more robust, because as it is now, the disposal could happen before it should.

Contributor

SteveSanderson commented Jul 31, 2012

Having it track all of the "mapped" nodes would make it more robust, because as it is now, the disposal could happen before it should.

Can you provide a repro of a situation where disposal happens too early? Is this only if the developer manually mutates the DOM from outside Knockout?

I'd be happy to enhance this if anyone has encountered a real-world case where it affected them in normal use (as it would then certainly be a bug) but if it's purely theoretical perhaps we're better focusing our efforts elsewhere :)

Owner

mbest commented Aug 1, 2012

Can you provide a repro of a situation where disposal happens too early?

Yes, I'll see what I can put together. But it's probably something that few, if any, have run into.

Mainly this is something that will be useful for #485, and I thought it would be good to make it a general-purpose feature that could benefit other areas of Knockout.

Contributor

SteveSanderson commented Apr 8, 2015

@mbest, can we close this or are you aware of scenarios where we'd still have an issue?

Owner

mbest commented Apr 8, 2015

Yeah, no need to keep this open.

@mbest mbest closed this Apr 8, 2015

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