alexanderGugel / ied

:package: Like npm, but faster - an alternative package manager for Node
http://alexandergugel.github.io/ied
MIT License
1.99k stars 53 forks source link

Question: why `async` and not Promises? #67

Closed rstacruz closed 8 years ago

rstacruz commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 :disappointed:

just-boris commented 8 years ago

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

rstacruz commented 8 years ago

curious, care to share why the dislike for promises?

alexanderGugel commented 8 years ago

taste

alexanderGugel commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

just-boris commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

RxJS rewrite complete and merged into master. 🎆