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

Better to just use Proxy state warning when using useSnapShot #26

Open
tavolafourcade opened this issue Jun 10, 2022 · 11 comments
Open

Comments

@tavolafourcade
Copy link

Hello,
I'm facing an issue with the latest version of eslint-plugin-valtio (0.4.4). The problem is that I'm using useSnapShot to re render the variable from the Store.
When I replace this implementation and call the variable directly from the Store the warning get fixed but the variable doesn't render anymore...

I tried changing the declaration of arrow function for the keyword function without success (#22).

const { isEditedMInformation } = useSnapshot(memberInformationStore)
vs
memberInformationStore.isEditedMInformation

I wasn't able to find any solution to my issue. Can somebody give me a guidance about it? Thanks!

@dai-shi
Copy link
Member

dai-shi commented Jun 11, 2022

@barelyhuman Can take a look?

@barelyhuman
Copy link
Collaborator

Yes sir! 🫡

@tavolafourcade

To get better context, you are using snapshots to add into the render tree correct?
and if you use proxy in the render tree it removes the warning?

or are you using snapshots in a hook or a function etc , I'll need a little more code snippet or repro to see what's actually causing the rule to fire

@barelyhuman
Copy link
Collaborator

barelyhuman commented Jun 11, 2022

@tavolafourcade

Assuming we are on the same page, both these tests seem to be passing and have no errors
so I'm going to need a better explanation of what you are trying to do to fix this. (check the image below)

Also, there was a bug in the middle which could cause an unwanted warning , which you can confirm by installing this
development version as mentioned in the comment here: #25 (comment)

If that doesn't fix, please try to give a small snippet

Screenshot 2022-06-11 at 7 40 10 PM

@tavolafourcade
Copy link
Author

Hello there! I will better explain my problem.
Previous to the update to the latest EsLint valitio version I was using useSnapShot to render the data in the following way:
memberInformation.ts
Captura de Pantalla 2022-06-13 a la(s) 08 33 25

Let's take isEditedMInformation as example.
When I go into my component:
import { memberInformationStore } from 'store/memberInformation'
const { isEditedMInformation } = useSnapshot(memberInformationStore)
And then I used it in my component:
<Button color='info' onClick={isEditedMInformation || isEditedPCP ? _handleOpenModal : _handleRedirect} variant='light'>Cancel</Button>

The EsLint warning made me change the way I was implementing this into the following way:
<Button color='info' onClick={memberInformationStore.isEditedMInformation || isEditedPCP ? _handleOpenModal : _handleRedirect} variant='light'>Cancel</Button>

So I used memberInformationStore.isEditedMInformationdirectly without using useSnapshot
The warning will dissapear but I also lose the functionality expected because isEditedMInformation will be stuck in the initial value and would not re render when needed.

Hope this explanation is more clear and you could suggest me the way to go, thanks in advance!

@barelyhuman
Copy link
Collaborator

Thanks, that should be enough to find the issue

@barelyhuman
Copy link
Collaborator

@tavolafourcade
Just to confirm,
can you check this repo and confirm that your specific case is actually giving a warning?
https://github.com/barelyhuman/valtio-eslint-repro-26

@barelyhuman
Copy link
Collaborator

@tavolafourcade
Hey, is it failing in the repo as expected ?

@haveanicedavid
Copy link

haveanicedavid commented Aug 9, 2022

Just installed the eslint plugin, and think I might be running into something similar (not sure, but didn’t want to open a new ticket).

My store:
image

comonent consuming:
image

error:
CleanShot 2022-08-09 at 15 50 53@2x

Changing line 60 in Navbar.tsx to reference the proxy itself (value={walletStore.activeChain?.id}) fixes the error, but then the UI doesn’t update correctly.

@barelyhuman Am I doing something wrong here?

@barelyhuman
Copy link
Collaborator

barelyhuman commented Aug 9, 2022

I think this is related to #32

You might wanna try the PR specific build as mentioned here: #33 (comment)

Let me know if that fixes it for you.

Also , simplest way to use valtio

  • snapshot for reads
  • proxy for writes

if a useCallback or useEffect is reading the snapshot just make sure to add it in the dependency array so you don’t have undefined behaviour

eg:

function Comp(){
  const snap = useSnapshot(prxyState)

  return <>{snap.count}</>
}
func Comp(){
 const snap = useSnapshot(prxyState)

  useEffect(()=>{
   console.log(snap);
},[snap])
}

but if you are writing to the snapshot, it won't work as intended and you should write to the prxyState directly

@haveanicedavid
Copy link

except if being used in a dep tree, eg, a useCallback or useEffect where you can execute the internal callback of useEffect or useCallback as soon as the snapshot changes.

This was my current understanding - I’ve built a decently large project with Valtio before and had no issue using in ways like this. Just installed the plugin and found the error confusing.

Looks like removing the export from the function declaration fixed it. IE

export const MyFunc = () => ()

throws the error, but

const MyFunc = () => ()
export { MyFunc }

Doesn’t.

if I change from an arrow function to a named function:

export function MyFunc() {}

it also fixes the error.

Thanks for the quick response! Glad to know i’m not using valtio incorrectly 😄

@barelyhuman
Copy link
Collaborator

yeah, so the next release should handle this, till then you can use the build from the PR if it’s blocking any CI/ git hooks.

A previous change we made to make sure the snapshot isn’t being written to inside hooks is what’s caused this issue.

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

No branches or pull requests

4 participants