Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Conditional/Branching construct #1454

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

justsml
Copy link
Contributor

@justsml justsml commented Sep 14, 2017

This is a proposed change related to #984
And my comments/examples here: #984 (comment)

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
Copy link
Contributor Author

justsml commented Sep 14, 2017

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

@spion
Copy link
Collaborator

spion commented Sep 14, 2017

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

@justsml
Copy link
Contributor Author

justsml commented Sep 14, 2017

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
Copy link
Contributor Author

justsml commented Sep 14, 2017

@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
Copy link
Contributor Author

justsml commented Sep 14, 2017

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:https://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

@spion
Copy link
Collaborator

spion commented Sep 15, 2017

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.

@justsml
Copy link
Contributor Author

justsml commented Sep 15, 2017

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:https://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
Copy link
Contributor Author

justsml commented Sep 16, 2017

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
Copy link
Contributor Author

justsml commented Sep 17, 2017

@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!

@jonathanherring
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants