Moving the Promises -- v3

[aray12]

In #226 @robconery expressed that he would be open to moving to promises for v3 of the library. This issue can serve as what someone can work against if they can help.

My suggestion would be to move to an all promise api, but to use something like asCallback for people that still prefer callbacks.

[aabenoja]

Would it be better to check if the last argument is a callback function instead of using an asCallback function? The internals use native (to node 4+) promises, only the callback is magically conveniently invoked if passed.

db.users.find(1)
  .then(doThingWithuser, reportErr)

// find could be returning a promise for all anyone cares
db.users.find(1, (err, user) => {
  if (err) { return reportErr(err); }
  doThingWithUser(user);
});

[robconery]

Check the bluebird branch - we're thick into this currently :).

[aabenoja]

Seems like I spoke just a bit too soon. It looks great. I can't wait to make use of this in my apps. Do you have any plans on putting this in a pre-release when it's ready?

[xivSolutions]

The Bluebird branch is sort of trying things on - we are exploring here a bit. Ultimately what/when we release, and what direction it takes is up to Rob, but I can see an interim pre-release with something like the structure you see in the Bluebird branch, and meanwhile we are also experimenting with a fairly major change to go all-in with promises, possibly also all-in with ES6.

In the meantime, if one were to pull down what we have and experiment/give feedback (what works, what doesn;t, etc) would be most helpful. :-)

[aray12]

When you talk about going all-in with es6 are you thinking of adding a transpiling step to pre-publish? Or based on node v6 support?

[xivSolutions]

For myslef, I'm still investigating - I think Rob has been a little bust of late. So far, it seems like transpiling might be a better way to go while the dust is settling out witht he various versions of Node/V8, but I'm still looking into it all.

In particular, so far as I can tell at the moment, Mocha is not yet ES6 compatible. Also, we can always code using the ES6 stuff, and pull the transpile steps out when the features we want are fully supported in the wild.

But I hstill have more research/experimentation to do. Transpiling does require some additional dev env setup, and feels a little flaky at times. But I'm still new to the ES6/Babylon stuff.

[Sinewyk]

I don't know what you mean by mocha is not es6 compatible but yeah, you just have to be careful about not using arrow functions for mocha's after, before, afterEach, beforeEach, etc.

Otherwise because this is passed around and it's used for hooks, you end up attaching hooks on every single tests

// mocha create a describe block wrapping all your tests
// so the this here
describe('', () => {
  // is the same as here because of arrow functions
  beforeEach(() => {
    // and so this hook is going to be executed on every tests, of every files
    // not only on the describe of this file like you probably intended to
  });
});

Use arrow function in your tests, just not for mocha functions. Jscodeshift transform available if you want.

PS: sorry for jumping in like this I guess ...

[xivSolutions]

@Sinewyk - Please do jump in. I was able to do some preliminary investigatin' on the ES6 front, and then had to move. I haven;t had a chance to jump back to it yet, but I had read somewhere (and had not had a chance to dig into it) that Mocha had some challenges with ES6.

However, it also looks like the Node v6 release moves things closer in terms of Node compatibility, so we'll see. Hoping to get back to this soon. Moving sucks. :-(

[Sinewyk]

According to the doc it's https://mochajs.org/#arrow-functions "discouraged", whereas there should be a big warning sign with sirens telling you to absolutely not do it at the beginning of the docs.

Mocha can handle ES6 just fine. Just no arrow functions as arguments to mocha functions describe, it, before, after, beforeEach, afterEach and context and you're good to go.

Also, because we are monkeys, if you want to take a look at transpiling, juste take a look at how https://github.com/reactjs/redux does it. It's quite complex but handle all the cases. You can skip the browser stuff but the lodash optimization are pretty interesting.

[purepear]

Promises will make async/await use possible.

[johanneslumpe]

@robconery any updates on this?

[robconery]

I'm a bit torn. On the one hand I can patch in a number of things using a promise lib. On the other I can wait until ES6 hits with full promise support (and other things) so I just need to refactor/upheave once. I like the idea of doing big changes just once - but I guess I'm open....

[Sinewyk]

I've actually read all the issues. I don't understand the problem. If author does not like Promises and is still nodejs callback compliant on all his asynchronous methods ...

Why are issues spawning all over the place when there's already a solution with bluebird promisification + co or bluebird coroutine and stuff for anyone wanting more generator, or async/await over callback or insert latest trend

I see callback as the lowest primitive possible so that userland can abstract it away as they wish, why the need for Promise directly in core ? It feels like reading the Promise in nodejs core debat all over again.

@robconery does moving to Promise in the v3 actually improve anything towards improving the library or is it somewhat peer pressure for zero gain ?

On a more perf related note: database code should always be as fast as possible, and you can't get faster than callbacks right now. Wrap it in bluebird, and you get the best of both world anyway as right now V8 promises sucks.

TL;DR: :-1: on Promises in core if library code doesn't actually get better/faster/more maintainable as a direct result.

[robconery]

Yeah I've gone through all that already - it's part of the problem :). ES6 will allow us to streamline a ton of shit - and promises will actually help with testing in a big way (making them easier to write).

Speed is a large concern, but it's not my primary metric TBH. If I can slim down the lib and make it easier to maintain I will do it. Moreover if it generates community involvement, that's even better.

I think we're beyond the promise debate now. I'm happy to give in and move on; it's where things seem to be going.

I don't want to be harsh about this - but I'd like to keep this thread technical and not flame out into a large opinion piece. I've pushed back on promises many times using the same exact logic... but it just so happens that ES6 and Node vWhatever will really bring some nice changes - the question is "when do we jump".

[thejones]

As @purepear mentioned Promises will make Async/Await possible and that should be moving forward as of a couple days ago for ES7/ES2017/ES? https://github.com/tc39/proposals/commit/e8c03544751b071b973064ef7d437a34ac6752cd

@robconery this guy jumped 25,000ft without a parachute so I say we jump! http://abcnews.go.com/Entertainment/extreme-skydiver-luke-aikins-prepares-jump-25000-feet/story?id=40986613

[robconery]

Yeah I'm ready - just need the time!

[igl]

Pretty much just waiting for massive to be Promise or Observable based so i can move away from knex.raw().

Both the v3 and bluebird branch didn't get commits since Mar/Apr 😢 I am curious about the state of massive and if those branches need any help.

[robconery]

The state is that it's pretty popular so far and I just need the time to work on it, which (once I get this book finished) should be in the next month.

[xivSolutions]

Likewise, I am eager to jump in on this V3, but have been bogged down huge with work.

[mischkl]

+1 for Promises! 👍

[retorquere]

What branch is this feature being worked on? I'd guess promises as that had a commit 7 days ago, but there's also v3 (last commit on Mar 29, 2016) and bluebird (last commit on Apr 3, 2016). Is there an overview of the state of the promises implementation?

[dmfay]

Yes, it's promises. The other two were experiments that didn't pan out.

Currently, the promises implementation is baseline complete; all tests pass, but I haven't tried integrating it into an application yet. There are a couple new changes on the master/2.x branch which haven't made it across (eg loading foreign tables), and there are some other things I want to at least look at like transactions before dropping 3.0. But the big thing right now is that all the documentation needs to be rewritten. I'm planning to put the v3 docs in GitHub Pages instead of RTD, and I've gotten at least basic API documentation done with JSDoc, but there's still just a lot of writing and template wrangling to do.

[retorquere]

I am starting a proof of concept (towards a production app) which will use the promises branch (installed using npm install --save 'git+https://github.com/dmfay/massive-js.git#promises' should anyone wonder). So far everything from Full JSONB Document Support from the README just works as is, but instead of callbacks I use promises (or to be specific, I use yield in a bluebird coroutine-wrapped generator).

The one thing that did initially fail was yield db.createDocumentTable('my_table') when the corresponding table existed, because that does a create table under water, not a create table if not exists. I'm fine with that, but it'd be useful to have a way to test for the presence of tables so I can avoid the error. What I have now is this:

function hasTable(db, name) {
  var [schema, tablename] = name.split('.');
  if (!tablename) {
    tablename = name;
    schema = 'public';
  }
  return db.tables.findIndex((table) => (table.schema == schema && table.name == tablename)) >= 0
}

var conn = massive(connectionString);

....

co(function*() {
  var db = yield conn;
  if (!hasTable(db, 'my_table')) {
    yield db.createDocumentTable('my_table')
  }
})

[retorquere]

Any reason BTW to not just use bluebirds promisifyAll?

[dmfay]

oh, cool! I've tried to keep the API as close as possible to 2.x so following the old docs and converting to promises should be what you need for most of it. If you can raise new issues for stuff like that you run into that would be awesome, and if you feel like submitting pull requests against that branch I'll happily review them.

I do want to keep the error in this case (since if you try to create a document table but actually get an existing table with a completely different structure that leads to unexpected behavior down the line), but I'm on board with adding hasTable to the Database object. some is shorter than doing a comparison on findIndex but it looks good otherwise.

I'm not sure what you'd be using promisifyAll for against this branch, since everything already uses promises...?

[retorquere]

OK, so that'd become

function hasTable(db, name) {
  var [schema, tablename] = name.split('.');
  if (!tablename) {
    tablename = name;
    schema = 'public';
  }
  return db.tables.some((table) => (table.schema == schema && table.name == tablename))
}

[retorquere]

No sorry, I meant was there a specific reason to rewrite v2 for promises rather than keeping v2 mostly as-is and calling promisifyAll at strategic points.

[dmfay]

If it's on the Database prototype you can just pass the name, but otherwise yeah.

[dmfay]

The biggest user-facing concern addressed by moving to promises internally is transaction support; it's much more difficult to manage a single connection in a pool with a callback architecture. But it's also just much cleaner and easier to work with in general.

[retorquere]

I'm mostly done with the README, but I'm not sure what to do about the streaming samples.

[dmfay]

test/queryable/find.js has an example implementation under streaming results.