don't call equalityComparer for observableArray

[mbest]

from https://groups.google.com/d/msg/knockoutjs/2lj89SXHGJ8/V5CnEFDuCgAJ

We ran into some unexpected behaviors with equalityComparer in Knockout 3.4 both with deferred updates on and off, and realized this was actually a combination of unexpected behavior from pre-3.4 and new unexpected behavior from 3.4 with deferred updates.

These are exemplified here:

• https://jsfiddle.net/r39vwz3z/3/#run (Knockout 3.3)
• https://jsfiddle.net/r39vwz3z/4/#run (3.4, deferred updates off)
• https://jsfiddle.net/r39vwz3z/5/#run (3.4, deferred updates on)

The fiddle in question sets up an observable array and binds to it with the checked binding. It also creates a computed that depends on the list, and subscribes directly to the list. Each one of these appends to a visible log to indicate that it has fired and what value of the list it's seeing. Finally, we set a custom equality comparer and log what it receives.

In 3.3 and 3.4 with no deferred updates, the equality comparer is never invoked. My suspicion is that we've actually just been setting the comparer incorrectly all along.

In 3.4 with deferred updates, the comparer is used—but both its parameters are the final version of the array, rather than having the version before the check that's currently going into effect and the version after. So no matter how clever one wants to get, the equality comparer will always return true for a deep comparison.

Not sure what the right move is here. Should I file an issue? Or is this expected behavior? Is the setup of equalityComparer that I use incorrect anyway, regardless of how deferred updates affect it?

[ThomasMichon]

Analyzing the knockout code, this appears to be because the wrapper methods for ko.observableArray do not go through the normal update path for a ko.observable. The issue is this passage:

ko.utils.arrayForEach(["pop", "push", "reverse", "shift", "sort", "splice", "unshift"], function (methodName) {
    ko.observableArray['fn'][methodName] = function () {
        // Use "peek" to avoid creating a subscription in any computed that we're executing in the context of
        // (for consistency with mutating regular observables)
        var underlyingArray = this.peek();
        this.valueWillMutate();
        this.cacheDiffForKnownOperation(underlyingArray, methodName, arguments);
        var methodCallResult = underlyingArray[methodName].apply(underlyingArray, arguments);
        this.valueHasMutated();
        // The native sort and reverse methods return a reference to the array, but it makes more sense to return the observable array instead.
        return methodCallResult === underlyingArray ? this : methodCallResult;
    };
});

Here, when you use the wrapper method, it simply goes updates the underlying array directly, in between calls to this.valueWillMutate() and this.valueHasMutated(), bypassing the normal diff call from ko.observable. I would assume the bugfix would be to add a conditional around this.valueHasMutated() which checks a call to this.equalityComparer first.

[mbest]

I don't think there's anything an equalityComparer could do with an array. Once the array is updated, the old version of the array is gone. As I mentioned in the forum, we have the arrayChange event which notifies only actual changes in the array.

I meant for this issue to note that we might want to not call equalityComparer at all for arrays, because currently it's called only sometimes.