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

Added promise support #50

Closed
wants to merge 1 commit into from
Closed

Added promise support #50

wants to merge 1 commit into from

Conversation

Kirill89
Copy link
Contributor

Fixes #35

@puzrin
Copy link

puzrin commented Jan 22, 2016

@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
Copy link
Owner

medikoo commented Jan 25, 2016

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
Copy link

puzrin commented Jan 25, 2016

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
Copy link
Owner

medikoo commented Jan 25, 2016

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
Copy link

puzrin commented Jan 27, 2016

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
Copy link
Owner

medikoo commented Jan 27, 2016

@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
Copy link

puzrin commented Jan 27, 2016

Let's wait Feb then, no problem.

@medikoo medikoo self-assigned this May 2, 2016
@medikoo
Copy link
Owner

medikoo commented Jul 7, 2016

This work is continued at #35 Therefore closing this PR

@medikoo medikoo closed this Jul 7, 2016
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

Successfully merging this pull request may close these issues.

3 participants