Promise api

[newlix]

Hi,
I found the promise api support is in the v1.0 proposal. I am using Koa , so the promise api is quite important for me. I try to promisify the dal.query and keep it backward compatible.

I turned this

query: function query($query, callback){
    var this_ = this, caller = arguments.callee.caller.name;

    callback = callback || utils.noop;

    if ( this.isSyncing && !$query.ignoreSync ){
      return this.syncQueue.push( function(){
        this_.query( $query, callback );
      });
    }

    this.runBeforeFilters( caller, $query, function( error ){
      if ( error ) return callback( error );

      var query = mosql.sql( $query );

      return this_.raw( query.toString(), query.values, function( error, result ){
        if ( error ) return callback( error );

        if ( validReturningQueryTypes.indexOf( $query.type ) > -1 || $query.returning ){
          return this_.runAfterFilters( caller, result.rows, $query, callback );
        }

        callback();
      });
    });
  }

into this

query: function query($query, callback){
    function _query($query,caller, callback){
      var this_ = this;

      if ( this.isSyncing && !$query.ignoreSync ){
        return this.syncQueue.push( function(){
          this_.query( $query, callback );
        });
      }

      this.runBeforeFilters( caller, $query, function( error ){
        if ( error ) return callback( error );

        var query = mosql.sql( $query );

        return this_.raw( query.toString(), query.values, function( error, result ){
          if ( error ) return callback( error );

          if ( validReturningQueryTypes.indexOf( $query.type ) > -1 || $query.returning ){
            return this_.runAfterFilters( caller, result.rows, $query, callback );
          }

          callback();
        });
      });
    }
    var this_ = this;
    var caller = arguments.callee.caller.name;
    return callback ? _query.call(this,$query,caller,callback) : new Promise(function(resolve,reject){
      _query.call(this_,$query,caller,function(error,result){
        if (error) reject(error);
        else resolve(result);
      });
    });
  }

And the other functions which return this.query($query,callback) inherently return promise now and are backward compatible. Some functions like findOne and raw need a little more works, but I think it won't be too hard.

What do you think?

[jrf0110]

I think using a Promise library like bluebird will help out a lot in cases like this. Using a promisify function like: https://github.com/petkaantonov/bluebird/blob/master/API.md#promisepromisifyallobject-target--object-options---object will likely be best if we want to support both a node callback style api and a promise-based one.

Otherwise, the code can get convoluted in attempts to support both styles.

At goodybag.com, we've successfully implemented promises on top of dirac.DAL API with no issues.

[newlix]

Actually I considered using promisifyAll, but I don't like the added ugly "Async" suffix and the most important thing is that I can't 100% sure that it works for all functions. We don't even have any unit test for the auto generated promise api. Of course, you as the author are familiar with the whole code base and may have the confidence, but for anyone else it's a different story. I am glad to know that you have used bluebird to implement promises api in production at goodybag.com with no issues, but I think providing the official support for promise api benefit new users a lot and can have unit tests to guard them.

If possible, can we move forward to promise-based api and drop node callback style api before v1.0?

For now, I will use "promiseifyAll" as you suggested. Thank you.

[newlix]

With a Promise library, the before filters won't work.
Because in dal.query

caller = arguments.callee.caller.name;

[jrf0110]

Hrmm... yeah, I always knew the callee.caller.name thing was going to bite me in the ass.

@newlix if you can turn your code you posted into a PR, I can help out with amending the docs and adding some unit tests. The main thing I want to avoid while trying to support both callbacks and promises is situations where one is used over the other, but by some accident, both are invoked

[jrf0110]

Also, sorry for the late reply! It's been busy at work

[newlix]

I am currently porting the whole library and unit tests into promise-based. I found it might be easier to rewrite into promise-based first then using the nodeify function in the bluebird library to support node-style.

Also, I think dira.sync is really fancy but a little dangerous. I prefer using a migration tool like node-pg-migration.

[jrf0110]

Yes, 1.0.0 will completely remove dirac.sync. We use https://github.com/goodybag/pg-delta because non-destructive syncs just aren't reliable enough :/ Although, the createTables method is quite useful.

[newlix]

I have finished the porting. https://github.com/newlix/promise-dirac
I didn't keep it backward compatible and skipped some middlewares I don't need now to simplify the work. The final goal is to contribute back to dirac .

[newlix]

Also, I found some inconsistent parts between the code behavior and the document :

1. The callback of Dal.remove and Dal.update should be function( error, results ){ } instead of function( error, result ){ } and they will remove/update one or more documents if $query is an object . The documents need update.
2. The first parameter fnName of before/after filter function is not optional .

In the document:

fnName [optional] - If provided, will add the filter only to the method on the dal, otherwise will add on all methods.

but there is a check in the source code.

if ( typeof this[ fnName ] != 'function' )
      throw new Error('Dirac.DAL.before cannot find function `' + fnName + '`');

1. Currently the Dal.insert and Dal.remove have the returning: ['*'] in query by default, but Dal.update doesn't. I suggest make the Dal.update also have the returning: ['*'] in query by default.

insert

var query = {
      type: 'insert'
    , table: this.table
    , values: values
    , returning: ['*']
    };

remove

var query = {
      type: 'delete'
    , table: this.table
    , where: typeof id == 'object' ? id : { id: id }
    , returning: ['*']
    };

update

var query = {
      type:     'update'
    , table:    this.table
    , where:    $query
    , updates:  $update
    };

The examples in the document need update too.

[jrf0110]

Hey, @newlix that's fantastic. Please please please circle back around and let me know what issues you end up happening with Promises (as it will inform the creation v1.0.0).

I'll fix the docs. I believe I ended up writing the docs first and then implemented some of those features. Perhaps for one reason or another, I didn't get to implement the full spec :/

Thanks for letting know!

[newlix]

Hi,
I will follow your spec as close as possible and document the difference.

I think we can both maintain our own unit tests, yours for node-style and mine for promise-based. Dirac V1.0.0 will have to pass all these tests if we want to support both node-style and promise-based.

Please also close the resolved issues, I was wondering about some very old issues.

Thanks a lot.