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

Introduce 'promise' mode #35

Merged
merged 38 commits into from
Jul 7, 2016
Merged

Introduce 'promise' mode #35

merged 38 commits into from
Jul 7, 2016

Conversation

medikoo
Copy link
Owner

@medikoo medikoo commented Jul 5, 2016

At this point promise returning functions are memoized natural way, which seems ok, but there's one flaw, Rejected promise results are treated same as successful ones, that's not right.

We should not store them, same way as we do not store sync function crashes, or errors in async mode.

@medikoo medikoo self-assigned this Mar 1, 2015
@medikoo medikoo added this to the v1 milestone Mar 1, 2015
@theothermattm
Copy link

+1 could definitely use this!

@asaf-romano
Copy link

+1.

Here's a temporary workaround I'm using (arity is a local helper that sets the length of a function):

function doMemoizeFunction(func, options)
{
    let memoized = memoize(arity(options.length || func.length,
                                 function(...args)
                                 {
                                     let result = func.apply(this, args);
                                     if (result && typeof result.then == "function")
                                     {
                                         result.then(null, () => memoized.delete(...args));
                                     }
                                     return result;
                                 }), options);
    return memoized;
}

@medikoo
Copy link
Owner Author

medikoo commented Jan 20, 2016

Other workaround, can look as:

var memoized = memoize(function () {
  var context = this, args = arguments;
  return returnsPromise.apply(context, args).then(null, function (err) {
    memoized.delete.apply(context, args);
    throw err;
  });
}, { length: returnsPromise.length });

Still it's not ideal when combined with other options as maxAge, dispose etc.

@Kirill89 Kirill89 mentioned this pull request Jan 22, 2016
@puzrin
Copy link

puzrin commented Feb 21, 2016

I have a couple of strange question:

  • Does current architecture approach still plays well for pending issues?
  • Does it worth to split sync / async / promise functionality to separate packages?

@medikoo
Copy link
Owner Author

medikoo commented Feb 22, 2016

@puzrin indeed your questions are not clear to me. Answering best I could:

  • I don't see bigger issues with current architecture. Some issues wait longer for resolution mainly because my time is very limited at this point, and there's hardly other contributors around.
  • I don't envision splitting this package into many packages. I think it works best as one package with sub functionalities split into modules.

@puzrin
Copy link

puzrin commented Feb 22, 2016

I considered, does it worth to create separate package for promises with my own efforts. Seems, that will have almost no advantages (+ additional time spends for maintenance).

@medikoo
Copy link
Owner Author

medikoo commented Feb 23, 2016

@puzrin you can introduce external extensions, e.g. that way:

// Configure new extension
var myExtension = function (myExtensionOptions, memoizedFunctionData, memoizeOptions) {
 ...
};
require('memoizee/lib/registered-extensions').myExtension = myExtension;

// Memoize function and use provided extension
var fn = function (...) {...}
var memoizedFn = memoize(fn, { myExtension: myExtensionOptions })

Just be sure to not use promise name so it won't collide with one that will soon be added.

@puzrin
Copy link

puzrin commented May 29, 2016

https://github.com/nodeca/promise-memoize did a separate simple package. No weakmaps, no lru, no stat... but it works :). Also does not drop cache on prefetch and unref timers.

@medikoo
Copy link
Owner Author

medikoo commented May 30, 2016

https://github.com/nodeca/promise-memoize did a separate simple package.

Great if that works for you, and sorry that I couldn't handle that one earlier

@medikoo medikoo merged commit e0d3cfc into master Jul 7, 2016
@medikoo medikoo deleted the promises2 branch July 7, 2016 13:27
@medikoo
Copy link
Owner Author

medikoo commented Jul 7, 2016

Published with v0.4.0

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.

5 participants