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.

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

Cache key could refer previous value #712

Open
1 of 2 tasks
DevSDK opened this issue Apr 17, 2023 · 18 comments
Open
1 of 2 tasks

Cache key could refer previous value #712

DevSDK opened this issue Apr 17, 2023 · 18 comments

Comments

@DevSDK
Copy link
Contributor

DevSDK commented Apr 17, 2023

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.

  • I've already opened a discussion before opening this issue, or already discussed in other media.

Please include a minimal reproduction.

@dai-shi
Copy link
Member

dai-shi commented Apr 18, 2023

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: #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
Copy link
Member

dai-shi commented Apr 18, 2023

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
Copy link
Contributor Author

DevSDK commented Apr 18, 2023

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

@DevSDK
Copy link
Contributor Author

DevSDK commented Apr 18, 2023

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
Copy link
Member

dai-shi commented Apr 18, 2023

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
Copy link

Juyeong-Byeon commented May 1, 2023

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
Copy link
Member

dai-shi commented May 1, 2023

@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
Copy link
Contributor Author

DevSDK commented May 2, 2023

Sure. I've opened very draft PR #721 to add docs.
I'll fill the PR description soon

@dai-shi
Copy link
Member

dai-shi commented May 2, 2023

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
Copy link

Juyeong-Byeon commented May 17, 2023

I need some time to look into it.
I think it is better to merge #721 first and edit later.

@Juyeong-Byeon
Copy link

@dai-shi FYI: I found change that caused this issue
#712 is an issue that has occurred since version 1.8.0 caused by ec44557 or #599

@dai-shi
Copy link
Member

dai-shi commented Jun 9, 2023

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

Juyeong-Byeon added a commit to Juyeong-Byeon/valtio that referenced this issue Jun 10, 2023
@serdaryesilmurat
Copy link

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
Copy link
Member

dai-shi commented Jul 20, 2023

// 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
Copy link

Juyeong-Byeon commented Jul 28, 2023

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

@Juyeong-Byeon
Copy link

@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
Copy link

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
Copy link
Contributor

LiamMartens commented Nov 25, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants