Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for promises in addition to callbacks #164

Open
jrgleason opened this issue Jul 7, 2015 · 15 comments
Open

Allow for promises in addition to callbacks #164

jrgleason opened this issue Jul 7, 2015 · 15 comments

Comments

@jrgleason
Copy link

This would be awesome and then I could stop wrapping everything :-)

@bojidar-bg
Copy link

👍 x 100
I'm currently using bluebird.promisify, but there must be a better way 😉

@jrgleason
Copy link
Author

@bojidar-bg Me too

var db = new neo4j.GraphDatabase(conf.connectionString),
Promise = require('bluebird'),
Cypher;

if(db.cypher)
   Cypher = Promise.promisify(db.cypher.bind(db));
...
return Cypher({
    query: query,
    params: params
})

@aseemk
Copy link
Member

aseemk commented Sep 15, 2015

Feedback noted! =) Let me wrap up v2 and ship it as-is, and then I'll look to add promises soon after.

@hilkeheremans
Copy link

Not to spoil the party: I could definitely applaud node-neo4j going all out for promises, but aren't there more important things to do when you could just use bluebird.promisifyAll which does the job just as well?

What I mean is that right now you can just do the following once somewhere in your node application (ES2015 example)

import Promise from 'bluebird'
import neo4j from 'neo4j'
Promise.promisifyAll(neo4j)

et voila, instant node-neo4j with promises all over the place. No wrapping involved; you just call the same methods you normally do with Async attached to the function name (see also https://github.com/petkaantonov/bluebird/blob/master/API.md#promisepromisifyallobject-target--object-options---object)

Of course, the above assumes you are using bluebird which I can understand not everyone is doing.

@bojidar-bg
Copy link

@hilkeheremans ~ I don't really like to type Async or Promise after every function call I make...

@hilkeheremans
Copy link

@bojidar-bg Maybe I'm missing something, please help me understand the complete issue for you?

My take on it: after promisifying it all, you can easily wrap neo4j's Async calls in a wrapper library with the same function names (=nearly zero effort). The api isn't that large so that shouldn't be a big issue?

For the async remark an example (without proper error handling but you get my drift)

    let query = function(params) {
        "use strict";
        return Neo4j.db.cypherAsync(params)
    }

Not sure where you are going with the "typing Promise" after every function call you make. Coul you explain what you mean?

I'm personally combining all this with ES7 async/await (read: Babel transpiling), where you can, using the suggestion I made above, essentially end up with the following, IMO very practical code (another quick 'n dirty example):

function async doSomeQuery() {
    try {
        let result = await neo4jwrapper.query({
                query: 'some_query',
                params: {
                    prop1: 'value'
                }
            })
    } catch(err) {
        console.error(err)
    }
}

How would this make things harder (*)?

A native implementation is better and I would certainly support it, but I personally prefer @aseemk focuses on other issues since this is an easy solution with minimal time effort, which also doesn't imply having to maintain two API surfaces (callbacks & promises) in the future. But don't let that stop anyone from doing this anyway. :-)

(*) aside from error handling which is a little easier to forget with Promises & await/async.

@jrgleason
Copy link
Author

I also seem to have issues if I do multiple promises (using bluebird) within the same transaction. I get the following error...

Unhandled rejection neo4j.ClientError: A request within this transaction is curr... (length: 1465)

Why does it matter if one is already running? Couldn't it just queue the Cypher query?

@aseemk
Copy link
Member

aseemk commented Sep 23, 2015

That limitation comes from Neo4j itself: concurrent queries within the same transaction are not allowed.

You have an interesting and good point that node-neo4j could queue the queries under the hood. I hesitate with something like that though, as part of node-neo4j's goals is to be transparent. It feels like it's worth knowing that the queries aren't running concurrently, rather than node-neo4j hiding that.

@jrgleason
Copy link
Author

I survived this by reverting back from Promises and using the async library. I also opened this #172 because I think it would be nice for it to be done on the node-neo4j side so I can still use promises :-)

@hilkeheremans
Copy link

@jrgleason Have you tried bluebird's Promise.map with {concurrency:1} as an option?

Promise.map([array_of_values],function(value) { // return a promise }, {concurrency:1})

Promise.map allows you to execute a variable number of promises, based on either a function array or value array, and the concurrency option ensures that bluebird will only execute one promise at a time, only starting the next once the previous one has resolved.

@hansman
Copy link

hansman commented Oct 14, 2015

I use bluebirds promisifyAll to get pomified neo4j methods. Could be minimal lines to get it in.

@tuananh
Copy link

tuananh commented Jun 13, 2016

Can someone send me the code snippet how to promisifyAll neo4j

var Promise = require('bluebird');
var neo4j = require('neo4j');
Promise.promisifyAll(neo4j);

This doesn't work for me.

@herlon214
Copy link

+1

@H3mann
Copy link

H3mann commented Sep 21, 2016

hi guys. Any update on promises with thingdom node-neo4j?
Thanks!

@guibaldissera
Copy link

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants