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

More specific notifications in observableArray #502

Closed
analogrelay opened this issue May 29, 2012 · 19 comments
Closed

More specific notifications in observableArray #502

analogrelay opened this issue May 29, 2012 · 19 comments

Comments

@analogrelay
Copy link

Hey knockout crew.

I'm working with Knockout in a Windows 8 app and in order to properly bind to some WinJS controls, I need to be able to adapt ko observableArrays in to IListDataAdapter implementations. I'm totally fine with writing such an adapter myself but it's proving more difficult than I'd like due to the fact that observableArrays just fire a simple notification with the new array and it's up to me to figure out what actually changed. I'd like to propose the following change:

  1. To Subscribables: notifySubscribers takes a third optional argument 'changeInfo'
  2. To Observables: valueHasMutated/valueWillMutate takes this third arg and passes it along
  3. To Observable Arrays: The array mutation functions construct an object describing the change and pass it to valueHasMutated/valueWillMutate.

This 'changeInfo' data structure might look something like this for a push:

{ type: 'add', items: [{ index: indexOfAddedItem, item: added }] }

Or for a removeAll

{ type: 'remove', items: [{index: indexOfRemovedItem1, item: removed1},{index: indexOfRemovedItem2, item: removed2}] }

Splices could either be a combination or a separate "type".

I don't really even need the removed/added/etc. item but I figured it would be useful data that the array has, the index alone would probably be sufficient.

Anyway, I'm happy to fork and work on this myself, but I don't want to spend a lot of time making this work inside KO if the core team isn't a fan of the idea (or if you have comments, I'd like to take them in to consideration before sending a PR).

Thoughts? If you guys really think this doesn't belong in KO, I can probably make it work by augmenting observableArrays myself, but I'd like to do something in the core to support this scenario :).

@analogrelay
Copy link
Author

I just realized that a cleaner solution would be to have separate events on top of the existing events (such as "inserted", "removed", etc.). I like that a little better and will probably play around with it in my fork.

@mbest
Copy link
Member

mbest commented May 29, 2012

This might be useful for you: http://jsfiddle.net/mbest/Jq3ru/

@analogrelay
Copy link
Author

Awesome! Thanks! I'll definitely use this to get me unstuck, but observationally it seems like it would be a slower solution than adding more granular notifications. Of course, without measurements I'm not sure but I wonder if this is something you guys would be interested in despite the ability to work around it?

Thanks again, if I get some time I might prototype this in my fork anyway, but for now, I'm unstuck!

@mbest
Copy link
Member

mbest commented May 29, 2012

Yes, and please share with us whatever you do put together.

@AndersMalmgren
Copy link

I would also love to see more info added to the subscribe callback. I checked the ko.utils.compareArrays, used in mbests example, and it will add alot of complexity for large arrays with lots of changes.

@SteveSanderson
Copy link
Contributor

Hey, thanks for the suggestion. It's unlikely that we'd precompute diffs on arrays proactively just in case the subscriber wants that information, as it would be a significant new perf cost.

However, subscribers can certainly make use of ko.utils.compareArrays to diff the array you had before with the one you have now, and thereby know what changed. Is that sufficient for your use case?

@AndersMalmgren
Copy link

Thanks for feedback Steve.
When a item is pushed / removed, you can intercept that and publish the event to the subscribers, no need to diff the array?

ko.utils.compareArrays is a solution offcourse, but it still has a lot of added complexity in worst cases above Ordo n^2

Im working on a Knockout Concurrency conflict resolver plugin and I would love this feature, check line 56 (extendObservableArray)
https://github.com/AndersMalmgren/Knockout.Concurrency/blob/master/src/knockout.concurrency.js

@SteveSanderson
Copy link
Contributor

When a item is pushed / removed, you can intercept that and publish the event to the subscribers, no need to diff the array?

No, it doesn't work like that. In general, when someone changes an observable array, they aren't pushing or removing items, they are simply replacing the entire observable array contents. The splice and push methods are just syntactic sugar for replacing the observable array contents with some new array that contains different entries (and the fact that it happens to still be the same underlying array instance is just coincidental).

The design of observable arrays is such that there are no guarantees that when someone mutates the array, they will do it through the push/splice/etc methods, so we can't intercept the change to know what the changed in advance.

@AndersMalmgren
Copy link

I'll have to look at the Knockout code, but to me all methods that mutates the array has knowledge about the items that did it (Add, AddRange, Remove, RemoveRange, etc).

Even the mapping plugin uses push to update the array (Subscribe methods triggers as many times has you have items in the JSON data), in most CRUD apps you let the user remove / add items too. So I think push / remove is a very common scenario...

@SteveSanderson
Copy link
Contributor

So I think push / remove is a very common scenario...

It's common, but it's not guaranteed. Anybody can write an entirely new collection to an observable array at any time.

@AndersMalmgren
Copy link

Yupp, so the subscribe method should give you an array of items (the entire array), an array of removed items, and an array of added items. Thats my two cents atlest, but I haven't had time to look at the knockout source for the observableArray, I guess you do not publish the subscribe event from the mutation methods on the array?

@SteveSanderson
Copy link
Contributor

Yupp, so the subscribe method should give you an array of items (the entire array), an array of removed items, and an array of added items. Thats my two cents atlest

Sorry, it just doesn't work like that. In general, people can make arbitrary mutations without using specific methods like splice and push. To have the API work as you describe in general, it would be necessary to run a diff after every change that wasn't traceable back to a known mutation function (and there would have to be a new mechanism to trace edits that were due to known mutation functions). That's why getting a diff is an opt-in thing - any subscriber can get one by calling ko.utils.compareArrays, but for perf reasons it doesn't happen by default.

@AndersMalmgren
Copy link

I still dont understand how you cant isolate the mutation methods and let them publish, but im sure you have good reasons, will go with the compareArrays for now, Thanks for your Help Steve!

/Anders

@AndersMalmgren
Copy link

Hmm, since the subscribe method gives the actual array reference I cant save the old state beacuse the old state will be updated to the new state. Any way to solve this without cloning the array each time?

ko.observableArray.fn.subscribeWithChanges = function(callback) {
    var oldArray = null;
    this.subscribe(function(items) {
        var changes = ko.utils.compareArrays(oldArray, items);
        var added = [];
        var deleted = [];
        ko.utils.arrayForEach(changes, function(item) {
            switch(item.status) {
                case "added":
                    added.push(item.value);
                    break;
                case "deleted":
                    deleted.push(item.value);
                    break;
            }           
        });
        oldArray = items;
        callback(added, deleted);
    });
}

@mbest
Copy link
Member

mbest commented Jun 7, 2012

Cloning is the only way to ensure they aren't the same array. The slice function is an easy way to clone: oldArray = items.slice(0); Also you can use concat: oldArray = [].concat(items);

@AndersMalmgren
Copy link

Yupp. it ended up being 40 slower than just doing n^2 :D

@mbest
Copy link
Member

mbest commented Jul 18, 2012

If an array is already tied to a foreach binding, the binding will be calling compareArrays on it anyway through setDomNodeChildrenFromArrayMapping. Maybe we could modify it to also post an "edit script" notification on the observableArray.

@togakangaroo
Copy link

First, awesome, thanks for ko.utils.compareArrays() - you guys should put that on the the actual docs page about observableArrays! Its crazy that the only way to find out about this very useful tool is through this issue and a google group posting. I'd recommend also explaining why each subscribe event can't receive the change information or you're going to be fielding a ton of questions like the one from Anders above.

Two, if in a particular case where someone interacts with an observableArray mostly through push/remove/splice, there is absolutely nothing stopping them from proxying those methods to run compareArrays behind the scenes and broadcast a different event. It should be all of...10 lines of code maybe?

Finally, @mbest your jsfiddle is broken in chrome (probably others).

Refused to execute script from
'https://raw.github.com/SteveSanderson/knockout.mapping/master/build/output/knockout.mapping-latest.debug.js'
because its MIME type ('text/plain') is not executable, and strict MIME type checking is enabled.

@mbest
Copy link
Member

mbest commented Sep 11, 2013

Now that #695 will be part of 3.0, does this issue still need to be open?

@mbest mbest closed this as completed Nov 2, 2013
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

5 participants