jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.09k stars 5.38k forks source link

Rethink Backbone.sync #3767

Open megawac opened 9 years ago

megawac commented 9 years ago

As we all know, there are numerous plugins in the wild which override Backbone.sync with specific domain logic. Things like storage proxies, web socket connections, etc. The way plugins add this functionality tends to be messy and conflict with one another.

For Backbone v2 I suggest we reimplement how a custom sync should be added to Backbone. I've long suggested these plugins export a CustomCollection and CustomModel with a custom sync function implemented there, as opposed to globally overriding BB.sync; however they both have some merit. I would suggest something like the following adding a custom sync

function predicate(model, options) {
   return model.useProxy;
}

function syncFn(model, options) {
   // sync function as normal
}

// Whether this sync method should stop other syncs from being checked.
// This could be useful for storage syncs which wish to add data to an indexedDb
// while also executing an http sync
var exclusiveSync = false;

Backbone.addSyncMethod(predicate, syncfn, exclusiveSync);
akre54 commented 9 years ago

You're talking a middleware approach, similar to Express, right? Like the different sync functions could be chained?

Why wouldn't a regular conditional in your custom sync work for this use case?

jashkenas commented 9 years ago

Why wouldn't a regular conditional in your custom sync work for this use case?

Ding ding ding.

I'm a pretty big anti-fan of middleware compared to a simple plugin that just calls what it needs to call.

jsatk commented 9 years ago

While the way to override backbone.sync can be a bit ugly, it's also very obvious and clean. Backbone rides the line between "magic" and allowing the implementer to understand what is happening quite well. I wouldn't want a middleware approach, personally. @megawac, I'm curious—why not make a Backbone plugin that extends Backbone to do exactly what you suggested?

megawac commented 9 years ago

Its pretty common for (major) plugin developers to fall into this trap and thrash the sync environment by storing access to the original/previous sync in a way that may conflict.

For instance when mixing two very popular sync overrides, indexed-db-adapter and localStorage you will run into a stack overflow issues due to recursive calls if you load the indexed-db-adapter second. This occurs when getSyncMethod resolves to the default case (ajaxSync) thus causing it to resolve again to getSyncMethod

https://github.com/superfeedr/indexeddb-backbonejs-adapter/blob/master/backbone-indexeddb.js#L618 https://github.com/jeromegn/Backbone.localStorage/blob/master/backbone.localStorage.js#L239


Though this could be implemented as a plugin I don't think that is the optimal way to disseminate this change. I think this is a core feature and something that should be implemented officially

s-devaney commented 9 years ago

+1

Would be good if the additional functionality was baked into Sync and we could configure behaviour ourselves rather than a plugin/chain/override system.

akre54 commented 9 years ago

Maybe we could rename sync to ajaxSync and do Backbone.sync = Backbone.ajaxSync if it's enough of a convention.

jsatk commented 9 years ago

I like @akre54's suggestion. This allows backwards compatibility but also (I believe) addresses @megawac's concerns. And @megawac—thanks for the thorough explanation. You've got me thinking about it differently now.