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

Question: why async and not Promises? #67

Closed
rstacruz opened this issue Jan 22, 2016 · 13 comments
Closed

Question: why async and not Promises? #67

rstacruz opened this issue Jan 22, 2016 · 13 comments

Comments

@rstacruz
Copy link
Contributor

It seems promises will make a lot of the internal code much easier to work with, with tools like serial and waterfall coming in for free.

@rstacruz
Copy link
Contributor Author

Example:

// install_cmd.js
  async.series([
    mkdirp.bind(null, path.join(cwd, 'node_modules', '.bin')),
    mkdirp.bind(null, path.join(config.cacheDir, '.tmp')),
    function (cb) {
      var installAll = async.map.bind(null, deps, function (target, cb) {
        var name = target[0]
        var version = target[1]
        install(node_modules, name, version, function (err, pkg) {
          cb(err && err.code === 'LOCKED' ? null : err, pkg)
        })
      })

      var exposeAll = expose.bind(null, node_modules)
      var waterfall = [ installAll, exposeAll ]

      if (argv.save) {
        waterfall.push(save.bind(null, cwd, 'dependencies'))
      }

      if (argv['save-dev']) {
        waterfall.push(save.bind(null, cwd, 'devDependencies'))
      }

      async.waterfall(waterfall, function (err) {
        if (err) throw err
        cb()
      })
    }
  ], cb)

Can be rewritten something like so:

Promise.resolve()
  .then(_ => mkdirp(path.join(cwd, 'node_modules', '.bin')))
  .then(_ => mkdirp(path.join(config.cacheDir, '.tmp')))
  .then(_ => installAll(node_modules, deps, _))
  .then(_ => exposeAll(node_modules, _))
  .then(_ => saveDeps(argv, _))
  /* might want to use `.bind` equivalents instead to support node 0.11 */

function installAll (node_modules, deps) {
  return Promise.all(deps.map(function (dep) {
    return install(node_modules, dep)
      .catch(function (err) {
        if (err.code === 'LOCKED') return err.pkg
        throw err
      })
  }))
}

function saveDeps (argv, result) {
  if (argv.save) {
    return save.bin(null, cwd, 'dependencies', result)
  } else if (argv['save-dev']) {
    return save.bin(null, cwd, 'devDependencies', result)
  } else {
    return result
  }
}

@alexanderGugel
Copy link
Owner

No. I don't like promises. Sorry. Spoiler alert: I'm working on a partial rewrite of ied-install in rx though (more like a PoC). async is going to go away, but I just don't like Promises. Sorry 😞

@just-boris
Copy link
Collaborator

@alexanderGugel well, look forward to see your results then.

@rstacruz
Copy link
Contributor Author

curious, care to share why the dislike for promises?

@alexanderGugel
Copy link
Owner

taste

@alexanderGugel
Copy link
Owner

ahh whatever. I'm not religious about it. If you feel strongly about it, feel free to send a PR. Happy to merge if it simplifies stuff.

@mstade
Copy link

mstade commented Mar 15, 2016

This is just noise, but to give at least one reason why promises are unpalatable to some (including yours truly) is that if errors aren't handled you just won't see them, and then waste time chasing red herrings. It's also not always clear how control flows through handlers, and if you forget to return proper values from handlers you may just end up not propagating values that really should have been. Promises being async by spec as well makes it difficult to properly trace execution, since you lose the stack.

There's a lot of apparent magic in promises and using them for control flow, as opposed to the original intent – the promise of a future value, where syntactically they have limited purpose in current javascript, case in point: async/await which hides them entirely – is likely going to cause more harm than good, unless you're intimately familiar with the pitfalls.

@STRML
Copy link

STRML commented Mar 23, 2016

Bluebird catches uncaught errors, and in combination with async/await leads to really nice, idiomatic code.

@just-boris
Copy link
Collaborator

If you are using async library, you will get usual exception.

Unlike promises, async callbacks are not wrapped in try/catch, so you don't need do anything special here. For me, it looks like advantage of async, not promises

@STRML
Copy link

STRML commented Mar 23, 2016

Automatic try/catch is a feature, not a bug, intended to prevent unintentional error swallowing if you forget to write yet another if (err) return cb(err), or crashing if you truck right along expecting a value. They instead bubble up to the nearest .catch() handler, or are logged. It's easy to set a policy to exit the process at any uncaught exception as well.

It appears moot anyhow as there's an open RxJS rewrite PR.

@alexanderGugel
Copy link
Owner

@STRML Yup. RxJS and observables are the future :). I don't really want to start a massive promise vs no promises discussion TBH. I don't want to use them. Reasons:

  1. They don't really simplify control flow IMO (compared to async).
  2. If you have super nested callbacks, something is wrong with your code in the first place IMO
  3. Name your callbacks, split them out and they're already more readable.
  4. Callbacks are easier to explain to people (I actually made the experience with a couple of grads that it's much easier for people to understand and explain Observables then promises). This is an important selling point IMO.
  5. Adding another promise library (such as Bluebird) as a dependency should be avoided if possible.

That being said, I don't feel religious about it, but I just don't see the need to use promises TBH.

@STRML
Copy link

STRML commented Mar 24, 2016

@alexanderGugel I'll support RxJS, I've been interested in figuring out what the fuss is about. Happy to help you finish the conversion. Please let me know what still needs addressing.

My hunch is that we don't really need the kind of event-based flow that RxJS provides, and that the simpler flow Promises handle (single action flowing through a number of async functions) is fine. But there's only one way to be sure, and that's to finish the rewrite.

Once the RxJS rewrite is finished and Babel is in master it would be trivial to rewrite the async parts to use async/await if we determine it's simpler.

@alexanderGugel
Copy link
Owner

RxJS rewrite complete and merged into master. 🎆

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

No branches or pull requests

5 participants