medikoo / memoizee

Complete memoize/cache solution for JavaScript
ISC License
1.73k stars 61 forks source link

Added promise support #50

Closed Kirill89 closed 8 years ago

Kirill89 commented 8 years ago

Fixes #35

puzrin commented 8 years ago

@medikoo is PR ok in general?

Comments:

  1. There is a side effect - prefetch kills item cache on the start, instead of "transparent update on the end if succeed". But since this problem exists in async too, we left it intact (for separate issue).
  2. Promis is required for test only, and needs polyfill for node 0.10. I don't know your preferences, and we did not added anything.
medikoo commented 8 years ago

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 async* events emitted, and handled in various extensions (as e.g. here -> https://github.com/medikoo/memoizee/blob/master/ext/dispose.js#L14 ).

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 async mode. So whenever async mode is tested, with same tests we should follow for promise mode.

For tests it'll be great to introduce some very simple promise implementation (that should be included in devDependencies). I would propose to rely on plain-promise for that (as we should not assume native Promise is implemented).

Using catch for eventual rejection resolution is not safe. First reason is that it's promise transform function that intercepts errors (any crashes that can happen in catch callback won't be exposed). Second reason is that we should support any thenables (not just native promise), and by definition all that thenable is required to implement is then. Safe approach for value resolution would be as follows:

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
    });
  });
}
puzrin commented 8 years ago

Partially fixed. One big problem left: to work with dispose, plugin logic should be copied from async (now it's like a sync). But due to #51, that logic is not perfect and should be rewritten. And that was a reason why we did simple kludge for promises instead of full implementation.

I don't know what to do:

  1. If you could release current implementation, i will ask @Kirill89 to clone existing async tests, and disable broken one (like for dispose). IMHO current code seems to be better than nothing :)
  2. If current state is not acceptable for release, i'm not sure we can continue, because change of async requires a noticeable time.

What to you think?

medikoo commented 8 years ago

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 ext/async.js to ext/promise.js and re-editing it. Any other version would be just not right for other extensions, and I think it's better to not have it all, than trying to take something that will introduce buggy version.

If you decide not to continue work on that now, I'll probably find a day for it sometime in Februrary.

puzrin commented 8 years ago

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.

medikoo commented 8 years ago

@puzrin Thank you! It looks way closer, still it misses proper handling in other extensions as dispose. If you can't finalise it, I will be able to do it sometime in Februrary.

puzrin commented 8 years ago

Let's wait Feb then, no problem.

medikoo commented 8 years ago

This work is continued at #35 Therefore closing this PR