pmndrs / eslint-plugin-valtio

An eslint plugin for better valtio experience
MIT License
78 stars 6 forks source link

I think this should be valid, but the rule `state-snapshot-rule` is violated #36

Closed xiechao06 closed 1 year ago

xiechao06 commented 1 year ago

Environment

Description

It is common to memoize a value that depends upon some snapshots and other properties. but when using snapshot in useMemo, eslint reports **Better to just use proxy state", just like below:

image

Or maybe I am using an anti-pattern to valtio? the problemic line is at https://github.com/xiechao06/valtio-groups/blob/5001116213ed2a08a5ed4a8fc447e29cf7c5e6f1/src/App.tsx#L33

Best regards!

dai-shi commented 1 year ago

Using snapshot objects for useMemo/useCallback/useEffect dependencies is a little tricky and can break dependency tracking (because, it can skip touching some properties on second render). It's better to avoid it. So, the rule detecting it is correct. The warning message could be improved though.

There should be several approaches to work around it, but in your specific case, useMemo seems unnecessary. I would just avoid useMemo in this case.

xiechao06 commented 1 year ago

because, it can skip touching some properties on second render

May you please elaborate on this?

Performance is critical in my current project (not this demo), so we need to memorize some expensive calculations. If using snapshot objects for useMemo/useCallback/useEffect is error-prone, what is the correct way to do it, using "subscribe"? like the following example:

image
dai-shi commented 1 year ago

May you please elaborate on this?

Here's a contrived example to show an error: https://codesandbox.io/s/angry-worker-97b1gk?file=/src/App.js

so we need to memorize some expensive calculations.

subscribe would work. I would use derive: https://codesandbox.io/s/silly-cookies-dq0ek9?file=/src/App.tsx

xiechao06 commented 1 year ago

May you please elaborate on this?

Here's a contrived example to show an error: codesandbox.io/s/angry-worker-97b1gk?file=/src/App.js

so we need to memorize some expensive calculations.

subscribe would work. I would use derive: codesandbox.io/s/silly-cookies-dq0ek9?file=/src/App.tsx

Actually, the first example really confuses me, hope I could dig out the reason in the future. Thanks for your patient reply!

xiechao06 commented 1 year ago

May you please elaborate on this?

Here's a contrived example to show an error: codesandbox.io/s/angry-worker-97b1gk?file=/src/App.js

so we need to memorize some expensive calculations.

subscribe would work. I would use derive: codesandbox.io/s/silly-cookies-dq0ek9?file=/src/App.tsx

I've found another problem, using "derive" in useMemo will make eslint reports error, I guess it's because "groupItems" is used by "useSnapshot".

image

if I rename "groupItems", everything works fine:

image
dai-shi commented 1 year ago

Do you think it's the same issue reported in https://github.com/pmndrs/eslint-plugin-valtio/issues/36#issue-1489736576? Otherwise, can you open a new issue?