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

Support Promise #864

Closed
hh9527 opened this issue Sep 21, 2015 · 57 comments
Closed

Support Promise #864

hh9527 opened this issue Sep 21, 2015 · 57 comments
Assignees
Labels
Projects

Comments

@hh9527
Copy link

hh9527 commented Sep 21, 2015

Will this project support Promise (which has already supported in Node 4.x)?

@stockholmux
Copy link
Contributor

I don't believe that is on the roadmap. You can, however, promisify the library with bluebird.

There is even a specific example for this library.

@BridgeAR
Copy link
Contributor

I do like to add a way to promisify the library if a certain parameter is passed. This won't be in the next release though.

@BridgeAR BridgeAR reopened this Sep 21, 2015
@BridgeAR BridgeAR self-assigned this Sep 21, 2015
@stockholmux
Copy link
Contributor

@BridgeAR Interesting. If the option parameter is passed will it essentially just use bluebird? Or will it be implemented in a different way?

@BridgeAR
Copy link
Contributor

Yes, I'd use bluebird internal.

@moretti
Copy link

moretti commented Sep 21, 2015

Related project:
https://github.com/mjackson/then-redis

@luin
Copy link
Contributor

luin commented Sep 23, 2015

As for me, it would be convenient to return a Promise if the last argument isn't a callback function. I implemented it in my redis client. Refer to https://github.com/luin/ioredis for more details.

Here's an example:

redis.get('foo', function (err, result) {
  console.log(result);
});

// Or using a promise if the last argument isn't a function
redis.get('foo').then(function (result) {
  console.log(result);
});

@BridgeAR
Copy link
Contributor

That might be an option. I'll think about it when I have the time to tackle this.

@puzrin
Copy link

puzrin commented Jan 31, 2016

Probably it could be better for performance to return "thenable" instead of promise - no need to create promise wrapper for each item of chained commands. Wrapping will happen only on .then() call.

@Nepoxx
Copy link

Nepoxx commented Apr 8, 2016

While it would require a lot more work, I'd recommend using native promises instead of a library like bluebird. As much as I like bluebird, using it will increase the number of dependencies and isn't the only library used.

Mongoose uses native promises but allows replacing it with any promise implementation by the user, which is convenient for everyone. See http://mongoosejs.com/docs/promises.html#plugging-in-your-own-promises-library

@PaulBGD
Copy link

PaulBGD commented Dec 8, 2016

Hi, just wondering if this is being worked on. Definitely one of the most requested features.

@inca
Copy link

inca commented Jan 11, 2017

With then-redis no longer maintained this issue becomes really critical. Adding bluebird to runtime which supports promises natively doesn't make any sense — so people end up inventing promisifying wrappers 😭

@PaulBGD
Copy link

PaulBGD commented Jan 11, 2017

I would be willing to PR this if the maintainers are interested.

@jayjanssen
Copy link

IMO it'd be very good if said promises handled errors in the standard promise way instead with 'on' handlers. (excuse my coffeescript):

redis.get('key')
.then (value) ->
  console.log value
.catch (err) ->
  console.error err.message

I really want to handle redis operation errors in the code where I'm actually doing the operation. ioredis seems to get that very wrong in its promise "implementation".

@luin
Copy link
Contributor

luin commented Feb 16, 2017

Hi @jayjanssen,

ioredis implements Promise in a standard way (by means of bluebird). For instance:

redis.get('foo', 'bar').then(console.log).catch(err => {
  // got an error since `GET` only accepts one parameter.
})

I'm wondering in which case do you need the 'on' handlers.

@jayjanssen
Copy link

I was just testing this today and I would swear I had errors getting thrown all over the place and not being properly handled in the .catch. I'd be happy to try it again.

@jayjanssen
Copy link

@luin see redis/ioredis#433 , thanks.

@n1ru4l
Copy link

n1ru4l commented Apr 9, 2017

For all people that do not want to use the bluebird lib:

import { promisifyAll, } from 'sb-promisify'
import redis from 'redis'

const RedisClient = redis.RedisClient.prototype
const Multi = redis.Multi.prototype
const RedisClientP = promisifyAll(redis.RedisClient.prototype)
const MultiP = promisifyAll(redis.Multi.prototype)

redis.RedisClient.prototype = Object.assign(RedisClient, RedisClientP)
redis.Multi.prototype = Object.assign(Multi, MultiP)

@divyenduz
Copy link

divyenduz commented Jun 16, 2017

The then-redis repository claims that it is not natively supported. Is that the case ?

If not that then, can I wrap this with node 8.0.0 feature util.promisify merged here

@kahwooi
Copy link

kahwooi commented Aug 9, 2017

Does it support promise natively, or I still have to use bluebird?

@n1ru4l
Copy link

n1ru4l commented Aug 9, 2017

@kahwooi You do not need to use bluebird, you could use the method I posted. If you are using the newest node version I'm pretty sure you could also monkey patch the objects with util.promisify (http://2ality.com/2017/05/util-promisify.html)

@manoharreddyporeddy
Copy link

manoharreddyporeddy commented Aug 14, 2017

@luin

Can you detail what is "if the last argument isn't a callback function"?

Looks like, last argument is a function in get():

redis.get('foo', function ...

which document are you referring to when you mention last argument?

@jyotman
Copy link

jyotman commented Aug 14, 2017

@manoharreddyporeddy it means that if you don't provide the last argument to the get method then it will return a promise. If you do provide the last argument, which is a callback function, then get will not return a promise and you'll have to handle the result inside the callback function argument.

@manoharreddyporeddy
Copy link

manoharreddyporeddy commented Aug 14, 2017

Makes a lot of sense now.
The english is too confusing.

"if the last argument isn't a callback function"
Should be either:
"if the last argument is missing" OR
"if the last argument/ the callback function is missing"

Above wording, or similar, is easy to understand, and is more correct.
If so, may need fixing. Even here: https://github.com/luin/ioredis

Thank you

@damianobarbati
Copy link

+1 to have native promise support without external libs into this! And +1 to have as the nowadays standard: if no callback is provided then return a promise :)

@LoganDark
Copy link

using util.promisify would be a breaking change that forces node6 users to upgrade.

Version check.

@LoganDark I do not see where this is a hack? The internal implementation for this would probably be the same...

You're right, it's not exactly a hack.. it is mutating the library though, which always makes me a bit uneasy.

@n1ru4l
Copy link

n1ru4l commented Jun 11, 2018

@LoganDark How about this:

const util = require('util')

const redis = require('redis')
const client = redis.createClient()

const handler = {
  get(target, propKey, receiver) {
    return util.promisify(target[propKey]).bind(target)
  },
}

const clientProxy = new Proxy(client, handler)

No mutation of the core library 😉

Beside that: I would love to see Promise support as core part of the library. However the author seems to have performance and backwards compat as a priority.

@damianobarbati
Copy link

@n1ru4l that's a biased approach when working with js ecosystem.

Also AFAIK using util.promisify would be a breaking change that forces node6 users to upgrade.

I agree but this is a good thing imho.
What about up-to-date developers which keep their environments up to date? Why should they be penalized?
We as developers have the right to stand against obsolete ecosystems and start thinking "forward compatible" instead of "backward compatible".

If the best implementation requires an up-to-date nodejs then we can have developers choose:

  • update node and use the latest and best version of ioredis
  • keep old node and use the latest compatible version of ioredis for it

Sooner or later ioredis (as every software) will reach the line when developers have to update their ecosystems to use it because of features we will have on node: why waiting then? 🤔

@n1ru4l
Copy link

n1ru4l commented Jun 11, 2018

@stockholmux Are you open for a pull request that makes the methods return a native Promise if there is no callback provided?

@stockholmux
Copy link
Contributor

@n1ru4l What do you thing @BridgeAR?

@Janpot
Copy link

Janpot commented Jun 18, 2018

In case anyone is interested. I went ahead and created promisify-redis for myself, I will us this from now on.

@adematte
Copy link

@stockholmux The approach @n1ru4l suggests is great as it prevents breaking changes on existing code while still allowing to use promises. This is also the approach the official node mongodb lib has taken to support promises.

@LoganDark
Copy link

@jmparsons holy fuck that would be horrible. .promise() would be everywhere. The majority of people want functions to return Promises when WE OBVIOUSLY DON'T PROVIDE CALLBACKS

@LoganDark
Copy link

@jmparsons are you claiming there were promises early on? :P

@egoarka
Copy link

egoarka commented Aug 14, 2018

Take look at ioredis, luckily it has compatibility with native promises

@t3ndai
Copy link

t3ndai commented Aug 14, 2018

Majority of people want Promises, majority of people are willing to contribute to make that happen. Opening a PR that pushes that idea forward would be wise idea, I think.

@insidewhy
Copy link

Even if I promisify this library, I'll lose typescript typings. If you want to use typescript and promises node_redis is essentially a no-go.

@NickStefan
Copy link

NickStefan commented Oct 18, 2018

This doesn't work:

const { promisify } = require('util');
const redisSADDSMEMBERS = (key, mem) =>
    promisify(redisClient
        .multi()
        .sadd(key, mem)
        .smembers(key)
        .exec).bind(redisClient);

nor does this

const { promisify } = require('util');
const redisSADDSMEMBERS = (key, mem) =>
    promisify(redisClient
        .multi()
        .sadd(key, mem)
        .smembers(key)
        .exec).bind(redisClient)();

Is this really the only way to do this?

const redisSADDSMEMBERS = (key, mem) => new Promise((resolve, reject) => {
    redisClient
        .multi()
        .sadd(key, mem)
        .smembers(key)
        .exec((err, response) => {
            if (err) reject(err);
            resolve(response);
        });
});

@jsejcksn
Copy link

I want native promises with this module, too. However—until it ships—what about this?

Just promisifying and using sendCommand:

const util = require('util');
const redis = require('redis');
const client = redis.createClient();
const clientAsync = util.promisify(client.sendCommand).bind(client);

(async function () => {
  console.log(await clientAsync('set', ['best-pizza', 'never pineapple'])); // OK
  console.log(await clientAsync('get', ['best-pizza'])); // never pineapple
  console.log(await clientAsync('del', ['best-pizza'])); // 1
  clientAsync('quit');
})();

Is there a technical problem with this approach?

@tbinna
Copy link

tbinna commented Apr 12, 2019

@ohjames just came across the handy-redis project https://github.com/mmkal/handy-redis

Haven't tested it myself but might be worth a try...

@leshow
Copy link

leshow commented Sep 23, 2019

@jsejcksn

AFAIK that method doesn't appear to work with client.mulit().x. Try to use util.promisify with client.multi().exec(cb) for instance, similar to what @NickStefan tried to. The only way is to manually write a new Promise.

@anasanzari
Copy link

+1

@insidewhy
Copy link

@anasanzari Just don't +1 k. This is what the "thumbs up" reaction is for. It doesn't annoy everyone in the list with a pointless message.

@zispo
Copy link

zispo commented May 19, 2020

I would like to request that if native promise support is implemented in the future that callbacks are not removed. We specifically look for libraries that support callbacks and not promises to reduce memory allocations. (And by the way, if callbacks are implemented on top of promises by the library for backwards compatibility then that would defeat the purpose, this would be pointless as internally the library would be doing all the allocations related to promises). Just to leave my two cents out there that the callback design is sometimes preferred.

@Syn9673
Copy link

Syn9673 commented Sep 1, 2020

I would like to request that if native promise support is implemented in the future that callbacks are not removed. We specifically look for libraries that support callbacks and not promises to reduce memory allocations. (And by the way, if callbacks are implemented on top of promises by the library for backwards compatibility then that would defeat the purpose, this would be pointless as internally the library would be doing all the allocations related to promises). Just to leave my two cents out there that the callback design is sometimes preferred.

if you're worrying about memory allocations, why use high level languages? If you care about low memory usage, then go use lower level langs.

@leibale leibale added this to To do in v4 Mar 8, 2021
@leibale leibale moved this from To do to In progress in v4 May 12, 2021
@leibale leibale moved this from In progress to Done in v4 Jun 24, 2021
@leibale leibale removed this from the v4.0.0 milestone Jun 29, 2021
@leibale leibale removed the v4 label Aug 31, 2021
@leibale leibale closed this as completed Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v4
Done
Development

No branches or pull requests