Promises/A+ for JS driver

[jamescostian]

I would like to suggest that the JS driver use the Promises/A+ spec. Callbacks are very important to support because they are the de-facto style, but that doesn't mean that promises should be unsupported. Modules can return a promise and still call a callback, allowing both to work. If you're on the fence with regards to promises, just consider that the following 2 examples do the same thing:

r.connect({ host: 'localhost', port: 28015 }, function(err, conn) {
  if(err) throw err;
  r.db('test').tableCreate('tv_shows').run(conn, function(err, res) {
    if(err) throw err;
    console.log(res);
    r.table('tv_shows').insert({ name: 'Star Trek TNG' }).run(conn, function(err, res)
    {
      if(err) throw err;
      console.log(res);
    });
  });
});

NB: the following uses gen-run and JS ES6

var run = require('gen-run');
run(function* () {
  var conn = yield r.connect({ host: 'localhost', port: 28015 });
  console.log(yield r.db('test').tableCreate('tv_shows').run(conn));
  console.log(yield r.table('tv_shows').insert({ name: 'Star Trek TNG' }).run(conn));
});

[neumino]

I haven't try it myself, but you may be interested in https://github.com/guillaumervls/rql-promise

I'm not a big fan of promises for different reasons, but some people do like them, so we will probably add them at some point. It's not on the immediate roadmap though. I would tend to think that it's not too hard -- but I'm not familiar enough with them to properly judge how much work it would be. I'll try to take a look at how doable it is next time I'll have some free time.

[coffeemug]

Moving to backlog for now, but I think it's a cool idea. If promises are wildly adopted, count us in!

[pixelspark]

fyi, gen-run provides a way to use the traditional callback-style functions as well. So I guess the following should already work (notice the 'resume' callback; when called, its parameters will be returned from the yield):

var run = require('gen-run');
run(function* (resume) {
  var conn = yield r.connect({ host: 'localhost', port: 28015 }, resume);
  console.log(yield r.db('test').tableCreate('tv_shows').run(conn, resume));
  console.log(yield r.table('tv_shows').insert({ name: 'Star Trek TNG' }).run(conn, resume));
});

[jamescostian]

Good point, I didn't know that. Thanks for the pointer! Btw, your code actually doesn't completely work because you have to call the resume function. But the following code works:

var run = require('gen-run'); run(function* (resume) { var conn = yield r.connect({ host: 'localhost', port: 28015 }, resume()); console.log(yield r.db('test').tableCreate('tv_shows').run(conn, resume())); console.log(yield r.table('tv_shows').insert({ name: 'Star Trek TNG' }).run(conn, resume())); });

[neumino]

If someone is following this thread, there is a new driver that uses promises. https://github.com/neumino/rethinkdbdash

It's almost complete. I couldn't run completly run RethinkDB's polyglot mostly because backtraces are not exactly the same, but I believe all the terms/options are defined.

I'll keep adding tests, but hopefully it should be out soon.

[coffeemug]

:+1:

[pilwon]

:+1: :+1: :+1:

@neumino Will we see your project merged with the official Node.js client?

[neumino]

@pilwon We have to wait for Node 0.12 to be released first. Then I'm not quite sure what's going to happen.

I believe promises/generators will take over the callback, but that's going to take some time (hopefully not too much). So we are probably going to keep the two drivers for some time. That would also give rethinkdbdash some time to be tested in the wild.

Then when promises will be the normal way to write code with Node.js, we'll tag rethinkdbdash as our official Node.js driver.

Writing the driver was some work, but updating it with new terms is really easy to do, so I'll keep it up to date, no need to worry about that :)

[pilwon]

@neumino I agree that the addition of generator could wait until Node.js 0.12, but I'd hope that promise be added sooner. In my opinion, promise is just another style and callback style will probably never be completely replaced with callback. Instead of waiting until the perfect signal (which I don't think will happen), one way would be to add support for both, like some other projects do, and give users an option.

I usually have no problem wrapping any callback-only code with q's Q.denodeify or Q.nbind but rethinkdb driver makes it really tricky to wrap it because the query builder functions are all built into the same module. I also tried rql-promise but what I concluded was it's the cleanest and the easiest to just add this to the official driver, especially when it doesn't take too much effort as you mentioned.

It's currently one of the biggest pain points for me right now as user's perspective. (mixing with other promise-based code)

[coffeemug]

@pilwon -- I have a couple of questions about this.

I also tried rql-promise but what I concluded was it's the cleanest and the easiest to just add this to the official driver

Could you give some more details about why that is? Why would it be cleaner/better than using rql-promise (or one of the other wrappers like it)?

one way would be to add support for both, like some other projects do, and give users an option.

Could you give an example of a project or two that offer both and do it well? Mixing the two seems hard to do cleanly.

[pilwon]

@coffeemug rql-promise feels more like a hack to me. The use of rql() and rql.lazy() quickly turns chained query bulky and less readable, especially for slightly more complex queries. I'd like to use only the officially supported client library as well, if possible.

Here are some examples supporting both callback and promise styles:

• https://github.com/admc/wd
• https://github.com/tlrobinson/node-flurry
• https://github.com/tomyan/coro
• https://github.com/pilwon/node-udoo
• https://gist.github.com/eugeneware/6860345

[jamescostian]

I think it would be cool if there was an r.connectPromise which would act like r.connect but it would return a promise. One could then wait until that promise is fulfilled with either .then() (Q-style) or by using a generator that yeilds to the promise.

Queries could be run with just .run(conn) if conn is a connection that was made with r.connectPromise and the way that one could tell if it was made with r.connectPromise is the connection object could have a property isPromise that could be set to true if the user wants to use promises or false if the user wants to use callbacks.

[neumino]

Do you expect a syntax like that

var connection = r.connectPromise();
r.table("foo").run(connection).then(function(result) {
    //...
}).error(function(error) {
    //...
})

Or like that

r.connectPromise().then(function(connection) {
    r.table("foo").run(connection).then(function(result) {
        //...
    }).error(function(error) {
        //...
    })
}).error(function(error) {
    //...
})

Or both?

[jamescostian]

The second. Although I don't plan to do that, I actually plan to use gen-run. I would write an example but my math teacher is still teaching (ugh stupid algebra 2), and if I type too much he might notice it.

On Fri, Jan 31, 2014 at 11:05 AM, Michel notifications@github.com wrote:

Do you expect a syntax like that

var connection = r.connectPromise();
r.table("foo").run(connection).then(function(result) {
    //...
}).error(function(error) {
    //...
})

Or like that

r.connectPromise().then(function(connection) {
    r.table("foo").run(connection).then(function(result) {
        //...
    }).error(function(error) {
        //...
    })
}).error(function(error) {
    //...
})

Or both?

Reply to this email directly or view it on GitHub: https://github.com/rethinkdb/rethinkdb/issues/1395#issuecomment-33820956

[jamescostian]

Ok I'm in lunch. So here's what I would do:

var run = require('gen-run')
run(function* () {
    var conn = yeild r.connectPromise()
    // run queries like:
    yeild r.table('foo').run(comm)
    // another example, taken from the rethinkdb website:
    yeild r.table('FairVerona').insert(r.table('Montagues').union(r.table('Capulets'))).run(con)
}, function (err, value) {
    if (err) throw err
    else console.log(value)
})

[neumino]

Ok, thanks @jamescostian

So I think that shouldn't be hard to do. Instead of throwing an error when a callback is not provided, we can just return a promise. Testing it is going to be a little more tricky but it should be doable.

Assigning to myself.

[coffeemug]

FYI, I think we should return a promise if the user doesn't provide a callback. That sounds much cleaner to me than having a second function (e.g. connectPromise).

[pilwon]

@coffeemug If that's possible with the current driver code, then I absolutely agree.

I also hope to see rethinkdbdash's connection pool feature be brought in as well to make the official driver easier to use. That leaves only optional callback parameter to run (run(cb) for callback, run() for promise)

[coffeemug]

Talked to @neumino. He should be able to get to this next.

[pilwon]

@coffeemug That's awesome. Thanks @neumino !

[coffeemug]

Moving to 1.12-polish, since we might be able to get it in.

[neumino]

Question for people using promises. What promises library would you prefer?

The choice from my point of view is between bluebird and co. I somehow have a preference for bluebird, but I was wondering if some people needed co for some reasons.

[neumino]

Also moving to subsequent. While I could probably do it for 1.12, I would rather not rush this process because

• I don't want to swap the promises library later
• We can release the driver with promises at any time. We don't have to do it with a major release of the server.

[pilwon]

@neumino I believe co is a generator-based flow control library rather than a promises library. I think bluebird for Promises/A+ is a fine choice.

[pilwon]

@neumino Also, I don't think swapping the promises library would be an issue. Promises/A+ compliant libraries would provide a consistent API for user's perspective, therefore the library change would only affect the internal code.

[StreetStrider]

Good day to everyone. I've looked through API and I've noticed that the only place where callback used is run. This makes «promisification» of library is very simple, especially in comparsion with mongo driver, where asynchroneity lives on three different levels: driver instance, collection & cursor.

I think that things like rql-promise, mentioned above is enough for most cases. I'll also try to write my own wrapper. Thanks for great API!

Anyway, promises in official driver are very welcome.

[neumino]

There is actually a little more than just run.

There are at least:

• cursor.toArray
• cursor.next
• connection.close
• r.connect

[StreetStrider]

@neumino, thanks for the tip. I've already noticed that ones, when implementing wrapper. Not so many, though.

[neumino]

In branch michel_1396_promises Code review 1390 assigned to @atnnn

[coffeemug]

@neumino -- a few questions:

• Which are the functions that return promises when the callback is omitted? Could you give a complete list here?
• Do we have tests for this? (I couldn't make out in code review whether we magically transform the current tests to use promises, or whether we need to write more tests)
• Could users mix callback and promises style? I.e. omit a callback for connect, but pass it to cursor.next? (We should have some basic tests for what happens here)
• How does this interact with connection.addListener?

And some thoughts:

• We'll have to update the API docs for relevant functions to give examples of both use.
• I think we should write a separate blog post showcasing how cool this is (basically, a mini tutorial), and may be link to it from the docs.
• I now agree with you that we should launch it separately as soon as Node is ready.

[neumino]

• All functions that takes a callback, except cursor.each and addListener (r.connect, conn.reconnect, conn.close, conn.noreplyWait, run, cursor.next, cursor.toArray)
• I wrote test for that: ql-js-promise
• You can mix callback and promises, for different invocations. You cannot pass a callback and retrieve a promise (but for example you can pass a callback for r.connect and use a promise for cursor.next)
• connection.addListener doesn't change, promise doesn't really make sense here. A promise is an object that will eventually be "resolved".
• The changes are backward compatible. You get a promise only if you omit the callback.

[coffeemug]

Got it, thanks! One more question. How do promises interact with errors? Do we now throw the object that we'd normally pass to the callback?

[neumino]

You call error on the promise. A typical syntax is

query.run().then(function(result) {
   // process result
}).error(function(error) {
   // process error
})

[coffeemug]

How does one deal with errors when using yield?

[neumino]

With a try/catch

try{
    var result = yield query.run()
    // process result
}
catch(error) {
    // process error
}

[coffeemug]

Got it, thanks!

[pilwon]

You call error on the promise. A typical syntax is

query.run().then(function(result) {
   // process result
}).error(function(error) {
   // process error
})

@coffeemug .then() could also take an optional onRejected function as the second parameter. (refer to promise.then(onFulfilled, onRejected) section of Promises/A+ spec)

[jamescostian]

So... ¿Qué pasa? Is anything happening with this?

[coffeemug]

@neumino already has this working in a branch. This is getting merged soon (we need to test a bit more and write the docs).

[neumino]

There was a question about whether we should include bluebird in the repository, but that was settled.

It's just about updating the docs and writing examples.

[jamescostian]

Awesome! Thank you guys so much for doing this work!

[AtnNn]

@neumino it would be nice if run accepted a promise of a connection, so that code like this would work:

c = r.connect();
query.run(c);

[coffeemug]

Wow, this would be amazing! (seems very monadic, btw)

[neumino]

You can do that for now

query = r.expr(1);
r.connect({port: 42935}).bind(query).then(query.run).then(console.log);

But that doesn't close the connection. We could remove the need for bindif we were fixing https://github.com/rethinkdb/rethinkdb/issues/1966 Closing the connection make things less nice since you have to manually create a function.

About @atnnn's suggestion, it feels kind of wrong to me because things are in the "wrong order". But I may be wrong, I'll google a few things to see if people are comfortable with this pattern.

[coffeemug]

Hmm, why is @atnnn's suggestion in the wrong order? The order is exactly the same, you just don't have to type yield until the very end.

[coffeemug]

In other words, instead of

conn = yield r.connect()
res = yield r.expr(1).run(conn)

I can type:

conn = r.connect()
res = yield r.expr(1).run(conn)

This is just wonderful, and if people don't like this notation, they can just avoid using it.

[jamescostian]

Oh, you could pass either pass an actual connection or a promise for a connection? That sounds cool IMO!

[neumino]

@coffeemug -- @atnnn's example can be rewritten

query.run(r.connect())

Which is the opposite of

r.connectI().then(query.run)

[neumino]

Oh, and the promises are in branch michel_1395_promises_squashed if someone wants to play with them. I'll start working on the docs tomorrow.

[coffeemug]

@neumino -- ah, I see what you meant. (I still think @atnnn's idea is awesome and we should do it).