Bluebird: second argument for .tap or similar method for rejections

[Janpot]

I often use .tap(console.log) for quick debugging. It would be useful for me to have it either work like .then so I could do .tap(null, console.log) as well to log rejections.
Or to have a similar method that works like .tap but is analog to .catch as in .tapRejection(console.log).

[benjamingr]

I'm not against this idea but can you show an actual flow using it (with a real use case)?

[Janpot]

Oh it's just in a quick and dirty debugging flow. Just to find where stuff is failing.

function someAsyncFlow() {
  return doSomeAsyncStuff()
    .then(doSomeMoreAsyncStuff)
    .then(doEvenMoreAsyncStuff)
    .then(soMuchAsync);
}

If I know the result fails but can't exactly pinpoint where, I could just add a tap(null, console.log) in between and move it up and down between lines to quickly figure out which of the functions is failing.

function someAsyncFlow() {
  return doSomeAsyncStuff()
    .tap(null, console.log)
    .then(doSomeMoreAsyncStuff)
    // run this again then put the tap line here
    .then(doEvenMoreAsyncStuff)
    // run again and put it here
    .then(soMuchAsync);
}

or when I want to double check that the error I get somewhere is legit

[benjamingr]

@Janpot long stack traces already tell you where in particular the result fails. I get the point though.

[Janpot]

Sometimes it's also a matter of sanity checking whether a promise actually ends or not. I often have situations where I can just ignore all errors (by swallowing them in a .catch) but when nothing is happening while you expect something it could be a bug. In this case I often find myself writing code like:

.then(function (result) {
  console.log(result);
  return result;
}, function (err) {
  console.log(err.stack);
  throw err;
});

while I could be writing .tap(console.log, console.log) as well. I'm very much aware this is not the cleanest way to debug a program but for me this is often quick and effective enough.

[petkaantonov]

It's pointless to log an exception and rethrow it, if you don't catch the exception it will do that automatically.

[benjamingr]

@petkaantonov those were my initial thoughts but I can see a flow like:

makeRequest().
    tap(null, console.log).
    then(parseRequest).
    then(applyToDb).
    catch(NetworkError, err => retry(attempts-1));

Where you might care that the request failed but you handle and recover from it later so from the whole app point of view it's not a rejection.

[petkaantonov]

@benjamingr Well you are not rethrowing the error

[benjamingr]

Oh, I thought you were talking about the metod in general

[Janpot]

@petkaantonov Why is "logging an exception and rethrowing it" more pointless than "logging a result and returning it"? Which is what .tap(console.log) does. Often some exceptions are recoverable or can be ignored.

Suppose your method can be called from several places and you don't know exactly which one is failing. In this case you'll have to log from inside the method but want the app to execute normally.

// makeRequest is called from several places in your app.
function makeRequest(opts) {
  return request.getAsync(opts).tap(null, console.log);
}

[petkaantonov]

Why is "logging an exception and rethrowing it" more pointless than "logging a result and returning it"? Which is what .tap(console.log) does. Often some exceptions are recoverable or can be ignored.

Because return values are not logged automatically if you don't log them but thrown exceptions are. See http://stackoverflow.com/questions/6639963/why-is-log-and-throw-condsidered-an-anti-pattern

you don't know exactly which one is failing.

That's what the stack trace tells you, remember to enable long stack traces though.

[Janpot]

That's exactly the point, my upstream code is handling and ignoring all exceptions. I can add a console.log in this catch handler but in my case it is running several promise resolving methods simultaneously which means I could be logging 100+ exceptions / second. I have long stacktraces enabled but it still means I have to dig through massive amounts of logs to find my exception.
Instead I add a catch handler only to the method I'm interested in and just log it as a sanity check and let the app resume business as usual as to not interrupt it furthermore.

[petkaantonov]

So if I'm getting this right: you are editing code in production to add a quick console.log, instead of logging all exceptions automatically and just grepping your function name from them? And you want bluebird to endorse this practice by adding a feature into the core which makes an anti-pattern take 1 line instead of 3?

[Janpot]

Who talked about production code? And I wouldn't endorse this practice in production, it's just a convenience to make tap work more analog to then as I seem to use tis pattern in some occasions in my development.

[petkaantonov]

You said you get 100+ exceptions / second, that only makes sense if it's production.

[Janpot]

Although I have to admit my comparison with then doesn't completely hold as you can return a promise from .tap and I can't think of a parallel for the hypothetical second parameter of it

[petkaantonov]

Trivial helper features that make something slightly easier only make sense to add to the core if they are something that most users encounter frequently and are a good way to accomplish something. It doesn't seem like this is either of those.

[spion]

Shouldn't be too hard to make it work with .catch - just add or import this function:

function logRethrow(e) {
  console.log(e);
  throw e;
}

then you can use .catch(logRethrow) in your chain

[spion]

@petkaantonov this doesn't really increase the API surface (therefore reducing learnability) and is consistent with the behavior of .then (so again easier to learn). The code size increase is pretty small too. I think the cost to benefit ratio may be low enough for this one?

[benjamingr]

@spion to be fair that same argument can be made for tap:

function log(val){
   console.log(val);
    return val;
}

.then(log)

[petkaantonov]

The issue isn't with ease of implementation but how useful and commonly required it is, .thenReturn and .tap are both trivial to implement but everyone runs into them all the time in legit use cases. Adding methods just because it doesn't take a lot of code is not a good justification.

[spion]

If its just a second argument to .tap its not even a new method. Although personally I wouldn't mind just using a separate function myself.

[Janpot]

I agree with you @petkaantonov . I see it as a limitation of the expressiveness of current javascript. If you could just do .then(() => value) there was not really a need for .thenReturn(value).

@spion Although it's a trivial function to write I don't think it's a real time saver. In practice you would make this a method in a module so it means you'll have to add an extra require statement, probably at the top, maybe after installing your module with bluebird helper functions, scroll back to your code, add the code to call it. It's not that this is a lot of work it's just quicker to type the three lines instead.

Unless you do something like .then(null, require('bluebird-helpers').logThenThrow)

[jasonkarns]

I just found this issue by looking for a catch variant of tap. I'm +1 for tap taking a rejection handler as second arg.

[joepie91]

For anybody still looking for this: I've written a small module to do this kind of thing. It requires a wrapper function, though.

var Promise = require("bluebird");
var tapError = require("bluebird-tap-error");

Promise.try(function() {
    return someBrokenAsyncThing();
}).catch(tapError(function(err) {
    console.log("We saw an error!", err);
})).catch(function(err) {
    /* The actual error handling goes here. */
})

[scottrippey]

I'd like to reopen this, for sure. I have MANY blocks in production that are "manually" tapping (using .then( successTap & return, errorTap & throw)), and it's semantically inferior to .tap(successTap, errorTap).
Not only am I logging, but I've also got things like status flags to keep up-to-date.

[ajoslin]

I've also got a legitimate production usecase: maintaining the lifecycle of a transaction for running sql queries.

Currently I have to do this:

var transaction = sql.transaction()
return action1(transaction)
  .then(() => action2(transaction))
  .then(transaction.commit)
  .catch(function (error) {
    transaction.rollback()
    return Promise.reject(error)
  })

I would prefer to do this:

var transaction = sql.transaction()
return action1(transaction)
  .then(() => action2(transaction))
  .then(transaction.commit)
  .tap(null, transaction.rollback)

Of course, a tapError function would be even better.

[benjamingr]

@ajoslin use a disposer like pattenr:

function transact(actions) {
    var transaction = sql.transaction();
    return actions(transaction).then(v => {
        transaction.commit();
        return v;
    }, 
    e => { 
        transaction.rollback();
        throw e;
    });
}

Once, and then do:

return transac(t => 
    action1(t).then(() => action2(t))   
);

Which I think is generally nicer.

[ajoslin]

Fair, that is much better. Thanks.

[kara-ryli]

Came here from #1204 and wanted to re-post some use cases for this behavior:

The primary use case for this would be error cases that have side effects, or trigger behavior based on aggregate data that might succeed, or might fail with an error we want to ignore in the context of the promise chain.

Rate Limiting

Bluebird.
  try(logIn).
  then(respondWithSuccess).
  catch(InvalidCredentialsError, countFailuresForRateLimitingPurposes).
  catch(respondWithError);

Circuit Breakers

Bluebird.
  try(makeRequest).
  then(respondWithSuccess).
  catch(RequestError, adjustCircuitBreakerState).
  catch(respondWithError);

Logging

Bluebird.
  try(doAThing).
  catch(logErrorsRelatedToThatThing).
  then(respondWithSuccess).
  catch(respondWithError);

This case is useful when you're stuck using generic error types (e.g. just Error).

[spion]

These are valid use cases, its not only logging. I think we can reopen?

[benjamingr]

FYI https://github.com/petkaantonov/bluebird/pull/1220