tildeio / rsvp.js

A lightweight library that provides tools for organizing asynchronous code
MIT License
3.61k stars 264 forks source link

missing done() is a bummer #373

Closed vitaly-t closed 9 years ago

vitaly-t commented 9 years ago

I've been working on my own library that's compatible with most of Promises/A+ implementations:

https://github.com/vitaly-t/pg-promise

And here's the result of running tests for compatibility that should give you an idea for a change ;)

Never mind Lie, it lost its momentum, but you see that all major competitions have done() in their implementation, so it is fair to say you guys really should add it also.

stefanpenner commented 9 years ago

We don't have plans to add done. In the aplus thread on done you will see @wycats's thoughts on it. Specifically how it is an anti pattern

vitaly-t commented 9 years ago

Popular usage patterns make specs, not someone's subjective views.

And with the promises competition tightening up you are voting yourself out by ignoring what's popular ;)

stefanpenner commented 9 years ago

And with the promises competition tightening up you are voting yourself out by ignoring what's popular ;)

popular !== good, the done pattern is IMHO an anti-pattern as it breaks the promise paradigm.

feel free to review some of the discussion: https://github.com/promises-aplus/promises-spec/issues/43 It is clearly a topic with many different opinions. Since that discussion has ended, I have continued to write complex asynchronous code without the need or want of done. I have not dismissed this proposed feature lightly, rather I have experimented with the approach time and time again, but continue to arrive at the same conclusion.

stefanpenner commented 9 years ago

RSVP - FAIL: missing method done(), have to replace with finally()

finally and done have very different semantics. I am extremely interested in pushing finally forward, and hopefully eventually it will become part of the spec. I do not have the same feeling about done.

If your use-case, is cleanup, or ensuring something runs regardless of outcome. Please use finally as that is it's intent and design. Promise libraries that do not implement it, should strongly consider implementing it.

vitaly-t commented 9 years ago

Promise is the only big player who doesn't have finally() implemented. They have the same arguments as yourself, just in the opposite direction :) And I'm just a user of these frameworks :)

I can't argue one approach over the other, I do not develop promise libraries, not yet anyway :)

I can only state the fact of the existing controversy between major implementations when it comes to methods done() and finally() :)

stefanpenner commented 9 years ago

They have the same arguments as yourself, just in the opposite direction

link to their issues on finally?

vitaly-t commented 9 years ago

no, can't find it, but i'm using the latest version of Promise, and finally isn't there, not even on to-do list.

stefanpenner commented 9 years ago

@vitaly-t they likely have valid concerns, I have several myself, especially around finally and ctrl-c of the process. I do not believe we have nailed these semantics down yet. But I believe we can, and should. Ultimately I believe my concerns with finally are solvable, but not with done.

tbremer commented 9 years ago

The thing I have really enjoyed and the reason I advocate for RSVP over other Promise implementations is that it doesn't put a lot of extra stuff around the Promises/A+ spec or it's test suite.

Just ¢.02

vitaly-t commented 9 years ago

@tbremer lots of extra stuff would signal that the spec wasn't good to begin with ;) I think we are missing some good usage tutorials to weight out every scenario and approach, case in point. I've been using promises for a while, but still find myself in the woods every so often, and I wonder why :) I don't think the spec is an overkill, if not an oversimplification, but any recommendations are totally missing.

stefanpenner commented 9 years ago

;) :)

you seem awfully happy

lots of extra stuff would signal that the spec wasn't good to begin with

the spec is rolling, I believe the initial release was solid. But will continue to be added to over time. finally and reflection are likely good candidates for future iterations, they need champions but the committee feels positive.

vitaly-t commented 9 years ago

@stefanpenner, that's a few pints talking, after a long week, and I respect you standing by not making changes that you think don't make sense. As I user, I just would like to see the top libraries get their game together regarding the protocol, so people can follow what I did with my library and say - "we support any Promises/A+" library without any reservation. https://github.com/vitaly-t/pg-promise

stefanpenner commented 9 years ago

As I user, I just would like to see the top libraries get their game together regarding the protocol.

lets figure out what the opposition to finally is, and try and solve it.

vitaly-t commented 9 years ago

kicking up some dirt: https://github.com/then/promise/issues/78

...to see what comes up :)

ForbesLindesay commented 9 years ago

Just for the record:

I/we have no objections to .finally, it's just that nobody's asked for it before, and it's not in any standards. Generally then/Promise works on the principal that you must have a really strong argument for adding anything that's not in the spec. That argument is pretty easy to make for .done, .nodeify and .denodeify. It has not previously been made for .finally. Ideally I would like to see a specification written up for .finally before implementing it, but I have wanted that functionality on a few occasions, so I can see that it may be justified as an addition.

ForbesLindesay commented 9 years ago

P.S. .finally and .done are completely orthogonal features. .done makes sure errors are not silenced, .finally saves you duplicating code when you want to do the same thing for success and failure.

stefanpenner commented 9 years ago

@ForbesLindesay ya. They are totally orthogonal, but I believe people use done often by mistake. When they want finally.

A spec. Or proposal for a spec is a good does. I believe the async finally semantics are currently extremely broken. Just consider what happens if the use process.exits during one of these gaps, the async finally is not called. This results in leaking cleanup steps. The only solution seems to layer something else on-top. Which seems like a failure of the spec.

I believe this is also a problem with try / finally and generators or async await.

If there is interest ( there seems to be ) I can start enumerating scenarios that can be used as the basis for a spec. I just likely need guidance as spec writing isn't something I am familiar with.

ForbesLindesay commented 9 years ago

I think the best bet would probably be to start an issue in the promises/A+ spec repo. It's pretty friendly to people who aren't used to writing specs.

It's worth noting that finally is never more than a best effort. e.g. consider what happens if the power cord is cut. The other issue is that most platforms only let you do synchronous work when they are about to exit, which would be severely limiting when it comes to performing any cleanup.

stefanpenner commented 9 years ago

It's worth noting that finally is never more than a best effort. e.g. consider what happens if the power cord is cut. The other issue is that most platforms only let you do synchronous work when they are about to exit, which would be severely limiting when it comes to performing any cleanup.

Ya, I believe these limitations need to be enumerated, and the scenarios that could be made to work and the ones that can not be made to work should be documented.

vitaly-t commented 9 years ago

So much talking about a perfect architecture, while it's being failing tests for 2 weeks now :(

stefanpenner commented 9 years ago

@vitaly-t low-blow, thanks for submitting a PR to help fix... anyways it works, issue are a dep of the Build tooling. Just been obscenely busy. Will sort shortly

vitaly-t commented 9 years ago

Just so, on what's been discussed, Promise just added finally() to their midst ;)

https://github.com/then/promise/pull/80

Are you still on the fence for adding done()? :)

stefanpenner commented 9 years ago

Ya. done is extremely unlikely to be added here