Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v2] breaking: compatibility with memo #866

Merged
merged 1 commit into from
Mar 2, 2024
Merged

[v2] breaking: compatibility with memo #866

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 5 additions & 12 deletions src/react.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,9 @@ export function useSnapshot<T extends object>(
options?: Options,
): Snapshot<T> {
const notifyInSync = options?.sync
// per-hook affected, it's not ideal but memo compatible
Copy link

@faceyspacey faceyspacey Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not "ideal" because every used key -- perhaps stale, from previous enders -- will now be remembered?

Whereas before, affected was re-created every render, ensuring the absence of possibly stale/unused keys, and therefore less checking in isChanged.

Why does React compiler require this exactly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @faceyspacey !

Exactly. You understand it perfectly.

This is not only the issue with the compiler. It has been an issue with React.memo and useMemo.
In Valtio, we has been recommending to avoid using React.memo and useMemo.
In React-Tracked, we had provided custom memo to replace React.memo (AFAIR, it's you who suggested it.)

The real issue is, in the case of useMemo, it doesn't evaluate the function in every render.
Here's a simple repro:

const Component = () => {
  const snap = useSnapshot(...);
  const value = useMemo((() => snap.value, [snap]);
  // ...
};

Valtio v1 doesn't work well in this repro:
On the initial render, .value is touched.
But, on the second render triggered by a parent component (not by useSnapshot), all tracking information will be gone. Same can happen with React compiler.

Copy link

@faceyspacey faceyspacey Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's crazy that I was the one who suggested -- brings things full circle!

Truthfully I have a lot to share with you. I've been using Valtio now for 2 years, and React Tracked for the years before it. I'm almost done with something I've been working on for at least 6 years. I forked Valtio a year ago and started to add my own customizations. Just recently I did experiments to make it lazy -- I was the one who asked about this about 8 months ago from a company account, Skins App, for which I'm CTO. I've had success making the valtio "set" proxy lazy based on usage in the proxy-compare "get" proxy. But for now, I actually shifted to making it lazy based on an additional "get" proxy in the valtio "set" proxy. This has the benefit of being able to receive large nested data sets, which don't immediately become proxies. For example, say we receive and set this data:

proxy.users = [
  { name: 'Daishi Kato', nested: { deeper: { soOn: {} } }, ...etc },
  // etc
]

Here's a simplified version of my get trap in the valtio proxy:

 get(target, k, receiver) {
      let v = Reflect.get(target, k, receiver)

      if (!proxyStatesMap.has(v) && canProxy(v)) {
        v = createProxy(v, store, receiver)
        Reflect.set(target, k, v, receiver)
      }

      proxyStatesMap.get(v)?.listeners.add(notify)

      return v
}

Here's the no longer eager set trap:

set(target, k, v, receiver) {
      const prev = Reflect.get(target, k, receiver)
      
      const equal = Object.is(prev, v) || cache.has(v) && Object.is(prev, cache.get(v))
      if (equal) return true

      Reflect.set(target, k, v, receiver)
      notify()

      return true
}

So going back to the example, essentially only the initial users array becomes a proxy, but the nested data won't until you access it, which interestingly is what will happen if you try to set it deeply, eg: proxy.users[0].nested.deeper.soOn.foo = 'bar'.

So it functions identically to your original set proxy, but only accidentally as a by-product of reaching deep into the nested data set.

This allows you to have large and deeply nested objects that operate as your refs currently do. Note: I've removed the refs feature (and a few other things I don't need) to squeeze out a bit more performance. Essentially by using the get trap we achieve automatic refs for anything that you never change. Keep in mind I have my entire app in valtio -- we have a lot of data in there!

And by the way, it's working great! And it was working great before I even forked your repo. Valtio works very well with significant data sets. I'm basically over-optimizing to preempt any concerns for when I release these tools to a larger audience.

So this approach isn't as lazy as if I did it in the proxy-compare snapshot proxy, because proxies will still be setup for root objects when they are assigned, and because your app may set deeply nested things which snapshot proxies don't actually use. But I did make it work work in the proxy-compare snapshot proxy too, in case you were interested. My end goal is to revert back to that approach after I remove my own unrelated customizations in the valtio "get" proxy, which necessitate doing this work there anyway.

So all that said, I'd just like to confirm with you: the new memo handling is only to support passing objects to memoized children or an entire snap as in your above example, correct? This is as opposed to primitives.

I've been following the rule of just passing primitives to memoized components, and have had zero problems. But I'm thinking the potential problem is that "I'm too good at using valtio", whereas new users may pass the entire object. The tradeoff you made in your latest V2 changes is in having to educate users about this, versus it just working [in all cases] as you have now, correct?

If so, that's a tough decision, because the tradeoff is we lose some performance in this approach. I personally wouldn't want to see this in my app. I get that there's now new React Compiler benefits. Frankly that won't be a big deal for my underlying tools because essentially all event handlers are statically setup once, and then maintain references. I actually have it so every component is memoized automatically by a babel plugin! I have some very novel discoveries about how to build apps very differently. And one of the benefits is you rarely use tools like useCallback nor have to fiddle with references to optimize rendering. Because you aren't creating event handler functions in components, you get automatic perf + DX. I have a Pressable component which is memoized to receive events and a nested arg prop.

Therefore, given I don't worry about references and memoizations, React Compiler is probably more of a problem than a solution (since it is struggling to be perfect). From the perspective of Respond Framework managing references is considered "implementation details" (which we don't have to deal with because they are automatically handled!)

React Compiler may be incrementally better, but it's another thing from the react team which I don't use. I haven't used anything new since hooks. No transitions, no RSCs, no suspense. None of this. I've tried it all -- so I'm well versed in it -- but the way Respond Framework solves problems is completely different (and simplified!) and doesn't necessitate these things. From the perspective of Respond everything since hooks is "solutions to problems they have created."

Anyway, my last question relates to the memoization challenges -- and that's this line in isChanged:

if (changed === null) changed = true

I recently purchased and watched your video on the simplified version of Valtio, which was great by the way! And if I'm correct, you are returning true also as a way to ensure memo still works when there is no record of what was accessed?

And I'm guessing you no longer need to fallback to returning true with the v2 approach:

const affected = useMemo(() => new WeakMap<object, unknown>(), [])

correct?

I ask this because I'm optimizing for a more narrow use case, and I'd like to achieve the best possible perf. It seems in valtio v1 for apps that never pass snap objects as props to memoized components, you don't need this line either, correct?

If correct, we can remove it and avoid a few possible unnecessary renders.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#836
I think I might have misunderstood one thing.
If we can make set lazy only with in vanilla.ts, it can be something considerable. But, I'm not sure the performance benefit or tradeoffs.

So all that said, I'd just like to confirm with you: the new memo handling is only to support passing objects to memoized children or an entire snap as in your above example, correct? This is as opposed to primitives.

Yes. You can easily confirm it with small examples.

The tradeoff you made in your latest V2 changes is in having to educate users about this, versus it just working [in all cases] as you have now, correct?

That is correct, but the education is not the motivation of v2 changes. The motivation is to be compatible with upcoming React 19+.

If so, that's a tough decision, because the tradeoff is we lose some performance in this approach.

Yeah, that's correct. But, otherwise, we would simply lose users because Valtio v1 can't be used with React compiler. And, it's probably easy to recover the lost performance with the compiler.

And I'm guessing you no longer need to fallback to returning true with the v2 approach:

hm, tbh, I haven't thought about it.

I think we still require that. One thing is "proxy-compare" is used elsewhere too. And the change in valtio v2 doesn't ensure anything.

And if I'm correct, you are returning true also as a way to ensure memo still works when there is no record of what was accessed?

Another thing is that I think you misunderstood something.
While it's related to memo, the rationale is if snap.obj is accessed, but snap.obj.foo isn't, .obj is marked as "used". Likewise, if snap.bar isn't accessed, snap itself is marked as "used".
Hope it explains.

Copy link

@faceyspacey faceyspacey Jun 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the other use case besides supporting memoized components (in valtio v1) is supporting !!snab.obj in order to conditionally render something. I fully understand now why we need if (changed === null) changed = true.

Regarding React Compiler, it seems it's just the same issue as before regarding memoized components that you passed snap objects to. The only difference is now users might be unaware that their child component is going to be memoized, causing more potential for users accidentally losing reactivity and therefore frustration.

Is there something else about React Compiler that causes valtio to breakdown? Perhaps something more explicit that crashes the app?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!!snab.obj

Yeah, that's a great example. Noted.

The only difference is now users might be unaware

correct. not only child components, but also the component with useSnapshot.

Is there something else about React Compiler that causes valtio to breakdown?

I don't believe there's any. But, we don't test it throughly. I'm waiting for user feedback.

const affected = useMemo(() => new WeakMap<object, unknown>(), [])
const lastSnapshot = useRef<Snapshot<T>>()
const lastAffected = useRef<WeakMap<object, unknown>>()
let inRender = true
const currSnapshot = useSyncExternalStore(
useCallback(
Expand All @@ -128,11 +129,10 @@ export function useSnapshot<T extends object>(
if (
!inRender &&
lastSnapshot.current &&
lastAffected.current &&
!isChanged(
lastSnapshot.current,
nextSnapshot,
lastAffected.current,
affected,
new WeakMap(),
)
) {
Expand All @@ -147,20 +147,13 @@ export function useSnapshot<T extends object>(
() => snapshot(proxyObject),
)
inRender = false
const currAffected = new WeakMap()
useEffect(() => {
lastSnapshot.current = currSnapshot
lastAffected.current = currAffected
})
if (import.meta.env?.MODE !== 'production') {
// eslint-disable-next-line react-hooks/rules-of-hooks
useAffectedDebugValue(currSnapshot, currAffected)
useAffectedDebugValue(currSnapshot, affected)
}
const proxyCache = useMemo(() => new WeakMap(), []) // per-hook proxyCache
return createProxyToCompare(
currSnapshot,
currAffected,
proxyCache,
targetCache,
)
return createProxyToCompare(currSnapshot, affected, proxyCache, targetCache)
}
Loading