KO sortable + Knockout 3.x not disposing elements?

[bago]

I hacked your example jsfiddle to show what I mean: This is your connected sample, I just added a "checkdispose" binding to check when the binding is initialized and disposed.

Is this correct for both KO 2 and KO 3?

ko.bindingHandlers.checkdispose = {
    init: function (element, valueAccessor) {
        console.log("init");
        ko.utils.domNodeDisposal.addDisposeCallback(element, function() { 
          console.log("dispose");
        });
    }
};

If that binding is correct then something is wrong with ko-sortable and ko 3.1:

This is using ko 2.2 and when you move item you see a dispose and an init in console: http://jsfiddle.net/Jr2rE/447/ if I simply change the dependency to KO 3.1 I don't see anymore the dispose logs, just inits: http://jsfiddle.net/Jr2rE/448/

[rniemeyer]

@bago - there is functionality in KO core that tries to reconcile that bindings may have manipulated the original elements that a template placed in the DOM. This was refactored in KO 3.0 and has some checks for the parent, which was causing our moved elements to not be considered. I am now manually ensuring that the element is removed/disposed, which fixes this in KO 3+ and is harmless in KO 2.x.

[bago]

In the mean time I had debugged the code and solved in a different way. Just before "if (targetIndex >= 0) {", so before ko viewmodel is changed, I "cancel" the jquery-ui sortable so that it reverts any DOM manipulation and let KO do it right:

$(sourceParent === targetParent ? this : ui.sender).sortable('cancel');

and I also removed this line that is no more necessary if cancelling:

ui.item.remove();

Wouldn't it be better to always cancel the jQueryUI sortable action so to be sure that everything is unchanged when we update KO and let it update the DOM? I guess you already thought at this and decided to follow a different route for a cause: can you give me some hints?

[rniemeyer]

@bago - I will have to look into approach a bit. I would actually be more comfortable with a solution like that one. In initial testing, I don't see any visual issues or anything else concerning about cancelling each time. I was testing with:

    //build up args for the callbacks
    if (sortable.beforeMove || sortable.afterMove) {
        arg = {
            item: item,
            sourceParent: sourceParent,
            sourceParentNode: sourceParent && ui.sender || el.parentNode,
            sourceIndex: sourceIndex,
            targetParent: targetParent,
            targetIndex: targetIndex,
            cancelDrop: false
        };

        //execute the configured callback prior to actually moving items
        if (sortable.beforeMove) {
            sortable.beforeMove.call(this, arg, event, ui);
        }
    }

    //call cancel on the correct list, so KO can take care of DOM manipulation
    if (sourceParent) {
        $(sourceParent === targetParent ? this : ui.sender).sortable("cancel");
    }
    //for a draggable item just remove the element
    else {
        $(el).remove();
    }

    //if beforeMove told us to cancel, then we are done
    if (arg && arg.cancelDrop) {
        return;
    }

    //do the actual move
    if (targetIndex >= 0) {
        if (sourceParent) {
            sourceParent.splice(sourceIndex, 1);

            //if using deferred updates plugin, force updates
            if (ko.processAllDeferredBindingUpdates) {
                ko.processAllDeferredBindingUpdates();
            }
        }

        targetParent.splice(targetIndex, 0, item);
    }

    //rendering is handled by manipulating the observableArray; ignore dropped element
    dataSet(el, ITEMKEY, null);

[bago]

I've just updated my code to match your last comment and works fine in my use case!

[rniemeyer]

ok- I have done my testing on it and haven't found any issues. I am going to push a new version with these updates.