-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Comments
Freudian slip? ;-) |
LOL, Imma keep it 😀 |
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. |
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. |
sounds reasonable. might be nice to add support for a few other modern features for 2.0 too. |
I'm 👍 on this one. |
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 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; |
👍 |
Edit: no, it doesn't. |
2.0 Should just use |
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 aCache
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?
The text was updated successfully, but these errors were encountered: