pmndrs / valtio

💊 Valtio makes proxy-state simple for React and Vanilla
http://valtio.pmnd.rs
MIT License
8.7k stars 244 forks source link

Cache key could refer previous value #712

Closed DevSDK closed 18 hours ago

DevSDK commented 1 year ago

Summary

The cache key is referred to the previous value when the shape of references in the cache key is the same.

Link to reproduction

https://jsfiddle.net/z41pyotn/

Minimum reproduction cases represent proxy in array proxy.

Here's my debugging context that may help:

It seems to be no cache invalidation when the 'remove' operation for the array.

In minimum repro. case, at line 40, the proxy and value are cached that key as a proxy of 'a'.

When handle() function is called, the array will empty. After that, the values are updated (not proxy reference.) and pushed again to a.

The result of the 'a' still refers to the same cache that has been made by 40 lines. Because it refers to the same shape of references that is used as the cache key.

Check List

Please do not ask questions in issues.

Please include a minimal reproduction.

dai-shi commented 1 year ago

Thanks for reporting. If I understand it correctly, this is a limitation with the current design choice. It would be nice if we can unlimit it in v2: https://github.com/pmndrs/valtio/discussions/703

(edit) On second thought, I'm not sure if it's a limitation only in v1. Even in v2, this limitation may persist.

dai-shi commented 1 year ago

Can this be a workaround for now?

  window.handle = function handle() {
    const temp = a[0]
-   a.splice(0, 1)
    temp.nested.nested2.a = 'Banana'
+   a.splice(0, 1)
    a.push(temp)
    console.log(Object.is(temp, a[0]))
  }
DevSDK commented 1 year ago

Sure. It worked correctly. That's the way we avoid this issue for now on our product.

DevSDK commented 1 year ago

It would be nice to resolve this issue as it can cause confusion and make debugging more challenging. Currently, the Redux DevTools are not notified when the state changes, which can lead to misunderstandings.

Additionally, relying on console.log for debugging further complicates the situation because proxies are updated correctly, but the snapshot is not.

dai-shi commented 1 year ago

I don't think it's resolvable without breaking something else. Anyone can give a try if interested.

Currently, the Redux DevTools are not notified when the state changes, which can lead to misunderstandings.

If you detach an object, there's no way to notify. So, people could know that they are doing something wrong. They should change the object before detaching.

I think we should describe this limitation in the docs: https://github.com/pmndrs/valtio/blob/main/docs/how-tos/some-gotchas.mdx Would you like to open a PR?

Juyeong-Byeon commented 1 year ago

I think it is about a version of removed item. Can't we update proxy version of item when item is removed from array as a simple solution?

dai-shi commented 1 year ago

@Juyeong-Byeon Don't we update proxy version when removing? Feel free to open a draft PR.

@DevSDK It would be nice if you can add a https://codesandbox.io/s/react-new reproduction in the description of this issue.

DevSDK commented 1 year ago

Sure. I've opened very draft PR https://github.com/pmndrs/valtio/pull/721 to add docs. I'll fill the PR description soon

dai-shi commented 1 year ago

oops, my request was to just add a codesandbox link here from your jsfiddle one.

hoping @Juyeong-Byeon has an idea.

Otherwise, let's add it to docs #721.

Juyeong-Byeon commented 1 year ago

I need some time to look into it. I think it is better to merge https://github.com/pmndrs/valtio/pull/721 first and edit later.

Juyeong-Byeon commented 1 year ago

@dai-shi FYI: I found change that caused this issue https://github.com/pmndrs/valtio/issues/712 is an issue that has occurred since version 1.8.0 caused by https://github.com/pmndrs/valtio/commit/ec445576274f6b1c53c86099846592b5550e24fd or https://github.com/pmndrs/valtio/pull/599

dai-shi commented 1 year ago

@Juyeong-Byeon Thanks for your investigation! ec44557 seems changing the cache key from receiver to target. Is it causing the issue?

serdaryesilmurat commented 1 year ago

This seems to be a relatively new issue, but since there is no update over a month, let me lit the fire again 😅 Will there be a fix anytime soon?

We also face the same issue in our project. We have a list of objects that are rendered in an antd Table component. Whenever we update the list in the store (replace an item in the array), Table component is not re-rendered.

// Not working
storeObj.nested.list.splice(index, 1, newObj); // Insert new item directly to the original array in store

// Workaround
storeObj.nested.list.splice(index, 1); // First delete
storeObj.nested.list.splice(index, 0, newObj); // Then insert the new item
dai-shi commented 1 year ago
// Not working
storeObj.nested.list.splice(index, 1, newObj); // Insert new item directly to the original array in store

I wonder if it's the same issue as this. Maybe @Juyeong-Byeon can confirm.

I thought and still think the original issue might not be fixable. But, splice(index, 1, newObj) should be fixed.

Juyeong-Byeon commented 12 months ago

I'll take a look on the weekend. In a day or three.

Juyeong-Byeon commented 12 months ago

@serdaryesilmurat Can you describe more about your situation or share reproducible code? 🙂 I tried to reproduce the error, based on your description about the case, but I couldn't reproduce it.

I tried bellow: https://codesandbox.io/s/valtio-examples-forked-tml257?file=/src/App.tsx

serdaryesilmurat commented 11 months ago

Sorry for the late response. We are using the list in the store as the datasource of an antd table. Not sure but the error seems to be in antd.

In the sample code below, you can reproduce the error by pressing "update first" after adding new rows. "update last" always updates the table, but "update first" does not.

https://codesandbox.io/s/valtio-examples-forked-hts2t9

LiamMartens commented 8 months ago

@serdaryesilmurat it looks like the problem you're having is specific to antd's Table component. I did some digging and it works when rendering a raw table; it looks like this is because the way the ant table accesses the properties doesn't register their usage in the proxy so it doesn't end up tracking the keys and thus never sees a change.

I would have to dig into the Ant source code but it's likely they are making copies of the objects somehow.

One way you could resolve it is to manually access those keys beforehand ie: https://codesandbox.io/p/sandbox/valtio-examples-forked-y2cmck?file=%2Fsrc%2FApp.tsx

I guess for any similar bugs the issue is likely to be key tracking especially when using it with third party libraries since there's no guarantee they will use the proxy objects as-is or whether they will be cloning them for their own usage which will always break the proxy logic.