Add several missing features to treeSelect #21625
Conversation
While trying to use `treeSelect` to optimize `stats/lists/selectors`, I found several missing features that make it impossible to use there. 1. Allow custom `getCacheKey` function. Stats selectors get a `query` object that is both used as a key in serialized form and the selector needs to access its properties. We need to allow object arguments and serialize them properly in `getCacheKey`. 2. Allow clearing the cache by calling `selector.clearCache()`. Used by tests. 3. Don't crash on an invalid `WeakMap` key when `getDependents` returns `null` or other falsy value as one of the dependencies. That happens all the time when the required dependency is being requested from network and its value in Redux state is `null` in the meantime. I consider all falsy values (`null`, `undefined`, `false`, ...) as identical and map them to one `WeakMap` key. Discussion whether that's a good idea or whether we should distinguish between them has a great bikeshedding potential, but I don't expect any trouble in practice.
jsnajdr
added
[Status] Needs Review
|
|
||
| cachedSelector.clearCache = () => { | ||
| cache = new WeakMap(); | ||
| }; |
There was a problem hiding this comment.
this needs a comment explaining that WeakMap has no .clear() or we should use an object that is like a WeakMap that does have .clear() - someone's bound to change this otherwise when they don't realize that
| /* | ||
| * This object will be used as a WeakMap key if a dependency is a falsy value (null, undefined, ...) | ||
| */ | ||
| const STATIC_FALSY_KEY = {}; |
There was a problem hiding this comment.
I actually liked the MixedMap @samouri originally had in here. It had clear semantics and didn't require these odd workarounds. Is there real opposition against using MixedMap?
There was a problem hiding this comment.
The semantics now is that the dependents are of type Maybe<Object> -- that looks quite clean to me. If I encounter a place where I need the MixedMap, I'll start rooting for it.
There was a problem hiding this comment.
If we only have a need to support null/undefined/false, then i don't think we should incorporate MixedMap which allowed for unbounded primitives like number and string
client/lib/tree-select/index.js
Outdated
| * The last map is a regular one because the the key for the last map is the string results of args.join(). | ||
| */ | ||
| function insertDependentKey( map, key, currentIndex, arr ) { | ||
| key = key || STATIC_FALSY_KEY; |
There was a problem hiding this comment.
how would you feel about making this a separate variable so we don't mutate the args? I find this often confusing to read code that does.
function insertDependentKey( map, rawKey, currentIndex, arr ) {
const key = rawKey || STATIC_FALSY_KEY;
…
}There was a problem hiding this comment.
Introduced a weakMapKey variable.
Thanks for iterating here @jsnajdr - I knew these objects would be a valid use-case for object-args. I'm not super-fond of the name |
|
@dmsnell I'm OK with the |
samouri
left a comment
samouri
left a comment
There was a problem hiding this comment.
Code LGTM, thanks for the improvements @jsnajdr!
One thing I disagree on is the need for a clearCache. It'll only ever be used in tests, and you can easily have this code in beforeEach:
beforeEach(() => {
selectorToClear = treeSelect(...);
})I wouldn't block on that though, merge away!
sirreal
removed
the
[Status] Needs Review
While trying to use
treeSelectto optimizestats/lists/selectors, I found several missing features that make it impossible to use there.Allow custom
getCacheKeyfunction. Stats selectors get aqueryobject that is both used as a key in serialized form and the selector needs to access its properties. We need to allow object arguments and serialize them properly ingetCacheKey.Allow clearing the cache by calling
selector.clearCache(). Used by tests.Don't crash on an invalid
WeakMapkey whengetDependentsreturnsnullor other falsy value as one of the dependencies. That happens all the time when the required dependency is being requested from network and its value in Redux state isnullin the meantime. I consider all falsy values (null,undefined,false, ...) as identical and map them to oneWeakMapkey. Discussion whether that's a good idea or whether we should distinguish between them has a greatbikeshedding potential, but I don't expect any trouble in practice.