Returning uncatchable promises


(Sam Saffron) #1

Internally we follow a pattern similar to this quite extensively in our models:

 toggleBookmark() {
    const self = this, firstPost = this.get("postStream.posts")[0];
    var path = self.get('bookmarked') ? '/remove_bookmarks' : '/bookmark';

    this.toggleProperty('bookmarked');
    if (this.get("postStream.firstPostPresent")) { firstPost.toggleProperty("bookmarked"); }

    return Discourse.ajax('/t/' + this.get('id') + path, {
      type: 'PUT',
    }).catch(function(error) {
      self.toggleProperty('bookmarked');
      if (self.get("postStream.firstPostPresent")) { firstPost.toggleProperty('bookmarked'); }

      let showGenericError = true;
      if (error && error.responseText) {
        try {
          bootbox.alert($.parseJSON(error.responseText).errors);
          showGenericError = false;
        } catch(e){}
      }

      if(showGenericError){
        bootbox.alert(I18n.t('generic_error'));
      }
    });
  },

This raises a few red flags on my side which I would like to have a plan to address

Uncatchable promises

Consumers calling

post.toggleBookmark().catch(function(){
   // rollback UI
});

Are in for a rude surprise when they discover the catch code never fires.

UI is being triggered from models

The model has a dependecy on bootbox, which seems odd, view logic is creeping in to models.


How should we address these issues?

cc @eviltrout @zogstrip

On the “uncatchable promise thing”

Should we be throwing from inside the “catch” so stuff leaks out?

It just feels like we are setting us up with a models that do not allow the UI to respond to failure cleanly at the point of failure.


(Sam Saffron) #2

Had a quick chat with @wycats and the first recommendation here is to simply be rethrowing:

So:

return Discourse.ajax('/something').catch(function(error){
   // internal cleanup
   throw error;
}

This gives a chance to consumers to handle errors.


(Jacob Chapel) #3

I feel that UI code in a model is a recipe for trouble. That logic should live in a View, then that makes the call to the model’s method.

As far as uncatchable, while not a fan of promises, re-throwing seems to be one of the only options if you want to expose the error outside. If a consumer down the line doesn’t catch it, it will be ignored, so it should be safe as far as I can tell.


(RĂ©gis Hanol) #4

Yeah I agree that it’s not good. We should really only do the ajax request in the model and let the view handle the bootboxes.


(Kane York) #5

I actually have some code on my computer for a generic error modal view. It would take the jQuery XHR object, which is what the catch blocks get.


(Robin Ward) #6

The actual problem here, is what @zogstrip said that the model is doing too much. A refactor to make sure it’s only doing AJAXy/model stuff is the correct fix. The model should raise an error if something happens, the UI layer should do something about it.

If we want to “rethrow” as @wycats mentioned we can throw a new exception. Another alternative is to return a new broken promise. I made a source comment about this not that long ago.