after adding a listener to a Promise, should I use the original promise or the new one?

Question

I have some javasript code that takes an existing promise (say, the promise returned by fetch()) and adds value (say, then/catch listeners for debugging, or maybe more):

let myFetch = function(url) {
  return fetch(url).then(function(value) {
    console.log("fetch succeeded: value=",value);
    return value;
  }.catch(function(reason) {
    console.log("fetch failed: reason=",reason);
    throw reason;
  });
};

I found myself modifying the above code so that the listeners are added only if some condition is true:

let myFetch = function(url) {
  let promise = fetch(url);
  if (some condition) {
    promise = promise.then(function(value) {
      console.log("fetch succeeded: value=",value);
      return value;
    }.catch(function(reason) {
      console.log("fetch failed: reason=",reason);
      throw reason;
    });
  }
  return promise;
};

Now I'm wondering, does it really make sense for myFetch to return the new promise returned by "then" (actually catch which is shorthand for another "then") as above, or would it make more sense for it to return the original promise (with the added listeners)? In other words, I'm thinking of leaving out the second "promise =", so that the code will look like this instead:

let myFetch = function(url) {
  let promise = fetch(url);
  if (some condition) {
    promise.then(function(value) {
      console.log("fetch succeeded: value=",value);
      return value;
    }.catch(function(reason) {
      console.log("fetch failed: reason=",reason);
      throw reason;
    });
  }
  return promise;
};

Is that effectively different from the previous version? Is either version preferable, and if so, why?

Answer 1

Well, if your success handler returns the value and your rejection handler throws the error - then it is basically the identity transformation for the promise.

Not only do you not need to do that promise = promise.then you don't even need to return the values:

let myFetch = function(url) {
  let promise = fetch(url);
  if (some condition) {
    promise.then(function(value) {
      console.log("fetch succeeded: value=",value);
    }.catch(function(reason) {
      console.log("fetch failed: reason=",reason);
    });
  }
  return promise;
};

That said, if you're using ES6 and let, you can use arrow functions which makes this a little nicer anyway:

let myFetch = function(url) {
  let promise = fetch(url);
  if (some condition) {
    promise.then(value => console.log("fetch succeeded: value=",value))
          .catch(reason => console.log("fetch failed: reason=",reason));
  }
  return promise;
};

Some promise libraries like bluebird provide a tap utility for this. The only problem is that if ever fetch adds support for promise cancellation, you are breaking the chain with the if (some condition) handler if you're not chaining it.

Answer 2

If your only use case is logging something in then/catch – it shouldn't matter as long as everything goes well. Things get more messed if you get an exception. Consider these two examples:

Return original promise

function myFetch() {
    let promise = new Promise(function (resolve, reject) {
        resolve(100);
    });
    promise.then(function () { throw new Error('x'); });
    return promise;
}

myFetch().then(function () {
    console.log('success!');
}).catch(function (e) {
    console.error('error!', e)
});

The result is success and the error thrown in the inner then might get swallowed in some promise libraries (although the most popular ones such as Bluebird handle this and you get additional error Unhandled rejection Error: x). The error might also get swallowed when using native Promises in some environments.

Returning modified promise

function myFetch() {
    let promise = new Promise(function (resolve, reject) {
        resolve(100);
    });
    promise = promise.then(function () { throw new Error('x'); });
    return promise;
}

myFetch().then(function () {
    console.log('success!');
}).catch(function (e) {
    console.error('error!', e)
});

Now the result is error! Error: x.

Answer 3

You're promise branching. In the second case, you're effectively branching the promise chain into two promise chains, because once a caller calls myFetch:

myFetch("localhost").then(request => { /* use request */ } );

then promise will have had .then called on it twice (once inside myFetch to do the console logging, and again here).

This is fine. You can call .then on the same promise as many times as you like, and the functions will execute together in the same order whenever promise resolves.

But, importantly, each function represents a branch off of the original promise, independent of every other branch. This is why you don't need to return or rethrow anything after your console.log: Nobody's listening on that branch, specifically, the caller of myFetch is not affected.

This is a good fit for logging IMHO, but there are subtle timing and error handling differences to be aware of when doing anything more:

var log = msg => div.innerHTML += msg + "<br>";

var myFetch = url => {
  var p = Promise.resolve({});
  p.then(() => log("a")).then(() => log("b"));
  return p;
}

myFetch().then(() => log("1")).then(() => log("2")).catch(log); // a,1,b,2

<div id="div"></div>

This emits a,1,b,2. As you can see, there are two chains going on here, advancing in parallel. It makes sense when you think about when promise is resolved, but it can be surprising.

The other subtlety is that error handling is also per branch (one branch will never fail another). In fact, the above code has a bug. Did you spot it? There should be a catch after .then(() => log("b")), or errors about anything you do in that branch end up unhandled or swallowed in some environments.