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

Promises implementation not working if results are available right away #5

Open
beders opened this issue Feb 15, 2014 · 9 comments
Open

Comments

@beders
Copy link

beders commented Feb 15, 2014

If any of the back-end functions returns a result right away and calls always(..), the event is missed as the always callback is yet to be registered.

In slightly better English:
Callback handlers registered after the promise is fulfilled are not being called.

@magwo
Copy link

magwo commented Sep 23, 2014

+1

This is due to the "Promise" implementation being too simplistic - it does not not automatically call registered handlers if they are registered after the event. Also, there is nothing preventing the promise to, erronously, be "resolved" to a success after being resolved to an error.

This should be solved by statefulness and automatic calling once the promise has been resolved. I think it's still possible to make a simple and short implementation by reusing Riot.js event features, but it needs some more logic and state..

It is wrong and misleading to call it a "generic promise interface" when it does not even behave similar to most promise implementations.

@ghost
Copy link

ghost commented Sep 23, 2014

@magwo makes sense. Thanks for pointing that out. Will be fixed.

@magwo
Copy link

magwo commented Sep 23, 2014

Cool. I think that this implementation would be highly interesting to include in Riot.js.

It's such a powerful and useful construction in JS code, and used very often. If it can be robustly implemented in a compact way, I would very much like to see it included in Riot.

Edit: It also goes hand-in-hand with the philosophy that jQuery is fine, but it needs to work outside of the DOM, which Riot.js solves partially with its observable pattern. jQuery promises/deferred are fine albeit messy and overcomplicated, so Riot.js could provide a minimalistic, fast and DOM/ajax-agnostic implementation.

@magwo
Copy link

magwo commented Sep 23, 2014

moot, are you working on a patch? Otherwise I might take a shot..

@tipiirai
Copy link
Contributor

Please do! I'm not working on it atm.

(I was logged in as muut previously)

@magwo
Copy link

magwo commented Sep 25, 2014

Ok I wasnt sure how you wanted the tests written, so I did the implementation and tests in a personal, unrelated repository.

Commits here:
magwo/elevatorsaga@ffe8f3e
magwo/elevatorsaga@2ba592b

Tests running here:
http://magwo.github.io/elevatorcrush/test/

Promise implementation here:
https://github.com/magwo/elevatorcrush/blob/ffe8f3eb4262d9abeb9f97fb89b9825633c1c6ac/base.js#L18

Let me know what you think!
I removed the fn argument to the Promise constructor, as it seemed superfluous. I'm assuming it stems from an effort to make the interface similar to the Promises/A interface that looks like this:
var promise = new Promise(function(resolve, reject) {

...which I personally find confusing, especially regarding beginner coders. I much prefer the idea of resolving by just calling done() or fail() on the promise object. Or am I misunderstanding some crucial feature of promises?

Edit: Updated with correction - wasn't correctly calling functions registered after resolution.

@magwo
Copy link

magwo commented Sep 25, 2014

However, I have been thinking about attempting to make a Promises/A+ compliant minimalistic implementation, that would work well as a shim for browsers that do not have native Promises yet.
http://www.html5rocks.com/en/tutorials/es6/promises/

@tipiirai
Copy link
Contributor

Looking good. Any chance to make a pull request? Would be much easier for me .

@magwo
Copy link

magwo commented Sep 26, 2014

Yeah sure, just might not get the tests included in the pull request.

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

3 participants