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

Consider switching memoize cache to be compatible with ES6 Map/WeakMap. #1862

Open
jdalton opened this issue Sep 19, 2014 · 11 comments
Open

Comments

@jdalton
Copy link
Contributor

jdalton commented Sep 19, 2014

At the moment the _.memoize cache is just a plain object. If we were to switch it to being a simple wrapper around the cache object (still doing the same param to string key use) but with the interface of ES6 Map, WeakMap that would allow devs to swap in ES6 Maps/WeakMaps for their cache. The thin wrapper would have an interface to mimic Map/WreakMap, so has, set, get, and optionally (delete, clear).

_.memoize could have a Cache constructor bolted on to it _.memoize.Cache to allow devs to swap it out with a Map/WeakMap or equiv shim as well.

Map/WeakMap are available on all modern browsers, node --harmony.

Thoughts?

@joshuacc
Copy link

WreakMap

Freudian slip? ;-)

@jdalton
Copy link
Contributor Author

jdalton commented Sep 19, 2014

LOL, Imma keep it 😀

@akre54
Copy link
Collaborator

akre54 commented Sep 19, 2014

I'm a tenuous thumbs down on this. Underscore isn't a Maps library and this would be quite a rabbit hole to go down. It might be something to explore in 2.0 or 3.0 however, depending on how much code it would add.

@jdalton
Copy link
Contributor Author

jdalton commented Sep 19, 2014

I'm a tenuous thumbs down on this. Underscore isn't a Maps library and this would be quite a rabbit hole to go down.

I'm not saying Underscore should implement anything more than its simple cache look up as it is now. I'm suggesting it wrap it with an interface that is compatible with ES6 Map/WeakMap to allow devs to swap them out.

Related to #1841.

@akre54
Copy link
Collaborator

akre54 commented Sep 19, 2014

sounds reasonable. might be nice to add support for a few other modern features for 2.0 too.

@joshuacc
Copy link

I'm 👍 on this one.

@megawac
Copy link
Collaborator

megawac commented Sep 19, 2014

How would the fallback work? Should we change it to using an array

@jdalton
Copy link
Contributor Author

jdalton commented Sep 19, 2014

How would the fallback work? Should we change it to using an array

There would be no fallback. We don't change the existing value -> use as key (coerce to string) behavior. This way it's consistent in old and new environments. By adopting an interface of Map/WeakMap we allow devs to use them or shims without having to implement a complex map implementation on our end.

For exampe devs could do _.memoize.Cache = WeakMap; and _.memoize wouldn't care. It'd be all "Oh the _.memoize.Cache constructor returns a cache object with the interface I'm expecting (has, set, etc)" and would use it just as it would the basic Cache object.

It could look something like:

function Cache() {
  this.__wrapped__ = {};
}
_.extend(Cache.prototype,
  get: function(key) {
    return this.__wrapped__[key];
  },
  has: function(key) {
    return _.has(this.__wrapped__, key);
  },
  set: function(key, value) {
    this.__wrapped__[key] = value;
    return this;
  }
});

_.memoize = function(func, hasher) {
  var memoize = function(key) {
    var cache = memoize.cache;
    var address = (hasher ? hasher.apply(this, arguments) : key);
    if (!cache.has(address)) cache.set(address, func.apply(this, arguments));
    return cache.get(address);
  };
  memoize.cache = new _.memoize.Cache;
  return memoize;
};

_.memoize.Cache = Cache;

@bnjmnt4n
Copy link
Contributor

👍

@jgonggrijp
Copy link
Collaborator

jgonggrijp commented May 9, 2020

Maybe depends on #2453.

Edit: no, it doesn't.

@jgonggrijp
Copy link
Collaborator

2.0 Should just use Map internally. No need for fancy wrappers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants