Closed sarimarton closed 1 year ago
Thanks for reporting! Sounds like an edge case, but still very important.
(As I keep saying at somewhere), using snap object in deps or memo leads to unexpected behavior. In general, I wouldn't consider it a bug, but wish our linter can catch it.
Playing with your sandbox, I think this should be fixed.
import { useEffect } from "react";
import { proxy, useSnapshot } from "valtio";
let renderCount = 0;
const obj = {};
const store = proxy({ slice: { key: obj } });
export default function App() {
const snap = useSnapshot(store);
useEffect(() => {
setTimeout(() => {
store.slice.key = obj;
}, 1000);
}, [snap.slice]);
return <div>render count: {++renderCount}</div>;
}
https://codesandbox.io/s/valtio-snapshot-unstable-reference-bug-forked-pj59yh
It shouldn't keep rendering. I'll open a PR later.
There are 2 separate issues here, both has been fixed in a follow-up implementation I link below.
(1) Snapshot behaves differently depending on whether a key is assigned to a primitive or object (2) Snapshot changes its reference when a key is set on the proxy
In my understanding, (1) is an obvious bug, and (2) is more of a design issue.
About (2): equality checks in render are a fundamental mechanics in react hooks (useMemo, useCallback, useEffect etc.). It's definitely the snapshot which should be used in the dep arrays, not the primary proxy: (2a) they should be tracked as well (2b) the useProxy concept (even the macro) correctly automates the render/callback separation, so it does the job in this way (2c) automated render/callback separation is not just a dev experience booster, but it's the only way which makes it possible to comply with the react linter. It's very important.
I understand that the snapshot concept comes with ref-renewal on any change. But it only highlights the weakness of the concept - the use case itself is solid.
There are a few consequences from these:
Having said all this, I followed up on the latest deep-switch-proxy POC from https://github.com/pmndrs/valtio/discussions/620#discussioncomment-4703484, and modified it that it's never the snapshot value which it returns, but always the primary proxy on leaf values, and it uses the proxy values as cache keys on non-leaf nodes, not the snapshot ones. The snapshots are only used for one thing: tracking.
It seems to have fixed both issues.
Here is the sandbox with a few detailed tests, the 3rd one pretty quickly blows up the current status quo: https://codesandbox.io/s/valtio-snapshot-unstable-reference-bug-forked-jmio2d?file=/src/App.tsx
(1) Snapshot behaves differently depending on whether a key is assigned to a primitive or object
snapshot()
should work like that (if not, it can be a bug), but useSnapshot()
with tracking doesn't always work as expected, and it's a known limitation (not fixable by design).
I'll check your rest of comment later. I think I should fix what I think is a bug in my previous comment.
I meant useSnapshot
everywhere by mentioning snapshot
It shouldn't keep rendering. I'll open a PR later.
https://codesandbox.io/s/valtio-snapshot-unstable-reference-bug-forked-g5izjm?file=/src/App.tsx
(1) Snapshot behaves differently depending on whether a key is assigned to a primitive or object (2) Snapshot changes its reference when a key is set on the proxy
I'd consider (1) is fixed. It now behaves the same between primitives and objects. (2) is a design decision for now. It's the definition of snapshot in valtio.
For (2) and discussion in https://github.com/pmndrs/eslint-plugin-valtio/issues/40, we still need more discussions and experiments. I'd appreciate if you open a new discussion in this repo, please.
Great!
Summary
useSnapshot reference is unstable, when a subkey is assigned to an object. This causes infinite rendering when using it as a useEffect dependency, in which it's being assigned to an object. The snapshot object is stable when the subkey is assigned to a primitive value, not an object. Note that it doesn't matter if the child key's new value is a newly created object or the same one. What matters only is that it's an object or not. The expected behavior would be that a snapshot parent object reference is stable no matter what kind of value a subkey is assigned to.
This code re-renders every second:
If store.slice.key is assigned to e.g. a different number, the slice reference is stable and it only re-renders once:
proxy vs snapshot in the dep array
Note that it's a possible workaround to use the proxy reference in the dependency array:
But this is a controversial usage of Valtio, because on one side, it satisfies the react linter, but on the other side, the primary proxy isn't supposed to be used for read usage in the render function. This controversy is inherent to the useSnapshot style, which is resolved by the useProxy hook (either the macro or the new one in 1.9.0):
The useProxy satisfies the react linter, but as it - correctly - resolves to the first version, it triggers the bug as well.
Link to reproduction
https://codesandbox.io/s/valtio-snapshot-unstable-reference-bug-j7406p?file=/src/App.tsx
Check List
Please do not ask questions in issues.
Please include a minimal reproduction.