Make the EventEmitter returned by query() thenable

[felixfbecker]

There have been some discussions about promise support, but those are very old. It is pretty clear that the future is moving towards promises / ES7 async-await.
Wrappers like promise-mysql have the problem of not always being up-to-date, hiding the EventEmitter by returning a promise and not implementing all APIs (like pools).

I would propose to add a then() method to the EventEmitter returned by query() and other async functions that take a callback, therefor implementing the promise interface.
The old API would not break in any way, but it would allow users to await a query.
It could also be beneficial to add the 2 other standard promise functions, catch() and finally(), otherwise users would just have to wrap the call in Promise.resolve() if they want to use these features.

The callbacks passed to then() would be called like the callback. The error callback is called with the error, the success callback with an object {results, fields} just like the arguments for the callback are named in the docs.

The performance of this is negligible in the context of the IO roundtrips, as already mentioned here: #929. If a callback is passed, the functions should not return a promise/expose .then() so if you absolutely love callbacks you can use them without a performance impact.

A simple example: Adding a JSON object of a book with an array of authors into a db with transactions and proper error handling.

This is how we would write it today. Total callback hell.

let book = {name: 'test', authors: ['author1', 'author2']};
let db = pool.getConnection((err, db) => {
  if (err) {
    return req.statusCode = 500;
  }
  db.beginTransaction(err => {
    if (err) {
      req.statusCode = 500;
      return db.release();
    }
    db.query('INSERT INTO books (name) VALUES (?)', [book.name], (err, result) => {
      if (err) {
        req.statusCode = 500;
        return db.rollback(err => {
          db.release();          
        });
      }
      let bookId = results.id;
      let completed = 0;
      let errorHappened = false;
      for (let author of book.authors) {
        db.query('INSERT INTO book_authors (book_id, name) VALUES (?, ?)', [bookId, author], (err => result) {
          if (err) {
            req.statusCode = 500;
            errorHappened = true;
            return db.rollback(err => {
              db.release();              
            });
          }
          completed++;
          if (completed === book.authors.length) {
            db.commit(err => {
              if (err) {
                req.statusCode = 500;
              }
              db.release();              
            });
          }
        });
      }
    });    
  });
});

---

It gets slightly better with promises, because you get error handling and parallel execution for free:

let book = {name: 'test', authors: ['author1', 'author2']};
pool.getConnection()
  .then(db => db.beginTransaction())
  .then(db => 
    db.query('INSERT INTO books (name) VALUES (?)', [book.name])
      .then(answer => {
        let bookId = answer.results.insertId;
        return Promise.all(book.authors.map(author => db.query('INSERT INTO book_authors (book_id, name) VALUES (?, ?)', [bookId, author])))
      })
      .then(() => db.commit())
      .catch(error => db.rollback().then(() => Promise.reject(error)))    
      .finally(() => db.release())
  )
  .catch(error => {
    req.statusCode = 500;
  })

---

But it gets even better with ES7 (you can use this today either with Babel or with co by replacing all awaits with yield):

let book = {name: 'test', authors: ['author1', 'author2']};
try {
  let db = await pool.getConnection();
  await db.beginTransaction();
  try {
    let bookId = (await db.query('INSERT INTO books (name) VALUES (?)', [book.name])).results.insertId;
    await* book.authors.map(author => db.query('INSERT INTO book_authors (book_id, name) VALUES (?, ?)', [bookId, author]));
    await db.commit();
    req.statusCode = 201;
  } catch (error) {
    await db.rollback();
    throw error;
  } finally {
    db.release()
  }
} catch (error) {
  req.statusCode = 500;
}

---

You can achieve this with bluebird today, but in every file, every test you write, you have to do the following and also add an Async suffix to all methods:

let mysql = require('mysql')
let Pool = require('mysql/lib/pool')
let Connection = require('mysql/lib/connection')
Promise.promisifyAll(Pool.prototype)
Promise.promisifyAll(Connection.prototype)

To sum it up:

• add then() to the EventEmitter returned by query() and make all other async functions which don't return anything atm return a promise.
• Don't return a promise / add .then() when called with a callback.
• keep all the API the same.

I would make a PR implementing all of this. Thoughts?

[dougwilson]

Feel free to make a PR :) I only have three real requirements: it does not pull in a dependency, it's backwards-compatible, and it only implements the .then method as defined for thenables in the ECMA spec.

[felixfbecker]

Awesome, I'm fine with that. Thanks for the quick reply :)

[dougwilson]

No problem! I'm looking forward to the PR :) I've been meaning add thenable to this lib, but still have not gotten to, so your contribution will be much appreciated!

[felixfbecker]

One thing though, some async functions currently return nothing, like Connection.prototype.connect(). It would make sense to return a full promise there if no callback is passed, and I would like to use any-promise for this, so people can choose which promise library they want to use. Native promises are much slower currently than libraries like bluebird and bluebird for example also has some nice debug features ("possibly unhandled rejection: ...").
Is it ok if I add this dependency?

[dougwilson]

Looking at that dependency, it looks like it'll throw an error for all our Node.js 0.6 and 0.8 users, as simply requiring the library will throw by default on those platforms. It would also mean those users cannot call the .then method either, which needs to be possible even if there is no Promise library installed.

[felixfbecker]

This can easily be solved by doing:

try {
  var Promise = require('any-promise')
} catch (error) {}

We could add any-promise to optionalDependencies (because that's what it is - you can use promises but you don't have to). If we support node all the way back to 0.6 we have to check for the existence of any promise implementation anyway to not trigger a ReferenceError whenever someone calls something without a callback.

[felixfbecker]

But there needs to be some way to use a promise library instead of native, and any-promise is the most elegant imo.

[dougwilson]

As long as Node.js 0.6 users can do conn.connect().then(function () {}) without having any Promise library installed.

[dougwilson]

But there needs to be some way to use a promise library instead of native, and any-promise is the most elegant imo.

Why are we talking about promise at all? The only agreement here was that this library would return thenables, as defined by the ECMA spec. This library will not return promises, only thenables.

[felixfbecker]

I thought it would make sense to return a new Promise() for functions that currently return nothing, like connect(). But I'm okay with thenables in these cases too.

[dougwilson]

I mean, it sounds good on the surface, but from a consistency point of view, none of the features should depend on what version of Node.js or other installed modues are, in our opinion. This means that if you want to do conn.connect().then(...) with version 2.7 of this module.

The problem is if we provide features that depend on non-dependency modules (like a parallel Promise library) or Node.js runtimes, when this module is a sub-dependency of another module (which it is--a LOT, since this is a low-level module users typically do not directly use), end-users end up with inconsistent results when those intermediates start using those features.

An example of this problem is from using something like any-promise: moduleA will expect this module's promises to be that of Bluebird and then use some Bluebird-only extensions on those returned promises. That module even puts bluebird as a dependency to try and make sure that happens. Then a user tries to use the library, but has PROMISE_IMPL=q in the environment, causing mysql to return Q promises, breaking that intermediate dependency.

In the end, this module using any-promise and supplying an API that is a moving target is at fault in the above scenario. This is not a made-up scenario, either, because I've been in "promise implementation hell" several times from these so-call "smart promise shim" libraries like any-promise being in various dependency chains interacting poorly.

[felixfbecker]

Regarding my last comment: Actually, that will not work. Per specification a thenable is an object that implements a then method. Per specification, a then method must return a (full) promise. And I'm not thrilled to write my own full promise implementation, that would also highly violate dry (or in this case, dont-reinvent-the-wheel). Promises are a language feature, and if a user is on an older version of Node which hasn't implemented this language feature yet, it should be common sense to him that he can't use it with this module - except if he installs a promise library, which I would always recommend anyways.

You are right of course, and the number of own dependencies should be kept low in general.
The other option would be to do it like MongoDB does it and have the promise constructor as a connection option. That way a user has to explicitly define what promise library will be used for constructing promises just in that connection/pool and they can't interfere.
But there needs to be some way to specify it because like I said .then() must return a new promise and native promises are far superior to the established libraries in performance and debugging features.

[dougwilson]

like I said .then() must return a new promise

Gotcha. My understanding was based off https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/resolve which shows that the then property can be a plain function that takes two function arguments. It would let you do Promise.resolve(conn.query(...)).then(...) as far as I could tell. I guess I'm mistaken?

As for anything else, there are, in the end, we have the agreement above on what the requirements would need to be to accept a PR. If you cannot implement it in a way that would provide a consistent API for all supported Node.js versions, then we can old the PR until the next major version of this library, whenever that would be, if necessary.