ko.cleanNode method should not call jQuery.cleanData #1130

Closed
remal opened this Issue Oct 2, 2013 · 11 comments

Projects

None yet

7 participants

@remal
remal commented Oct 2, 2013

jQuery.cleanData can be called in ko.removeNode, it makes sense. But calling cleanData in cleanNode causes many unexpected side effects.

@jbblanchet

I have such a side effect: I'm trying to create a custom binding for gridster (http://gridster.net). I'm using the foreach with a beforeRemove to delete the element from gridster. When removing an element, gridster needs the data attached to the dom element to fill the space vacated by the removal. This data has been cleaned by knockout, and gridster gives me an exception. Demo: http://jsfiddle.net/Be4cf/11/ (The demo references my own copy of knockout where I've removed the cleanup for testing purposes.)

@SteveSanderson
Contributor

Thanks for describing this issue.

ko.cleanNode's design has always involved trying to erase all additional state from the node, so I hope you can see why it makes sense to call $.cleanData in most cases. Your situation is quite unusual in that you don't really want the node to be cleaned completely.

It would be unusual for us to make a breaking change for everyone because of one scenario, so for now I'd suggest a workaround for you which would be using an alternative technique to store your data on DOM nodes. It's quite easy to replicate what $.data does, so you can implement your own separate storage area that neither KO or jQuery would erase.

Does this seem reasonable?

Closing the issue for now as I don't think we'd be likely to change this behavior imminently, but please reopen if you disagree and want do discuss further!

@jbblanchet

Thanks for the quick response.

First let me say that I love what I've seen of KnockOut thus far. I started prototyping with it on Monday, and I have a hard type letting go of my keyboard. I wake up at night with new ideas of where we could use it. You guys are doing GREAT job.

Also, I understand your reservations about making breaking changes, and I admit I'm not familiar with KnockOut enough to see all the unintended consequences of changing the cleanData method. But let's say for the sake of discussion that we're starting from a clean slate. I can see a couple of reasons why cleanNode shouldn't remove the jQuery data:

  1. On a purely abstract level, and this is the most important point for me, I agree with the OP. KnockOut shouldn't call cleanData because this may cause unexpected side effects. KnockOut should clean it's own data, but leave the rest alone. A big reason why we're very cautious about using any framework in the first place is the amount of "black magic" that goes on behind our back. I think limiting possible side effects should be a top priority of any framework.

  2. As a user of KnockOut, I don't call cleanData. It is called by the modification of the view model. And by specifying a beforeRemove callback, I'm really telling KnockOut to let me handle the removal of my item. I don't see why, if I'm taking the responsibility for the removal, I shouldn't also have the responsibility of cleaning up my data. An explicit call to cleanData would probably be different, but this is not the case here.

  3. I understand that I can store my data elsewhere. But gridster is a relatively stable widget. Changing this behaviour in gridster could be a breaking change for its users (plus I'm not a developer or gridster, simply a user). I could also stock the data on my own, but this would be breaking the encapsulation. There are plenty of hacks that would make this work. But this is the kind of code that will be a mess to maintain in the future.

  4. Even if we forget gridster, I agree that I could stock my data elsewhere, but why would I? The data structure from jQuery is there for this explicit purpose ("Store arbitrary data associated with the matched element"). I don't understand how KnockOut can determine that it should be destroying arbitrary data it knows nothing about.

I'm not saying this is a show stopper for us. Worst case, we'll fork our own branch and comment that specific line. It's more the philosophical implications that I find worrying. The decisions to move our rendering to KnockOut is a big one for us. I'm really excited about the possibilities, but I'm a little uneasy about the willingness to go outside the boundaries of what I'm asking from it.

Thank you for your time.

@mbest
Member
mbest commented Nov 22, 2013

There is currently a pull request for this, #922, modifying the behavior specifically for the beforeRemove callback.

@mbest
Member
mbest commented Nov 22, 2013

Take a look at what I've done here #865.

@srgstm
srgstm commented Dec 17, 2013

I use KnockOut with jQuery Mobile. When I need to remove KO bindings, there is only the option of calling ko.cleanNode(). However doing this also removes all jQuery Mobile data associated with the node. SteveSanderson says this is by design, as cleanNode is supposed... to clean a node. That's fine. If put this way, then I don't need to clean a node. I need to unapplyBindings!

@remal
remal commented Jan 3, 2014

First of all, sorry for bad description.

Second, jQuery holds all it's event bindings with $.data(). So, jQuery.cleanData() will remove all bound handlers. For node and its children.

So, when I call ko.cleanNode(), all event handlers will be removed. This is very unexpected...

@vamp
vamp commented Jan 4, 2014

@remal, Try to use event delegation using selectors...
$("html").on("click", ".btn", function(){ alert("click"); }); intead of $(".btn").on("click", function(){ alert("click"); });

@remal
remal commented Jan 4, 2014

I can write workaround, no problems. But such workaround can't fix bad behaviour inside knockout.

@rathkopf

+1 for not calling jQuery.cleanData(). @jbblanchet summed it up beautifully. This just bit me when ko.cleanNode() removed all jqueryui widgets.

@mbest
Member
mbest commented May 23, 2014

@rathkopf - take a look at this change that was included in KO 3.1: #865. It allows you to override ko.utils.domNodeDisposal.cleanExternalData. Documentation.

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