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

Return a rejected promise instead of false on invalid model save #2489

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

akre54
Copy link
Collaborator

@akre54 akre54 commented Apr 16, 2013

From the discussion in #2345, returning false from a failed save prevents a clean chaining API (model.save().done(...).fail(...)). Using deferreds, we can reject a promise instead to avoid the discrepancy.

This pull doesn't introduce any new options vars (i.e. isValid), instead suggesting a separate event listener for the model's invalid event or querying model.isValid() directly within the failed handler. The invalid event will always be fired before the deferred is rejected and returned. The fail handler is fired for both failed validation as well as any ajax errors which makes it difficult to mutate the options object or keep the two callbacks' arguments consistent.

@@ -443,7 +443,7 @@
// If the server returns an attributes hash that differs, the model's
// state will be `set` again.
save: function(key, val, options) {
var attrs, method, xhr, attributes = this.attributes;
var attrs, method, xhr, dfd = new $.Deferred, attributes = this.attributes;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about re-mapping Deferred to live directly on the Backbone object, like Backbone.ajax? This would allow someone to sub in a different promise / deferred implementation if they're not planning to use jQuery's.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea. Would be nice to be able to swap for q, when.js, standalone-deferreds, etc. in here if desired. I was debating a fallback to returning false if no deferred implementation was set (esp. in the case of zepto where ajax methods aren't promise-compatible). What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally be in favor or requiring Zepto users to supply their own promise implementation, but if the branching behavior is clean to implement and fairly minimal I think that could be good compromise. @tgriesser, @tbranyen or @braddunbar have anything to say about that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I think it'd be better to keep the api consistent than provide concessions for non-standard $ libs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking something along the lines of changing L462 to if (!this.set(attrs, options)) return !!dfd && dfd.reject(); It'll return false if the method is not implemented, and the rejected promise if it does.

@tgriesser
Copy link
Collaborator

If we're going to allow swapping the promise lib, it would be great if that applied to all ajax instances...

sync: function() {
  return Backbone.Deferred().when(Backbone.sync.apply(this, arguments));
}

Since jQuery's promises don't follow the promises spec... I would really, really love to see this change.

@@ -959,10 +959,16 @@ $(document).ready(function() {
var model = new Backbone.Model;
model.validate = function(){ return 'invalid'; };
model.sync = function(){ ok(false); };
strictEqual(model.save(), false);
model.save()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to use then instead of chaining done and fail. This change would keep the door open to alternate promise implementations that don't have done and fail sugar (if someone were so inclined to run the test suite with something else, of course.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call. updated.

@wookiehangover
Copy link
Collaborator

@tgriesser I would want to stay away from relying on when and stick to the generic reject, resolve, promise API... when isn't in every promise implementation and in any case in jQuery it lives directly on the $, not on $.Deferred()

@tgriesser
Copy link
Collaborator

@wookiehangover - ah right, it does live on $... I guess I was thinking that it'd be nice if we were going to use promises (outside of the ones included in jqXHR), and you want to swap it for an implementation that works properly (not jQuery's), it'd be nice to have that switch be automatically applied across the board... but I guess it only takes a simple shim of Backbone.ajax to do that...

@tgriesser
Copy link
Collaborator

Ran into this again today when I forgot you can't assume a promise on the save. I think it'd be a nice enhancement.

@wookiehangover
Copy link
Collaborator

@jashkenas can we get a go/no go on merging this?

@@ -518,7 +518,7 @@

if (this.isNew()) {
options.success();
return false;
return Backbone.Deferred().reject();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wondering about the fact that the "success" handler is called here but sends a rejected promise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does seem kind of strange. If anything this should be a resolved promise because the state of the action is technically complete (and successful,) there's just not a call to the server, and ergo no promise to chain off of.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I was trying to follow the return false pattern, but technically since the result completes, it makes more sense to resolve the promise here. Fixed.

@braddunbar
Copy link
Collaborator

I have to say, this one makes me a bit nervous. Despite returning promises, Backbone doesn't currently use them. While it might be slightly nicer to get a promise instead of false (and this is certainly debatable), it's a giant can of worms to open for a fairly small issue.

@wookiehangover
Copy link
Collaborator

@braddunbar which is why I closed the original issue in the first place.... @jashkenas opened it back up in the interest of having a cleaner api.

But there are several things that fall out of this, including breaking out-of-the-box compatibility with Zepto once and for all (which has already happened IMO, but I digress)

@braddunbar
Copy link
Collaborator

Yep, Zepto and any other library with a similar API. Good point.

@jashkenas
Copy link
Owner

Yes -- I'm fairly far towards leaning towards a "no go" on this ... but I haven't sat down and thought about it as much as I'd like to just yet. Maybe tag as a change wontfix, but leave it open a little longer?

@akre54
Copy link
Collaborator Author

akre54 commented Jun 9, 2013

As @ErichBSchulz pointed out in #2597, jqXHR's success, error, and complete callbacks methods were deprecated in jQuery 1.8 in favor of the promises API, and may be removed in a future version. I agree Backbone should generally keep its API surface small, but I think this solution is much more elegant when dealing with these async actions. Checking for a false return value separate from a server / network failure just feels like a kludge.

To @braddunbar's point, adding extra stuff is always a slippery slope, but the advantage is this opens Backbone up to do a lot better with success callbacks along the Backbone.sync path (no more var success = options.success cascades).

Zepto support is easily shimmed with Simply Deferred.

edit: clarified callbacks wording. The methods on jqxhr are deprecated, not the passed-in options.

@jashkenas
Copy link
Owner

Closing this particular PR. I'd still be happy to look at and discuss a more holistic approach to pervasive promises, in concert with a version bump ... but it looks like this patch is incomplete.

@jashkenas jashkenas closed this Oct 10, 2013
@akre54
Copy link
Collaborator Author

akre54 commented Oct 11, 2013

It seems like the best solution in the interim is to wrap all save/destroy calls with Q/when/etc. then, right? That is, if you cared about promise correctness you probably wouldn't be using jQuery's promises in the first place...

@Artazor
Copy link

Artazor commented Apr 29, 2015

I still think that this PR is independent of #3582
The only thing that we should agree that
save and destroy should always return thenables.

Thus, if we combine this PR and #3582 it will look like:

Backbone.newDestroyed = function(model) {
    return Backbone.Deferred().resolve(model);
}

Backbone.notValidated = function(model) {
    return Backbone.Deferred().reject(model.validationError);
}

There also a question: should the obtained xhr be also wrapped into Backbone.Defered().resolve(xhr)? To unify all thenables?

@jridgewell
Copy link
Collaborator

I see this as solving the root problem brought up by #3582. Additionally, it only adds one method onto the Backbone namespace, and it's a well-defined class (deferreds are easy to create from Promise libraries).

There also a question: should the obtained xhr be also wrapped into Backbone.Defered().resolve(xhr)? To unify all thenables?

So that all thenable return values are the same class, yes we should. Imagine overriding Backbone.Deferred to use native promises:

Backbone.Deferred = function() {
    var deferred = {};
    deferred.promise = new Promise(function(resolve, reject) {
        deferred.resolve = resolve;
        deferred.reject = reject;
    });
    return deferred;
};

I'd hate for some return values to be jQuery promises and some to be native promises.


As a side note, I don't think we should return deferred.resolve(). #resolve and #reject return a non-standard value (the deferred, vs undefined), and that the deferred object has #then is non-standard.

@Artazor
Copy link

Artazor commented Apr 29, 2015

So why Deferred? Why not Backbone.Promise?
And use pretty sandard Backbone.Promise.resolve(value) and Backbone.Promise.reject(reason)
?

There is widespread belief that you should avoid using deferreds at all.

@jridgewell
Copy link
Collaborator

So why Deferred? Why not Backbone.Promise?

Because jQuery provides a Deferred class, so we don't have to write a ton of extra code.

There is widespread belief that you should avoid using deferreds at all.

Hm?

@jashkenas
Copy link
Owner

Opening for more discussion ... but we'd still need a real patch+proposal.

@jashkenas jashkenas reopened this May 13, 2015
@jashkenas jashkenas removed the wontfix label May 13, 2015
@Artazor
Copy link

Artazor commented May 13, 2015

I would strongly advocate for the Backbone.Promise that should provide only two static methods resolve and reject

declare module Backbone {
    ...
    export interface Thenable<T> {
        then<U>(
             fulfilled: (result: T) => U | Thenable<U>,
             rejected: (reason: any) => U | Thenable<U>
        ): Thenable<U>
    }
    ...
    export class Model<TSync> {
          save(...): Thenable<TSync>
          fetch(...): Thenable<TSync>
          destroy(...): Thenable<TSync>
          sync(...): Thenable<TSync>
    }

    export class Collection<TModel extends Model, TSync> {
          fetch(...): Thenable<TSync>
          sync(...): Thenable<TSync>
    }

    export function sync<TModel, TOpts, TSync>(
          method: string, 
          model: TModel, 
          options: TOpts
     ): Thenable<TSync>

    export class Promise {
         // lets rely only on those two static methods:
         static resolve<T>(value: T | Thenable<T>): Thenable<T>;
         static reject<T>(reason: any): Thenable<T>;
    }
}

Inside you can use promises created by jQuery.Deferred by default or the same jqXHR
but Backbone itself should rely only on two standard static methods that are placed under Backbone.Promise: resolve and reject.

My motivation:

  • Those who never want to override methods will get the same jqXHR and JQueryPromises by the default implementation. Note that we do not need to implement promises in Backbone, we will reuse jQuery Deferred.promise() or similar things
  • Those who wnat to override this, probably want to use Promises/A+ implementation and would be able to do it with single:
    Backbone.Promise = MyFavoritePromiseClass

@akre54
Copy link
Collaborator Author

akre54 commented May 15, 2015

I agree fully with @Artazor here (and thanks for the detailed interface). Do you propose we wrap sync (or ajax) in Backbone.Promise or should the implementing user's ajax method be required to return a thenable?

It'd be awesome to see a pull with this implementation if you have time.

jridgewell added a commit to jridgewell/backbone that referenced this pull request May 31, 2015
Competes with jashkenas#2489.

Specifically, is implements `Backbone.Promise` instead of
`Backbone.Deferred`, so it can be easily swapped with any ES6
compatible Promise library.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants