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

Promise support #186

Open
yzarubin opened this issue Nov 5, 2015 · 23 comments
Open

Promise support #186

yzarubin opened this issue Nov 5, 2015 · 23 comments

Comments

@yzarubin
Copy link

yzarubin commented Nov 5, 2015

I noticed there was a thunkify option to return the API using thunks instead of callbacks, which is great, thanks for that! Any thoughts on doing the same and returning bluebird promises? It would be very helpful seeing as how async/await currently favours promises.

@jonpacker
Copy link
Member

Please note that the thunkify option is undocumented because it is untested, so use it with care. I don't have any plans to add promises, but if it is as simple as writing the thunk support was, then I can look into it.

@jonpacker
Copy link
Member

Is there any reason you specifically mention bluebird promises, and not ES6 promises?

@yzarubin
Copy link
Author

yzarubin commented Nov 5, 2015

I mentioned Bluebird because seeing as how you're using thunkify to convert callbacks to thunks, I figured you'd similarly use a promise library such as Bluebird to promisify() the callbacks into promises. An implementation using ES6 promises would be best, but would involve more work than just promisifying the API. Let me know if you'd like a PR for this feature.

@jonpacker
Copy link
Member

If there's an easy way to do it, a PR would be great. I added thunks to work with co and ES6, not promises specifically.

It would probably best to avoid relying on seraph/co.js to do it. That was purely experimental, and I think it probably would not work with batching (it's untested).

@n321203
Copy link

n321203 commented Jan 21, 2016

+1 for promises

@akotulu
Copy link

akotulu commented Feb 9, 2016

+1

@Vani-gurnani
Copy link

+1 for Promises.

Spoiled with async await cant go back to callbacks.

@alelavoie
Copy link

+1 for Promises as well

@alelavoie
Copy link

Note that, meanwhile, you can use bluebird's promisifyAll() to wrap all seraph's functions inside promises.

Don't worry, it's non-breaking since you need to append Async to the function name to receive a promise instead of a callback.

Here is an example :

var bluebird = require("bluebird");
var neo4j = bluebird.promisifyAll(require("seraph")("http://localhost:7474"));

neo4j.queryAsync(cypher, params).then(function(results) {
  //Do stuff here
});
Callback Promise
neo4j.query() neo4j.queryAsync()
neo4j.save() neo4j.saveAsync()
neo4j.delete() neo4j.deleteAsync()
... ...

@jonpacker
Copy link
Member

@alelavoie do you think that using having promisifyAll built into the library (so that you have access to async functions by default` would be a good stopgap way of supporting promises? I want to retain callback support in the near future, and argument sniffing would be basically impossible in most of seraph's API.

Totally open for input here, I'm not sure what the best way to go about this is.

@alelavoie
Copy link

If we consider what promisifyAll do :

Promisifies the entire object by going through the object's properties and creating an async equivalent of each function on the object and its prototype chain.

I'm not sure it would be a good idea since it would create useless overhead everytime the library is loaded. It would be better to create promisified prototypes or methods directly in the module.

@alelavoie
Copy link

But then again, it wouldn't be the best solution.

I'm not an expert in the matter but the best way to support both promises and callbacks seems to be by building the code with promises and using bluebird's .nodeify or .asCallback functions to return a callback if a callback was passed.

Brief example here : http://stackoverflow.com/questions/23501146/creating-node-library-that-support-both-node-callbacks-and-promise-with-bluebird

@jonpacker
Copy link
Member

It's a lot more (human) overhead to first manually convent the entire library to promises and then use nodeify to convert it back, than the other way around. Frankly, too much overhead.

it would create useless overhead everytime the library is loaded

I'm not sure that's actually the case. It would happen only once in the lifecycle of the program, on the first require, and only if you require the promisified module. I'm thinking something like require('seraph/p') here. After that, the module is cached and promisifyAll would not need to be run again. I also don't think the overhead is as high as some of the operations that happen inside seraph itself, for example upon instantiation and batching.

@alelavoie
Copy link

@jonpacker I agree that converting every async function to promises would be a huge task but it would be the ultimate solution (unless there is a better way of supporting promises and callbacks).

You are also right about the fact that the overhead is minimal. I didn't realize that modules were cached after the first time they were loaded, even if you load different instances of them.

I guess having promisifyAll built into the library is not as bad as I first thought: we would have a cached version of the promisified module instead of promisifying it every time with promisifyAll.

That being said, with ES7 Async / Await, promises will become even more important so I think it would be worth it to do a full code migration.

@alelavoie
Copy link

You don't have to do it all yourself though.

If I ever get some free time, would you be open to the idea ?

@sitexw
Copy link

sitexw commented Jul 4, 2016

+1

@jonpacker
Copy link
Member

jonpacker commented Jul 28, 2016

Hey guys. I've spent a bit of time working on this now. Since neo4j 3 released the "bolt" protocol, I thought it would be a good idea to update seraph to work with it. Since the neo4j javascript driver is promise based, it made sense to make the bolt-based seraph promise based as well. It's a total rewrite, with all functions now cypher based.

Feel free to play with it by setting the bolt: true flag in the options (as well as the bolt server instead of the REST API server) when instantiating seraph. All functions will return promises by default, but the nodeify: true option exists for backwards compatibility.

You can check out a brief breaking-changelog here: lib/bolt/CHANGELOG.md and the source here: lib/bolt/seraph.js

Since I just wrote this, you can expect some bugs. A subset of the tests are passing (the ones that make sense to run with bolt, see npm run-script bolt-test), but there are some bolt-specific tests to be written that aren't there yet - so there's probably some bugs.

@jonpacker
Copy link
Member

I've just pushed a new version of seraph-model now that works with seraph with bolt:true and nodeify:true enabled. Promise support for seraph-model would be awesome, but for the moment I think it's best gained with some variant of promisifyAll.

@yourtion
Copy link

+1

@TarekDeeb
Copy link

TarekDeeb commented Jan 18, 2018

I achieved it by creating promise functions for each api and extending it

// neo4j-helper.js

let db = require('seraph');

db.queryPromise = (query, params) => {
  return new Promise((resolve, reject) => {
    db.query(query, params, (err, res) => {
      if(err) reject(err) else resolve(res)
    })
  })
};

module.exports = db;

Then calling it in another file

const db = require('./neo4j-helper');

db.queryPromise("MATCH (n:Movie {name:{name}}) RETURN n LIMIT 1", {name: "Star Wars"})
  .then(res => {
    // handle if success
  })
  .catch(err => {
    // handle if error
  })

This is especially useful to me because I can extend it with any functionality I want and use often like for example

db.getAllMoviesWatched({email:"[email protected]"})
  .then(movies => {
    // do stuff with those movies
  })
  .catch(err => {
    // whoops...
  })

@jonpacker
Copy link
Member

jonpacker commented Jan 18, 2018

@TarekDeeb sorry that this is so poorly documented, but promise support is already built in if you use the bolt API:

const seraph = require('seraph');
const db = seraph({
  bolt: true,
  server: 'bolt://localhost',
  user: 'neo4j',
  pass: 'password'
})

db.query(`MATCH (n) RETURN n`).then(r => console.log(r))

I really need to get on making this the standard API, it is way overdue.

@TarekDeeb
Copy link

Oh. I see. I just switched to the bolt protocol and stumbled upon this issue. Let me know if you need any help with this.

@ghost
Copy link

ghost commented Feb 6, 2019

So what's the status of this? Are bolt: true and promises usable for production?

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

No branches or pull requests

9 participants