Prefer throw or reject when failing promise asynchronously

Question

I have a Bluebird promise that wraps an AJAX request and need to reject the promise when the request fails. I would like to provide the reason why the request failed, largely drawn from the status code, to any catch blocks that may be attached. To achieve that, I have UnauthorizedError and NotFoundError and similar classes, all extending Error to work with Bluebird's pattern-matching catch.

The part I'm not sure on is whether I should throw or call the reject handler. My code looks something like:

class Request {
  // other methods

  send(method, url, args, body) {
    return new Promise((res, rej) => {
      let xhr = new XMLHttpRequest();
      xhr.open(method, url);

      xhr.onload = () => {
        res(JSON.parse(xhr.responseText));
      };

      xhr.onerror = () => {
        let status = xhr.status;
        switch (status) {
          case 401:
            // Should I use throw:
            throw new UnauthorizedError(url);
            // or
            rej(new UnauthorizedError(url));
        }
      };

      xhr.send();
    });
  }
}

score 6 · Accepted Answer · edited Jun 20 '20 at 09:12

Inside the `Promise` constructor

The promise constructor is throw safe, but by nature you're typically not dealing with throw safe things inside - so for example the following is not safe:

new Promise(function(resolve, reject){
     setTimeout(function(){
         // NEVER throw here, it'll throw globally and not reject the promise
     }, 100);
});

The promise constructor is typically only used to convert callback APIs to promises and since callbacks are not throw-safe like promises you have to reject when they error asynchronously (rather than throw).

Inside a `then` handler

The two are identical functionally. When you throw from a then handler you're returning a rejected promise. I prefer to throw because it is more explicit that an error happened than a return but it does not matter.

This is true for any promise implementation except Angular 1.x's $q which distinguishes the two - but it's the odd ball out (when you throw there it logs even if you handle the error).

In your code

In your code there are several bugs in the way you reject and handle the promises. Promises are very robust in that they handle errors gracefully for you - you have to be extremely careful when you convert callback APIs to promises in that regard:

class Request {
    // other methods

    send(method, url, args, body) {
        return new Promise((res, rej) => {  // it's good that new Promise is the first
            let xhr = new XMLHttpRequest(); // line since it's throw-safe. This is why it
            xhr.open(method, url);          // was chosen for the API and not deferreds

            xhr.onload = () => {
                // This _needs_ a try/catch, it will fail if responseText
                // is invalid JSON and will throw to the global scope instead of rejecting
                res(JSON.parse(xhr.responseText));
            };

            xhr.onerror = () => {
                let status = xhr.status;
                switch (status) {
                case 401:
                    // this _must_ be a reject, it should also generally be surrounded
                    // with a try/catch
                    rej(new UnauthorizedError(url));
                }
            };

            xhr.send(); // it's important that this is in the promise constructor
        });
    }
}

What should be in the try/catch for `JSON.parse`? `reject(new ParseError());`? — Florian Margaine, May 29 '15 at 14:35
@FlorianMargaine even just a `try{ res(JSON.parse(...)); } catch(e){ rej(e); }` is fine - as long as it's a promise rejection and not a "wild" error. — Benjamin Gruenbaum, May 29 '15 at 14:36
Why do we need to surround the rejection handler with `try..catch`? — thefourtheye, May 29 '15 at 14:41
Well, `xhr.onerror` might execute asynchronously - at which point it's not executing inside the promise constructor so a thrown exception could not possibly be caught by it. This is exactly like the `setTimeout` example - you can't catch errors in async functions you don't control. You must manually convert exceptions to rejections which is normally what the promise library does for you. — Benjamin Gruenbaum, May 29 '15 at 14:42
Another option would be to have onload resolve and onerror reject immediately, and then use a `.then(xhr => JSON.parse(xhr.responseText), err => { if (status === 401) throw new UnauthorusedError(url); })`. Then you don't have to worry about doing your own try/catch. Just make sure the only action you do in async callbacks is calling `resolve` or `reject`. — loganfsmyth, May 29 '15 at 17:15

Prefer throw or reject when failing promise asynchronously

1 Answers1

Inside the Promise constructor

Inside a then handler

In your code

Inside the `Promise` constructor

Inside a `then` handler