False positive for "Better to just use Proxy state" without separate assignment

[kirbysayshi]

Hello, thanks for making this plugin, it's been helpful while learning valtio!

I believe I've found a bug 😅

In the following example, the goal is to get the useExampleX hook's useEffect callbacks to execute when one of the watched properties changes, and without a parent component passing in a new version of state. An additional goal is that only when a1's properties change would the hook rerender.

import { useEffect } from 'react';
import { proxy, useSnapshot } from 'valtio';

const state = proxy({
  a0: { b: { c: { d: 'hello', e: 'world' } } },
  a1: { b: { c: 'a1c' } },
});

// executes ok, lint notok
function useExample0(s: typeof state) {
  const snap = useSnapshot(s.a1);

  useEffect(() => {
    if (snap.b.c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [snap.b]);
}

// executes notok, lint ok: this is what blindly following the linter
// recommendation would produce!
function useExample1(s: typeof state) {
  useEffect(() => {
    if (s.a1.b.c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [s.a1.b.c]);
}

// executes ok, lint notok
function useExample2(s: typeof state) {
  const {b: {c} } = useSnapshot(s.a1);

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

// executes ok, lint ok (but renders for a0 changes as well)
function useExample3(s: typeof state) {
  const snap = useSnapshot(s);
  const {c} = snap.a1.b;

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

// executes ok, lint ok
function useExample4(s: typeof state) {
  const snap = useSnapshot(s.a1);
  const {c} = snap.b;

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

// executes ok, lint ok
function useExample5(s: typeof state) {
  const snap = useSnapshot(s.a1).b;
  const {c} = snap;

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

// executes ok, lint ok
function useExample6(s: typeof state) {
  const c = useSnapshot(s.a1).b.c;

  useEffect(() => {
    if (c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [c]);
}

It's unclear exactly what is happening. The bug seems to be triggered by destructuring directly from useSnapshot or directly referencing the snapshot in the useEffect. Or is it that assignment of the snapshot defeats the lint rule?

A worrying aspect is that if a developer blindly followed the recommendation from the linter, it would produce code that does not behave as expected. It's somewhat obvious in these contrived examples (useExample1), but is less obvious in a larger codebase with multiple layers of hooks.

A workaround for now appears to be:

• useSnapshot(state.a1) on the parent you need
• destructure or assign child properties in a separate statement

[dai-shi]

thanks for reporting! so, for lint, it recommends safe practices, so "executes ok and lint notok" isn't a big issue by design (but it would be nice to fix if it's one of the safe practices.)

The first one is not a safe practice. It might not work in some cases.

The second one 👇 seems like a real issue for eslint-plugin-valtio.

// executes notok, lint ok: this is what blindly following the linter
// recommendation would produce!

The third one seem a safe practice, it would be nice if lint can pass it.

The forth one 👇 sounds weird. a0 changes shouldn't render. (but it's not the issue of the linter. would you like to open an issue in the main repo with a reproduction?)

// executes ok, lint ok (but renders for a0 changes as well)

[dai-shi]

@barelyhuman would you be interested in digging this??

[kirbysayshi]

so, for lint, it recommends safe practices, so "executes ok and lint notok" isn't a big issue by design (but it would be nice to fix if it's one of the safe practices.)

@dai-shi Makes sense, but I tend to treat even lint warnings as something to avoid, otherwise I prefer to disable the rule 😆 . Linter noise can condition devs to ignore the warnings completely if they are not high-value.

The first one is not a safe practice. It might not work in some cases.

🤭 Can you help me understand why?

Is that because the dependency array of useEffect did not contain a reference to c? I think that was a typo on my part. Here is a fixed version. Is this still not safe? (Note: lint rule still does not pass)

// executes ok, lint notok
function useExample0(s: typeof state) {
  const snap = useSnapshot(s.a1);

  useEffect(() => {
    if (snap.b.c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [snap.b.c]);
}

The forth one 👇 sounds weird.

I think this is because I was ambiguous and also guessing! I didn't mean the useEffect would execute, but that the hook would re-execute (render) because how would useSnapshot() know that only some of the properties in the snapshot were accessed? Unless it tracks property usage scoped to the hook!? 🔮

[dai-shi]

Is that because the dependency array of useEffect did not contain a reference to c? I think that was a typo on my part. Here is a fixed version. Is this still not safe? (Note: lint rule still does not pass)

// executes ok, lint notok
function useExample0(s: typeof state) {
  const snap = useSnapshot(s.a1);

  useEffect(() => {
    if (snap.b.c === 'a1c') {
      state.a1.b.c = 'example';
    }
  }, [snap.b.c]);
}

Yes, this ☝️ is safe because deps array only has primitive values. What's not safe is passing snap objects in deps, because useEffect may conditionally access its properties. The linter can't detect this pattern because there's no way for it to know if snap.b.c is a string or an object.

The forth one 👇 sounds weird.

I think this is because I was ambiguous and also guessing! I didn't mean the useEffect would execute, but that the hook would re-execute (render) because how would useSnapshot() know that only some of the properties in the snapshot were accessed? Unless it tracks property usage scoped to the hook!? 🔮

ah, okay. yeah, it tracks property usage, so changing a0 shouldn't trigger re-render.

[barelyhuman]

@barelyhuman would you be interested in digging this??

Sure, I'll pick this up

[barelyhuman]

So, to confirm

useExample2 has an issue that needs debugging useExample3 is something the lint should consider as a valid case

anything else that needs to be added as a test case ?

[dai-shi]

@barelyhuman Example1 is something we need to fix. Example2 is something nice to fix.

[dai-shi]

It turns out that it's extremely difficult to catch edge cases, because a) we can't know if a property of an object is a primitive value or another object, and b) we can't know if a prop in a component or an argument in a hook is a proxy object or a snap object.

[saggiyogesh]

@dai-shi when this fix is being released? Yesterday only removed this plugin, due to this issue.

[dai-shi]

@Aslemammad will be working on something before releasing it. (edit: it's released.)

https://ci.codesandbox.io/status/pmndrs/eslint-plugin-valtio/pr/17/builds/221434 Can you try the codesandbox ci build? Fine "Local Install Instructions" there ☝️ . I'm not sure if that solves your case, though.

[kirbysayshi]

EDIT: apologies for this disjointed reply, I hadn't refreshed the page before responding. Thank you fixing the issues that could be fixed!

What's not safe is passing snap objects in deps, because useEffect may conditionally access its properties.

Are snap objects not comparable between component renders? I understand why the lint rule would be confused, but not why the usage of a snap in a useEffect dependency array would be unsafe.

Or is it that valtio does not track property usages that resolve to objects, only primitive values?

yeah, it tracks property usage, so changing a0 shouldn't trigger re-render.

WOW I didn't know this. Pretty incredible!

So just to be clear, two components that useSnapshot at different levels:

const snap = useSnapshot(state.nested.property)
snap.something;

and

const snap = useSnapshot(state)
snap.nested.property.something;

... will both rerender the same number of times?

[dai-shi]

What's not safe is passing snap objects in deps, because useEffect may conditionally access its properties.

Are snap objects not comparable between component renders? I understand why the lint rule would be confused, but not why the usage of a snap in a useEffect dependency array would be unsafe.

No, it's because React will access properties conditionally. Let's say, in the first render, it access snap.p1 and snap.p2, both .p1 and .p2 are marked as used. Then, .p1 value is changed and it re-renders. In the second render, if it doesn't access snap.p1, but just snap.p2, only .p2 is marked as used. Afterward, .p1 value change doesn't trigger re-renders.

Or is it that valtio does not track property usages that resolve to objects, only primitive values?

Objects work fine. It's just "leaf" usage is tracked.

yeah, it tracks property usage, so changing a0 shouldn't trigger re-render.

WOW I didn't know this. Pretty incredible!

So just to be clear, two components that useSnapshot at different levels:

const snap = useSnapshot(state.nested.property)
snap.something;

and

const snap = useSnapshot(state)
snap.nested.property.something;

... will both rerender the same number of times?

In terms of the number of re-renders, yes. The former is preferable, because the subscription is more scoped (narrower).

[kirbysayshi]

Ok, all makes sense. Thank you for the explanation!