search

facebookexperimental / Recoil

Recoil is an experimental state management library for React apps. It provides several capabilities that are difficult to achieve with React alone, while being compatible with the newest features of React.
https://recoiljs.org/
MIT License
19.61k stars 1.19k forks source link

Naive memoization in selector family doesn't work #776

Open idubrov opened 3 years ago

idubrov commented 3 years ago

I'm trying to setup a very naive memoization for a selectorFamily so it returns the same value (referentially) if the result of computation is the same as in the last invocation.

Selector I have returns an array of strings, and even if it is structurally the same as the previous computation, it still causes dependent selectors to recompute since Recoil uses referential comparison for computed selector values (if I understand correctly).

So, I thought that I can store "last" value in some variable and then check if newly computed value is the same (compare new array of string with the previous one), and if so, just return the previous one.

However, I didn't find a good place to store my "cache". I tried to create it in get closure of the selector, hoping that it will be called once for each selector parameter (and retrieved from the cache here, on subsequent invocations), but it turns out that it is being called every time.

See the example here: https://codesandbox.io/s/intelligent-lehmann-36fl8?file=/src/App.js

const lastValue = undefined; at line 18 is my attempt at creating the "cache".

If I click "add" button (which just adds a new component reading the same selector state), it prints to the console "created for key hello".

I looked at the Recoil sources and it looks like the cause is this line: https://github.com/facebookexperimental/Recoil/blob/ca888d3c3a55a3128279d0124ebca9002377770b/src/recoil_values/Recoil_selectorFamily.js#L108

Even though it caches selector by its parameters, it calls options.get(params) every time.

My question is would it make sense to only call it once? Replace:

const myGet = callbacks => options.get(params)(callbacks);

with

const get2 = options.get(params);
const myGet = callbacks => get2(callbacks);

? Or is there some reason for it is being called each time?

drarmstr commented 3 years ago

That change should probably be safe. The nested evaluation callback function itself should be idempotent, but exceptions are allowed for logging and caching. Feel free to explore with a PR if you like.