search

jmorrell / backbone.obscura

A read-only proxy of a Backbone.Collection that can be filtered, sorted, and paginated.
http://jmorrell.github.io/backbone.obscura/
MIT License
107 stars 19 forks source link

Obscura does not extend Backbone.Collection #17

Closed frank-weindel closed 10 years ago

frank-weindel commented 10 years ago

I'm sure this is by design but I'm a bit confused as to why. For this to really work in all cases the object itself should be instanceof Backbone.Collection. There are some libraries (Backbone.Epoxy for example) that accept collections and check if they are an instance of Backbone.Collection. If they are not, they do not work.

frank-weindel commented 10 years ago

I've managed to work around this by doing:

Backbone.Obscura = Backbone.Collection.extend(Backbone.Obscura.prototype);

This takes all of the attributes of Obscura and plops them into a Backbone.Collection extension.

But I still think this should just be like this from the get go for maximum utility. By the way, great work on this module. It really serves a need that is not addressed in any good way elsewhere.

jmorrell commented 10 years ago

I honestly hadn't thought of people calling instanceof on a collection passed to them.

Backbone.Obscura = Backbone.Collection.extend(Backbone.Obscura.prototype);

Forgive me if I'm wrong, but I think that this still leaves a couple of functions on the Obscura collection that will behave strangely, namely any method listed here: https://github.com/jmorrell/backbone-collection-proxy#usage

I originally tried to support every method on Backbone.Collection, but eventually decided it wasn't worth it. If you call proxy.add(model, { at: 3 }); on a sorted, filtered, paginated collection, what does that mean for the underlying superset collection? I could try to pass those arguments on to the superset, but that could result in odd behavior.

Currently trying proxy.add(model, { at: 3 }); should throw an error, but with your change I think it would add a model to the proxy.models array ignoring all of the obscura settings. (Please correct me if I'm wrong).

If this isn't something that bothers you because you know you won't try to directly modify the proxy collection using normal collection methods, then use your solution, but I think that it would lead to more confusion for the average user of this library.

jmorrell commented 10 years ago

I suppose I could overwrite those methods to throw errors as unsupported actions... That might be one way of solving this...

frank-weindel commented 10 years ago

In my use cases (using collection binding in Epoxy) I'm never modifying the Obscura'd collections directly so the workaround works. You're right though that those modify methods won't work right, and they should have to. I agree that overwriting those methods to throw "unsupported action" errors is the best way to go about it. Allow Obscura to still represent a read-only collection, but have it be a Backbone.Collection.

jmorrell commented 10 years ago

@frank-weindel Want to give https://github.com/jmorrell/backbone.obscura/pull/18 a try?

frank-weindel commented 10 years ago

Sorry meant to try this earlier. Looks good but I'll try to give it a go tomorrow.

jmorrell commented 10 years ago

Fixed by https://github.com/jmorrell/backbone.obscura/commit/d3d5665edbd526ddcd341a9047cce30e00ebca92