bluebird icon indicating copy to clipboard operation
bluebird copied to clipboard

Add Conditional/Branching construct

Open justsml opened this issue 6 years ago • 10 comments

This is a proposed change related to #984 And my comments/examples here: https://github.com/petkaantonov/bluebird/issues/984#issuecomment-328633367

API

Promise.thenIf(condition(value), ifTrue(value), ifFalse(value))

Defaults

  • condition, echo/truthy function: (x) => x
  • ifTrue, echo function: (x) => x
  • ifFalse, quiet function: () => null

The condition function should return either true/false or a promise that resolves to something true/false.

ifTrue function is called if the condition resulted in a truthy value. Conversely, ifFalse will be called if we got a false answer.

The return value of either ifTrue/ifFalse handler will be handed to the next .then().

Usage

// Use like so:
const checkEmail = email => Promise.resolve(email)
  .thenIf(
    e => e.length > 5,              // Conditional
    e => console.log('Valid: ', e), // ifTrue
    e => console.error(e))          // ifFalse
// Or like so:
const checkEmail = email => Promise.resolve(email)
  .thenIf(e => e.length > 5)
// Or:
const checkEmail = email => Promise.resolve(email)
  .then(Promise.thenIf(e => e.length > 5))

Lots of cool stuff is possible, the default values let you call .thenIf with no args - if you simply want to exclude falsey values down the chain.

justsml avatar Sep 14 '17 19:09 justsml

Does this make sense? Here's what I get running it locally: image

Tests are passing in all my local tests for node v6-v8. image

justsml avatar Sep 14 '17 19:09 justsml

As in the closed issue, usually we recommend writing a then combinator for these cases:

Promise.resolve(email)
.then(iff(e => e.length > 5,         // Conditional
    e => console.log('Valid: ', e),  // ifTrue
    e => console.error(e)))          // ifFalse

Main benefit being that this iff combinator will work with any promise library and can therefore be distributed and used outside bluebird

spion avatar Sep 14 '17 20:09 spion

I support that pattern (exposed as Promise.thenIf) - I've been using it in almost all my projects for a few months now. Considering the array methods, I feel branching is a natural addition.

Mainly I hate having to patch bluebird with my util/helpers. (tired of disclaiming - "don't ever normally hack onto native prototypes...") You might ask, Well then, "why?" Is this scope creep?

I'd say bluebird is already a batteries-included library. Adding something that seems more relevant than .delay() is at least worthy of some more consideration.

justsml avatar Sep 14 '17 21:09 justsml

@spion that is really close, but if I wanted to write my function chains that way I'd use https://github.com/sindresorhus/ (still awesome) 1000 mini promise libs. 😉

justsml avatar Sep 14 '17 21:09 justsml

Here's the ES6 I'm working off, hopefully easier to read/review:

module.exports = function _conditionals(Promise) {
  Promise.prototype.thenIf  = thenIf
  Promise.thenIf            = _thenIf

  function thenIf(cond = (x) => x, ifTrue = (x) => x, ifFalse = () => null) {
    if (this instanceof Promise) {
      return this.then(value => _thenIf(cond, ifTrue, ifFalse)(value))
    } else {
      return _thenIf(cond, ifTrue, ifFalse)
    }
  }

  function _thenIf(cond = (x) => x, ifTrue = (x) => x, ifFalse = () => null) {
    return value => Promise.resolve(cond(value))
      .then(ans => ans ? ifTrue(value) : ifFalse(value))
  }
}

http://babeljs.io/repl/#?babili=false&browsers=&build=&builtIns=false&code_lz=LYewJgrgNgpgdDAHgBxAJwC4GcAEBeHAMwgDsBjDASxBJwH0yaxKqaBDKLACgAU0RglLDACUOAN4AoHDj4Ch8ZPwwgMAT2TwMACxgkAkoRkEdew9Nn9BwuKYNGZjp8fp3zF4uVa03hLoxIwfBwuRDE8AD4cRAAaHEpCABU0CBhg0PCo2PjCADEOYXTMnBJoKDEpJwSQnSF4kiwMNnIYECM5a1EJCyc0GAwINB9tIVtdEi4ANw5U_Ci6X38mOITk1JW8gtEpmdERHpwAXxwYTjTK5z6BoddxwyXAjbWYDfyz_adDiy-PUgpqWgLO5-AJBAgZObRJ4pNLgsKQ7IJN6FcHFUpQcrdXr9Qa0aZQWaRSzyGx9LAgKCTGAPMA7Al7A4yMZ6LjNXBEtk4AD8OWedNSYgAXDlkdT8QKPkdJF8gA&debug=false&circleciRepo=&evaluate=false&lineWrap=false&presets=es2015%2Ces2017%2Creact%2Cstage-2%2Cstage-3&prettier=true&targets=&version=6.26.0

justsml avatar Sep 14 '17 21:09 justsml

Bluebird's external API is currently closed-by-default, unless there is a significant new construct that provides important new functionality (e.g. using, streams, etc). API that can be achieved by adding a combinator is generally not added anymore since we find that the large API surface area burden on users outweighs the gain (users look at the documentation, see the billion methods and give up immediately)

This sucks, but unfortunately JS's OO flavour doesn't make it easy to separate extensions from a library, so either everyone is burdened with a large surface area, or noone gets all the features they want. Maybe the bind operator would make things better, if only TC39 got their act together in pushing it through.

@benjamingr @petkaantonov correct me if I'm wrong.

spion avatar Sep 15 '17 10:09 spion

Thanks @spion - agreed, bind::can't come soon enough.

I understand the criticism of bluebird is often about bloat. To me, this is because Bluebird was used primarily as a browser polyfill for years. Most people only used .then() - and from that perspective the API looks crazy bloated. There are plenty of tiny Promise polyfills people should use if that's all they need.

Bluebird is special because of it's inclusive API - it's more amazing that it's only a few dozen KB (and in the age of 3MB JS bundles... not bad).

To me, the value of the inclusive API becomes clear in comparison to RxJS' patterns. Essentially with Bluebird I get 80% of the chaining power of RxJS with A FAR LESS intimidating API.

... And if I add some EventEmitter code, i'll be at 90% of RxJS. 😀 :trollface:


Speaking of scaring users off... Can we remove the blank placeholder pages: http://bluebirdjs.com/docs/beginners-guide.html ? They've had cobwebs for years.

Btw, the API reference page is an excellent, simple & clear landing page.

justsml avatar Sep 15 '17 15:09 justsml

I keep finding examples where .tapIf()/.thenIf() would clean up code very nicely IMHO (by eliminating extra wrapping clousures/arrow funcs.)

const ensureDirectory = (file) => Promise
  .resolve(path.join(__dirname, '..', file.category))
  .tapIf(fs.existsAsync, fs.mkdirAsync) // <-- oh daayyyum!!!
  .then(() => file) 
// Will create folder path (if needed), then return the input `file`

justsml avatar Sep 16 '17 02:09 justsml

@spion Regardless of the feature relevance, I'd really appreciate some help figuring out the error in tests, late_buffer_safety.js throw's integers, I thought that wasn't supported? 😕

And how on earth does my code affect this unrelated file? 😖

Thanks!

justsml avatar Sep 17 '17 22:09 justsml

Seems like a good way to easily add if/else conditions. 👍

jonathanherring avatar Oct 03 '17 19:10 jonathanherring