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.

[felixfbecker]

Yeah you could do Promise.resolve(conn.query(...)).then/catch/finally(...).then/catch/finally(...) but if we write in the docs that the EventEmitter is now thenable they will try to do things like this:

conn.query(...).then(...).then(...);
conn.query(...).catch(...);
var query = conn.query(...);
query.then(...);
query.then(...);

which I cannot implement without having access to a real promise implementation, if I don't want to roll my own. In the simplest implementation, .then() would just set the _callback property. That would mean you cannot call .then() multiple times, you cannot use it together with a callback, you cannot start a promise chain with .then(), you cannot call .then() after the query has already ended, you cannot call .catch() and all the other methods. It would require only minimal changes to the codebase but you could do everything you want if you do await conn.query(...) or Promise.resolve(conn.query(...)). All of this would need to be highlighted in the docs. Node 0.6 users could use the .then() method, but as it just acts like passing a callback, it wouldn't benefit them - except if they do Promise.resolve(). Not spec compliant, but consistent across Node versions.

In the a little more complicated approach, query objects make a full internal promise object when you call .then() and basically proxy the .then() method to it. The promise constructor would be passed in as a connection option and would fallback to a global Promise. This implementation means that you can only use the .then() method if you either use a new Node version or pass in a promise library, but you would have a full chainable promise at hand (it would be debatable if catch and finally should be added too).

Most complicated to implement would be to make Query objects implement a Promise as per spec. It may only expose .then(), but it must take care of its state, allow multiple calls to .then(), try/catch the callbacks and return a new promise - we would end up with just another promise implementation and this is not desirable.

I will implement option 1 and when version 3 is coming and Node version requirement is raised I may revisit it and implement option 2.

[felixfbecker]

This is much harder than I thought it would be. No problem with the functions that talk to the server - they all return some instance of Sequence so .then can be added to Sequence.prototype. But what about the methods that don't talk to a server? Look at Pool.prototype.getConnection - if the pool is already closed, it calls the callback with an error, without returning any Sequence/EventEmitter. Then there's Pool.prototype.acquireConnection - it is async, but when the connection was aquired from the wrong pool, it throws synchronously and unleashes Zalgo. The only thing that makes sense to return from these methods is a promise. There is no EventEmitter that can be extended with a .then() method. But I cannot return a promise if the requirement is to make all features work with 0.6.... And I didn't even talk about PoolCluster....

TL;DR it is impossible to implement this feature for all node versions correctly. I would propose to add true promise support in 3.0, if the node requirement is raised to 0.12.

[atas]

Hi,

I wrote this small piece of code that lets me allow do async queries (I am really a fan of async and awaits, they make the code very nice :))

I use it like this

async function getUser(id) {
  const users = await db.queryAsync('SELECT * FROM users WHERE id=?', [id]);
  return users.length > 0 ? users[0] : null;
}

const user = await getUser(10);
console.log(user ? 'Hello ' + user.firstName : 'No user was found');

And this is the queryAsync addition I made:

const db = Mysql.createPool(options);

db.queryAsync = function (...args) {
  return new Promise((resolve, reject) => {
    args.push((err, result) => {
      if (err) return reject(err);
      resolve(result);
    });
    db.query.apply(db, args);
  });
};

If you like it let me know and I can send a PR for integrating queryAsync().

[felixfbecker]

@AtaS thats exactly what bluebird does

[atas]

Hmm yes it is indeed :) Thanks for all the guidance, node-mysql is beautiful with async/awaits.

[golopot]

How about we provide .then() method, in it check that global Promise exists, if not then throws error You need node > 0.12.18 to use query.then()?