dai-shi / proxy-compare

Compare two objects using accessed properties with Proxy
https://www.npmjs.com/package/proxy-compare
MIT License
283 stars 18 forks source link

createProxy(object, affected, cache) should produce a new proxy if `affected` changes. #29

Closed justjake closed 2 years ago

justjake commented 2 years ago

I've been reading the code that consumes proxy-compare in a few different places, trying to work out the lifetime semantics for affected and cache.

Here are my references in downstream libraries:

https://github.com/pmndrs/valtio/blob/5ed0c7e081c80be4d947deadf1ed45875ca06a49/src/react.ts#L194-L195

  const proxyCache = useMemo(() => new WeakMap(), []) // per-hook proxyCache
  return createProxyToCompare(currSnapshot, affected, proxyCache)

https://github.com/dai-shi/react-tracked/blob/5b26f96c5138044667ee7f09ea586fd9493cc1b1/src/createTrackedSelector.ts#L57-L58

    const proxyCache = useMemo(() => new WeakMap(), []); // per-hook proxyCache
    return createProxy(state, affected, proxyCache);

I am concerned that there can be an issue where affected is a new, empty WeakMap, representing the listener's next run, which should produce a new, independent set of reads, but the proxyCache contains a cached proxy that internally references an old/stale affected WeakMap that has already been de-referenced.

If that occurs, isChanged(oldState, newState, newAffected) can return false, because we use a new empty affected map by the time we perform the compare, but our old, cached proxy is writing affected keys to a map we no longer reference.

Can you explain a bit more about what makes this caching correct? In my usage (closed source for now), I think I need to either always create fresh proxy instances for each run of my observable scope, or, I need to somehow clear the affected map.

justjake commented 2 years ago

Oh, nevermind! I misread the proxy creation code - the new affected is always assigned into the proxy:

https://github.com/dai-shi/proxy-compare/blob/c9e745b39743192108f8f6665451277c3edfbe8a/src/index.ts#L177

Makes sense!

dai-shi commented 2 years ago

the new affected is always assigned into the proxy

Yes, this is very important to proper handle concurrency in React, which many libs overlook.

(I think you are one of only a few people who read the code so deeply.)