Regression from changing success callback parameters.

[timmywil]

In 6d589ba, success callback parameters were altered to include options as the third argument. Unfortunately, this shadows any options argument in the scope above. I ran into this when upgrading to 0.9.10 and noticed an exception thrown on a fetch call with no arguments. "Uncaught TypeError: Cannot read property 'update' of undefined " See line 814 for an example.

[timmywil]

Probably fixed by something like

...
fetch: function(options) {
      options = options ? _.clone(options) : {};
      if (options.parse === void 0) options.parse = true;
      var success = options.success;
      options.success = function(collection, resp, opts) {
        options = _.extend( options, opts );
        var method = options.update ? 'update' : 'reset';
        collection[method](resp, options);
        if (success) success(collection, resp, options);
      };
...

wherever options need to be extended through the success callback.

[tonylukasavage]

I'm seeing the same thing with backbone 0.9.10 and underscore 1.4.3. Did not have this issue with 0.9.2 and 1.4.2.

[tonylukasavage]

@timmywil: I tried what you suggested but no luck on my side. The call doesn't fail as undefined anymore, but the collection object doesn't seem to be a Collection. It appears to be the array of models contained in the collection, because it has neither the reset() or update() function on it. It seems like something further up the chain is causing the problem.

[timmywil]

I was guessing it was an easy fix, but I didn't do a ton of investigation. I will look into it more and do a pull request.

• Timmy

On Jan 18, 2013, at 9:49 PM, Tony Lukasavage notifications@github.com wrote:

@timmywil: I tried what you suggested but no luck on my side. The call doesn't fail as undefined anymore, but the collection object doesn't seem to be a Collection. It appears to be the array of models contained in the collection, because it has neither the reset() or update() function on it. It seems like something further up the chain is causing the problem.

—
Reply to this email directly or view it on GitHub.

[wyuenho]

options gets defaulted to {}, passed to Backbone.sync and then passed back to success. It's not shadowed and will always present from 0.9.9 onwards.

[tonylukasavage]

@wyuenho I get that that's supposed to happen, but what I'm seeing is not the case. The options in the success callback is not defined, and the collection does not appear to be a Collection object so much as it is an array of models.

[caseywebdev]

Custom sync methods are expected to be updated to call success(model, resp, options)

[timmywil]

@wyuenho : It is definitely shadowing, but it sounds like it may have been purposeful, which is what brought about all this confusion.

@caseywebdev : seems superfluous and unnecessary. Is there ever a time where you'd want options to be something different than the options var it is shadowing? Why not just extend with whatever options were changed? It wouldn't take much to make all backbone plugins with custom sync methods backwards compatible.

[tgriesser]

@timmywil and @tonylukasavage - #2221 should fix this one - internally the success callback parameters are reverted, so the options and model are referenced from the closure and the success/error from sync are only called with a single argument. If there are any issues with this, let us know and we can reopen. Thanks!