-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
Added promise support #50
Conversation
@medikoo is PR ok in general? Comments:
|
Great thanks for that work. Still unfortunately it can't work properly in that form. To have it really sound it should work similarly as async extension, with One difference that should make promise version a bit simpler, is that we should cache promise results and just return them if query with same arguments is made (so there's no two steps operation as in async case, where we wait for actual result to have it cached). Tests coverage should be exactly same as in case of For tests it'll be great to introduce some very simple promise implementation (that should be included in Using if (typeof promise.done === 'function') {
// If `done` is implemented rely on it, as it's most optimal value resolver
promise.done(null, function (err) {
// delete from cache
});
} else {
promise.then(null, function (err) {
nextTick(function () { // escape error interception
// delete from cache
});
});
} |
Partially fixed. One big problem left: to work with I don't know what to do:
What to you think? |
For #51 probably some bigger overhaul is needed, and I plan to do one sometime this year (It will cover also #34 and some other things that can't be just patched on current version) When this extension is concerned, it's possible to bring it now, but it really should be done by copying If you decide not to continue work on that now, I'll probably find a day for it sometime in Februrary. |
Take a look. I don't know how good is it in your eyes, but we will not do better. At least, that could save your time. |
@puzrin Thank you! It looks way closer, still it misses proper handling in other extensions as |
Let's wait Feb then, no problem. |
This work is continued at #35 Therefore closing this PR |
Fixes #35