Promises are "awkward"


(Sam Saffron) #1

I just implemented proper “save” functionality for Groups and wanted to report back some awkwardness around our API that I think should be addressed, the code in question is:

 save: function(){
    var group = this;
    group.set('disableSave', true);

    return Discourse.ajax("/admin/groups/" + this.get('id'), {
      type: "PUT",
      data: {
        group: {
          name: this.get('name'),
          usernames: this.get('usernames')
        }
      },
      complete: function(){
        group.set('disableSave', false);
      }
    }).then(null, function(e){
      var message = $.parseJSON(e.responseText).errors;
      bootbox.alert(message);
    });
  }

My points of contention:

There is no way to specify a “always” condition:

In jQuery you get always, done and fail : Deferred Object | jQuery API Documentation

This means that if you want to pass in something that happens regardless of “failure” or “succees” you can type:

promise.always(function(){ \* gets called always *\ });

There is no way to untangle “success” and “failure” callbacks.

In jQuery you can do:

promise.fail(function() { \* only called on fail *\ });

\\ in ember we have the awkward
promise.then(null,function(){\* only called on fail*\});

(our Discourse.ajax implementation): the whole parseJSON crap above should be handled automatically.

We can transparently handle errors with hooks for overriding the bootbox stuff. Our error generation on the server is more or less consistent, handling on the client should be automatic.


Raised this on Github as well

@eviltrout , @Neil thoughts ?


(Robin Ward) #2

The Promises spec (now A+) has gone through so many revisions and scrutiny I doubt they’d add the similar callbacks at this point to RSVP as they are aiming for standards compliance.

We could easily add our own equivalent to fail though, which would just be an alias for (null, func), so I think that problem’s solvable.

The always is a bit messier but we can do the following workaround in this case:

  save: function(){
    var group = this;
    group.set('disableSave', true);
    var enableSave = function() {
       group.set('disableSave', false);
    }

    return Discourse.ajax("/admin/groups/" + this.get('id'), {
      type: "PUT",
      data: {
        group: {
          name: this.get('name'),
          usernames: this.get('usernames')
        }
      }
    }).then(enableSave, function(e){
      enableSave();
      var message = $.parseJSON(e.responseText).errors;
      bootbox.alert(message);
    });
  }

As for the JSON parsing of errors and automatic bootboxing, I agree completely. It’s something I’ve wanted to do since I converted all the AJAX calls to use Discourse.ajax. It should be done, it’s just technical debt!


(Neil Lalonde) #3

It’s suprising that A+ doesn’t have a way to do always/complete, while it has added any, every, etc. consider implementing Promise#every Promise#some Promise#any Promise#catch · Issue #87 · tildeio/rsvp.js · GitHub

If we could pass an array of functions, it might improve the API (or make it even uglier).

    return Discourse.ajax("/admin/groups/" + this.get('id'), {
      type: "PUT",
      data: {
        group: {
          name: this.get('name'),
          usernames: this.get('usernames')
        }
      }
    }).then(enableSave, [enableSave, function(e){
      var message = $.parseJSON(e.responseText).errors;
      bootbox.alert(message);
    }]);

Meh.

I seem to remember that the response will get parsed as JSON automatically if you put dataType: 'json' in the $.ajax call. I’ll have to try.


(Domenic) #4

(EDIT: oh damn, I just realized this post is many months old; sorry for resurrecting it.)

There seem to be a few misconceptions about promises, the A+ spec, and RSVP here…

The A+ spec only specifies then. Nothing else. Not even how to create promises.

Libraries are free to add sugar methods, and indeed are encouraged to do so. RSVP is a pretty bare-bones library in terms of adding sugar methods, and hasn’t really done a very good job of presenting a nice API to consumers, but it has more than just .then.

In particular, RSVP already has .fail, although .catch is a better name (and will match the upcoming ES6 built-in promises): tildeio/rsvp#111. I am not sure why you guys do not use .fail. (EDIT: probably it did not exist 4 months ago.)

A .finally method is also a great addition for any library, and most promise libraries have such a method. RSVP appears to not have it, but that’s just an omission. However, finally should not behave like .then(cb, cb) as jQuery’s does. This breaks the sync-async parallel. Instead, it should work as specified in domenic/promises-unwrapping#18. See reasoning there.

Also, since you guys are handling the error and not propagating it, you don’t even need to repeat the code, just put the enableSave in the success handler:

return Discourse.ajax(...).fail(function (e) {
  bootbox.alert(JSON.parse(e.responseText).errors);
})
.then(enableSave);

Since you do not re-throw the error in the rejection handler, the returned promise is fulfilled, so enableSave will be called. (Unless bootbox.alert throws an error, of course.)


(Sam Saffron) #5

One huge pain point I have seen is debugging promises, in this regard GitHub - petkaantonov/bluebird: Bluebird is a full featured promise library with unmatched performance. is … wait for it … promising.

Has there been any work in RSVP in this regard, does Ember want to be agnostic about the promises library it uses? cc @wycats @tomdale