Idea: Type-aware linting

[dai-shi]

Somewhere (there #16 (comment)), we discussed the limitation of linting because we can only analyze code statically.
If we could use type information, we could make linting better.
I stumbled upon eslint-plugin-functional which utilizes types.

We should be able to detect this:

const state = proxy({ obj: {}, num: 1 })

const Component = () => {
  const { obj, num } = useSnapshot(state)

  useEffect(() => {
    // ...
  }, [obj]) // this is not allowed

  useEffect(() => {
    // ...
  }, [num]) // but this is fine

  return "wow"
}

Related: pmndrs/valtio#633 (comment)

cc: @barelyhuman @sarimarton

[sarimarton]

Why shouldn't obj be allowed? I have fully legimitate use cases which can't be replaced with other ways, and there's a POC which fixes this. Disallowing this would conflict with the official react linter.

[barelyhuman]

I understand why we wouldn't want to monitor objects and only primitives instead since the renders for objects would be hard to manage but still, are we sure we wanna move forward with blocking that? as @sarimarton said, would actually conflict with the official react linter

[dai-shi]

Passing snap object to useMemo (and all others) deps and also React.memo will break tracking property usage, because React will skip touching properties on next render. I think I've created a simple breaking app before, but can't find it now.

So, we must destructure snap objects into primitive values in render function. I don't think it will conflict with react linter.
Note, I'm not saying I'm happy with it, but it can't be really helped theoretically. You can't guess how shocking it was to me when I found this limitation back in year 2019 or so. (For React.memo case, react-tracked provides custom memo to avoid this problem.)

Some old discussions: pmndrs/valtio#301 pmndrs/valtio#322

[sarimarton]

Objects are needed in equality checks.

useEffect(() => {
  fetch(api).then(response => { store.foo.bar = response })
}, [store.foo]) // <--- react linters mandates this

The tracker shouldn't rely on object/primitive distinction. Now the tracker triggers on the most specific keys' value change + content change. (By content I mean any descendant key.) This makes it non-deterministic on the individual usages. Content change should be taken out of the picture.

A few examples:

foo.bar

should trigger only when foo or foo.bar's equality with the previous value has changed. Now it triggers when foo.bar's value is changed or its content is changed.

foo.bar
foo.bar.baz

should trigger only when foos, foo.bar's or foo.bar.baz's equality with their previous values have changed. Now it triggers when foo.bar.baz's value or its content is changed, disregarding foo.bar's other content.

With this algorithm, a key reference's tracking is non-deterministic based on whether a subkey will be referenced later.

The latest version of useProxy fixes 3 problems, and I think those ones as well which you linked:

• ref stability on the top level (fixes caveat 1)
• ref stability on lower levels (useSnapshot parent object has new reference when a child key is assigned to an object valtio#633 (comment))
• behavioral difference on setting a primitive or object (useSnapshot parent object has new reference when a child key is assigned to an object valtio#633 (comment))

The only outstanding issue is the above design decision (which is maybe related to caveat 2) in the tracking logic, for that I'd need to write a new tracker :)

(Which is not impossible btw, my next step would be to subscribe to shallow changes on line 31 of utils.ts. The only problem is that there's no shallow subscribe API)

[dai-shi]

should trigger only when foos, foo.bar's or foo.bar.baz's equality with their previous values have changed.

how would you know if both foo.bar and foo.bar.baz are used, or only foo.bar.baz is used?

[sarimarton]

I shouldn't need know it. I should listen on foo, foo.bar and foo.bar.baz's change disregarding their subkeys. On every single get trap, I should register a shallow subscription on their value.

[dai-shi]

doesn't foo.bar change if foo.bar.other changes?

[sarimarton]

It wouldn't. The current subscribe() API makes sense outside of the react world. But in a render function, deep subscribe doesn't make sense. That's why we track usages, because we know exactly the realm of tracking (the render function itself).

[dai-shi]

I'm confused. Not sure if we are on the same page.

---

useEffect(() => {
  console.log('this should fire when foo.bar.other is changed');
  if (foo.bar.baz) {
     console.log('baz is truthy.');
  } else {
     console.log('baz is falsy.');
  }
}, [foo.bar])

[sarimarton]

Give me a shallow subscribe API, and I'll remove useSnapshot from under useProxy, and everything will be perfect 😉

[sarimarton]

I'm confused. Not sure if we are on the same page.

useEffect(() => {
  console.log('this should fire when foo.bar.other is changed');
  if (foo.bar.baz) {
     console.log('baz is truthy.');
  } else {
     console.log('baz is falsy.');
  }
}, [foo.bar])

No, that should only fire when foo.bar's value changed with a strict equality check. That's idiomatic react. Deep subscribe is the root of evil. In a tracking context, deep subscribe is bad.

[sarimarton]

const unsub = subscribe(proxy.foo.bar, callback, { shallow: true })

It only calls callback, when proxy.foo.bar gets reassigned to newVal, and newVal !== oldVal. It doesn't matter if it's an object or primitive value.

[dai-shi]

if you mean you don't use snapshot(), it will be concurrent unfriendly.

but, anyway, what's your shallow subscribe look like?

[dai-shi]

const unsub = subscribe(proxy.foo.bar, callback, { shallow: true })

For now, you can simulate it with this:

const unsub = subscribeKey(proxy.foo, 'bar', callback)

[sarimarton]

Cool, I'll give it a try!

[sarimarton]

if you mean you don't use snapshot(), it will be concurrent unfriendly.

Could you give me a hint why?

[dai-shi]

without snapshots, two components rendered from parent will produce different results. It's why "reading" useRef is prohibited in render, and why useSyncExternalStore requires getSnapshot.

If I understand correctly, what you are trying may work in legacy rendering, but not in concurrent rendering as we want. snapshot is the reason why valtio exists.

It's not hypothetical, but hard to reproduce. you can see my contrived demo in https://www.youtube.com/watch?v=oPfSC5bQPR8

[sarimarton]

That was a great video, @dai-shi, and it enlighted me about your mindset on the design of Valtio. Thanks!

A few observations:

• Although you keep mentioning that snapshots are the reason for Valtio's existence, Valtio has other unique values, namely its API simplicity built around simple objects - no HOCs/HOFs, no classes, no decorators. Basically a minimal subscription proxy API and a hook; that's how people perceive. Snapshots play little role in the appeal.
• Concurrent rendering's value is controversial for a while. Especially in Svelte and SolidJS communities it has been growingly articulated that concurrent rendering brings marginal value. (Think about Rich Harris's notable Svelte intro in which he mocks the React concurrent rendering demo by replicating the same demo in Svelte achieving the same speed, thanks to the precise change detection/propagation. SolidJS has the same merits.)
• Having said that, it is a controversial situation that such a marginal, although seemingly canonical characteristics or react has such a large impact of any proxy-based approach. Valtio's current useSnapshot is essentially non-compliant with dep arrays, an idiomatic element of everyday react development. It leads to pitfalls very quickly, and it's everything but not simple, as the headline says. It seems like a default non-concurrent-friendly but otherwise really dep-array-safe and simple API would bring a lot of value, whereas a concurrent-ready hook would be best put into an opt-in category, where the user is warned that their life won't be easy anymore without ref stability.
• But I understand a purist position as well that keeps concurrency support a first-class feature.
• In the upcoming days, I'll try to experiment a bit more to involve useSyncExternalStore, and only subscribe to keys, not trees.
• I still think that tree-subscription in usage tracking is a mistake, and that primitive/non-primitive distinction even in a linter doesn't solve anything.

[dai-shi]

I get your point and there might be something that I can re-consider, while I still believe the snapshot is crucial for valtio internals (but could be hidden from users.)

I'll challenge you from another aspect.
Even if we forget about concurrency support, hook-based API should be difficult and I can probably create a breaking scenario. As long as I remember, that's why mobx people gave up with providing hook-based API. Same applies probably for react-easy-state. So, it feels like something is missing in our discussion, but can't explain it clearly. I still think key-subscription doesn't work. If it does, it's a new library for me (as another motivation of mine is to use proxy-compare. it's not a technical reason. valtio is one of proxy-compare family.)

So, look forward to your experiment.

[dai-shi]

After discussing in pmndrs/valtio#635, I understand that what it means by conflicting with react linter, or I'd say conflicting with react best practice. And, that's exactly the point of having the rule in eslint-plutin-valtio.
I'll open a new issue.