muut / riotjs-admin

Administration panel – A Riot.js demo
https://moot.it/riotjs/demo/
125 stars 27 forks source link

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

Open beders opened 10 years ago

beders commented 10 years ago

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 commented 10 years ago

+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 commented 10 years ago

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

magwo commented 10 years ago

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 commented 10 years ago

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

tipiirai commented 10 years ago

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

(I was logged in as muut previously)

magwo commented 10 years ago

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

Commits here: https://github.com/magwo/elevatorcrush/commit/ffe8f3eb4262d9abeb9f97fb89b9825633c1c6ac https://github.com/magwo/elevatorcrush/commit/2ba592b525e686adbf3c2e4107b333a3d7551ad9

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 commented 10 years ago

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 commented 10 years ago

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

magwo commented 10 years ago

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