senecajs / seneca

A microservices toolkit for Node.js.
http://senecajs.org
MIT License
3.96k stars 314 forks source link

Promise support for common API #26

Closed oliverzy closed 8 years ago

oliverzy commented 10 years ago

Hi,

When I experiments with Seneca, I found two much callback needed especially about data entity.

Promise is a very good way to improve callback hell and provide robust error handling mechanism.

What about adding promise support for seneca.act and data entity api like save$/load$ and so on?

Trello Card

rjrodger commented 10 years ago

Yes!

I completely agree :)

I already did some experiments with this using bluebird

0.5.16 is nearly ready, but I can look at this for 0.5.17

AdrianRossouw commented 10 years ago

While it's really easy to write the boilerplate to turn a function into a promise aware version, there are some libraries that do it for you.

I happened to be writing some code using the elasticsearch client library, which exposes all it's methodd as promise returning functions through CujoJS' when library.

It has a method called fn.lift (api docs), which does it for you. Interestingly it also has one named fn.liftAll (api docs) which does it for all the methods attached to an object. I might try it out later and see how it goes.

ChenRoth-old commented 10 years ago

@rjrodger bluebird would be a good choice because it's lighter, right?

AdrianRossouw commented 10 years ago

So this is not an actual suggestion, because of how immature this library is. I'm kind of enamoured with what highland.js does.

It provides a stream-like wrapper around various async mechanisms, that (theoretically) allows you to mix streams, callback and promises as needed.

When you add a callback to seneca, I don't think the problem is really that it doesn't give you a promise, but rather that it doesn't allow you to return a promise.

I also kind of feel that the natural api that seneca should be built around is streams. Not promises or callbacks. The pattern matching feels like it should just be a filter, and you should just be piping to an out-stream.

myndzi commented 10 years ago

Boy would I like to see this. Streams do make sense for the handlers, but handlers returning promises makes them extremely useful. (Also note 'object mode' streams in the node streams api, if you're not familiar with it). I've done a number of experiments on this count, but it seems like you're well along into it, which is super nice for me :)

One snag I've run into is the ability to use promise scoping. Once a message goes out to a task queue or whatever, it loses the local context it had. Promises can't fix this really, because even though you could maintain a long-running promise, what happens to the message when your service crashes or is restarted? What if it has a dozen steps in it? Now you have to start it all over, which can be problematic if the steps aren't idempotent.

Anyway, bluebird is fantastic and 2.0 branch is even better. I strongly recommend it.

Edit: I was just reading through the docs with this specific case in mind, and I realized Seneca doesn't really solve it either. If the requester closes before the responder responds, what happens then?

rjrodger commented 10 years ago

Hi guys - just picking up this thread - we are adding much better Seneca-transport plugins for 0.5.18, and also req/res tracking to handle some of these failure modes

asafyish commented 9 years ago

+1

yieme commented 9 years ago

+1

Rolilink commented 9 years ago

+2

boxxxie commented 9 years ago

does bluebird.promisfyAll() not work for seneca?

sdd commented 9 years ago

Would it be possible to add this single line to seneca.js so that https://github.com/tasinet/seneca-bluebird would work without having to modify seneca.js manually? This would allow us to use Bluebird based promises.

private$.exports.Entity = Entity;
sdd commented 9 years ago

Ignore my query. I've just seen the legacy callback message in my console output from seneca. :-)

sdd commented 9 years ago

seneca-bluebird will work by modifying 3 files.

Add the following to the end of seneca/lib/entity.js:

module.exports.Entity = Entity;

Add the following to seneca/seneca.js, in make_seneca(), before it returns:

private$.exports.Entity = make_entity.Entity;

Change seneca-bluebird/promisify.js to remove the done argument from the main exported function.

dotmilk commented 9 years ago

Any word on this? I'd prefer not to have to use a module that has to modify seneca to get support for bluebird, and the arcane nature of seneca has made it resist my attempts to promisify entity / save$ or anything I have attempted.

svperfecta commented 9 years ago

+1

svperfecta commented 9 years ago

This bluebird module is pretty close. Unfortunately it requires some hacking to core modules. Would be nice to just drag this in.

mrnerdhair commented 9 years ago

I've submitted PRs to seneca and seneca-bluebird to implement the changes @sc0ttyd identified. Hopefully we can get seneca-bluebird into a proper npm package soon. I've really taken to a promisified seneca, especially now that Node v4 is here! My main code is all inside a generator function lifted with bluebird's Promise.coroutine, and it lets me do a one-liner like "yield seneca.readyAsync();" to wait for seneca's initialization intead of unwieldy callback nesting.

sdd commented 9 years ago

Thanks @reidrankin :+1:

svperfecta commented 9 years ago

Very nice!

On Monday, September 14, 2015, Scott Donnelly notifications@github.com wrote:

Thanks @reidrankin https://github.com/reidrankin [image: :+1:]

— Reply to this email directly or view it on GitHub https://github.com/rjrodger/seneca/issues/26#issuecomment-140219133.


Brian Corrigan Managing Partner / Principal Engineer at MadGlory https://www.linkedin.com/company/2657284 Wb: http://madglory.com http://www.madglory.com Tw: @madglory Ph: 518.867.1439

faceleg commented 8 years ago

For what it's worth, I've been using bluebird and:

seneca.actAsync = Promise.promisify(seneca.act);

With much success.

svperfecta commented 8 years ago

I think I saw @kixxauth or @blainsmith do something similar On Fri, Oct 16, 2015 at 6:11 AM Michael Robinson notifications@github.com wrote:

For what it's worth, I've been using

seneca.actAsync = Promise.promisify(seneca.act);

With much success.

— Reply to this email directly or view it on GitHub https://github.com/rjrodger/seneca/issues/26#issuecomment-148675024.

blainsmith commented 8 years ago

Yes, what @faceleg said. Works like a charm.

mcdonnelldean commented 8 years ago

@faceleg @blainsmith Does this work ok? Could we get the basis of a simple guide going here to put on the site? I think this is the best way to handle this until we get to the stage where we can explore it further.

blainsmith commented 8 years ago

Sure. I can add a new tutorial to this page (http://senecajs.org/tutorials) "Promisified Seneca" or something along those lines. It'll be a fairly short one, but at least it will be something.

mcdonnelldean commented 8 years ago

@blainsmith Oh this would be awesome. If you add it here: https://github.com/senecajs/senecajs.org/tree/master/src/pages/drafts I can even get our resident write to do magic on it for you afterwards!!

AdrianRossouw commented 8 years ago

how do promises tie into the 'gating' stuff i have seen in the api?

mcdonnelldean commented 8 years ago

@AdrianRossouw good question.

AdrianRossouw commented 8 years ago

https://github.com/rjrodger/gate-executor

that has promises written all over it.

mcdonnelldean commented 8 years ago

From what I read about this will be a thing at some stage but for now at least having documents explaining how to do it is good middle ground :D

blainsmith commented 8 years ago

@AdrianRossouw If you promisify seneca.act and the gate executer times out then the promise is treated as rejected and caught. I just verified this while wiring the tutorial. I will polish this up and PR it soon.

faceleg commented 8 years ago

@blainsmith I love you

blainsmith commented 8 years ago

:boom: https://github.com/senecajs/senecajs.org/pull/87

mcdonnelldean commented 8 years ago

@blainsmith Your so epic :D

nfantone commented 8 years ago

Any updates or plans on this? I'd love to see not only this baked into the core, but a tutorial on promisifying entities methods (such as save$). Some hacky attempts were made before, but it seems they have become obsolete by now.

geek commented 8 years ago

@nfantone I think we are definitely open to it. It hasn't become a major priority though, as there are ways around it, as Blain wrote about in his tutorial. But, we are definitely happy to consider a PR to add in promises.

nfantone commented 8 years ago

@geek The thing is, the tutorial only approaches "promisification" of seneca.act. What's the "way around" entities' API? I don't think there's such a straight forward way for those, is there?

nfantone commented 8 years ago

@geek @oliverzy Also, would you consider opening up the Trello board for the community? That way, anyone could see progress and/or vote up/down features and see what others would like to see implemented. It'd help re-organize priorities, as well.

nfantone commented 8 years ago

Just wanted to say, for future reference, that this:

  var entity = seneca.make(base, name, payload);
  var save$ = Promise.promisify(entity.save$, { context: entity });
  return save$();

Works wonders. Maybe we could include a mention in the tutorial?

geek commented 8 years ago

@nfantone we have moved away from the trello board to plain old github issues and milestones!

Please fix the tutorial, its at https://github.com/senecajs/senecajs.org/blob/master/src/pages/tutorials/seneca-with-promises.md

nfantone commented 8 years ago

@geek Will do!

japel commented 8 years ago

@geek was just trying out the example at: https://github.com/senecajs/senecajs.org/blob/master/src/pages/tutorials/seneca-with-promises.md#advanced-example

and got:

2016-03-11T11:42:40.685Z b5tr3kr5vzx8/1457696560620/1544/- INFO hello   Seneca/1.3.0/b5tr3kr5vzx8/1457696560620/1544/-  
[TypeError: Object #<Object> has no method 'idgen']

node v0.10.41

geek commented 8 years ago

@japel looks like a bug, opened an issue here: https://github.com/senecajs/seneca/issues/370

mcdonnelldean commented 8 years ago

Issue on bug is open, see tutorial for how to enable.