codebrew / backbone-rails

Easily use backbone.js with rails 3.1
MIT License
1.62k stars 255 forks source link

Overrided sync seems to call error callback with wrong parameters #139

Closed Derbeth closed 10 years ago

Derbeth commented 11 years ago

I think that when you upgraded Backbone from 0.9.2 to 0.9.9 you forgot to update backbone_rails_sync.js (the Git history says the file is 8 months old and Backbone 0.9.9 was released in December 2012).

I observe that when using backbone-rails, 'error' callback of Model#save() gets different parameters:

This breaks the contract defined in Model#save(): the parameters should always be (model, xhr, options), regardless of the error cause. As result code using Rails-Backbone 0.9.2 will be broken.

The bare Backbone 0.9.9 seems to work fine, so I suppose your modified version of Backbone.sync is the reason.

Comparing your version of Backbone.sync with the original: you pass 'complete' callback to jQuery while the original passes only 'error' and 'success' callbacks. I also don't like that you send different events than the standard Backbone: the original sends 'sync' on AJAX success and 'request' when a request is sent to server; you send 'sync:start' when a request is sent to server and 'sync:end' when a request is completed (even on AJAX error). See the marked line in the original code: https://github.com/documentcloud/backbone/blob/0.9.9/backbone.js#L1463

From my understanding:

gabetax commented 11 years ago

Is there any real advantage to have to effectively maintain a full fork of Backbone.sync when all it's really accomplishing is setting the CSRF in the $.ajax header? Would it make more sense to either:

  1. Wrap the original Backbone.sync, to pass the beforeSend option that sets the X-CSRF-Token header.
  2. Set the token globally for all requests via $.ajaxSetup as done in https://gist.github.com/3960219. This extends past the concerns of just Backbone, and looks like it may set the token on external requests too, but is fairly simple.
Derbeth commented 11 years ago

I switched my code to use rails-backbone from the linked issue branch and everything seems to work fine - I don't observe any regressions and I can use error callbacks as defined in Backbone documentation.

manusajith commented 10 years ago

This should be now fixed in master with f627521 Thanks @gabetax