tap on a rejected promise triggers unhandled promise rejection

[TheSpyder]

Replication code:

open Promise.Js;
rejected("my error")
->tap(Js.log2("debug print", _))
->catch(e => {
    Js.log2("regular error handling:", e);
    resolved();
  })
->get(_ => ());

This prints regular error handling but also triggers an unhandled promise rejection.

The cause, as far as I can tell, is that tap forks the promise and moves the callback out of the error handling chain: https://github.com/aantron/promise/blob/master/src/js/promise.re#L122-L125

I think the implementation should use map directly, instead of get, executing the callback and returning the same value. This keeps the tap side effect in the same chain as normal error handling.

  let tap = (promise, callback) =>
    map(promise, (v) => {
      callback();
      v
    });

[aantron]

@TheSpyder Thanks for the report and the suggestion. I've added a reproducing test case in https://github.com/aantron/promise/commit/356dd59e96732d293b1712b7e045012a6682abed, and your fix. It's now in npm as reason-promise 1.1.4. Apologies for the pretty big delay.

[aantron]

I updated only Promise.Js.tap, but not Promise.tap, Promise.tapOk, Promise.tapError, or Promise.tapSome. In an idealized type-safe world, these functions would never face promise rejections, and it might actually be good for them to cause rejection reports, if they do. Please let me know if you have any issues with that, and would like them changed, as well.

[TheSpyder]

Thanks! I was tempted to log a PR but I wasn't sure if I had the right idea :)

I think it's fine for this to only work with Promise.Js. Those are the only ones I feel the need to tap anyway, since I use them for bindings.