jotaijs / jotai-devtools

A powerful toolkit to enhance your development experience with Jotai
https://jotai.org/docs/tools/devtools
MIT License
124 stars 29 forks source link

useAtomsDevtools causes react warnings with latest version of jotai #158

Closed axel-havukangas-tt closed 1 month ago

axel-havukangas-tt commented 1 month ago

After upgrading to jotai version 2.9.0, which introduces the new store implementation, I'm getting react warnings for any components that uses the useAtom jotai hook.

Warning: Cannot update a component (AtomsDevtools) while rendering a different component (Counter). To locate the bad setState() call inside Counter, follow the stack trace as described in https://reactjs.org/link/setstate-in-render

The error seems to be caused by the use of useAtomsDevtools in our AtomsDevtools component, which apparently seems to trigger an update when atoms connect to the store.

No issues with jotai version 2.8.4

arjunvegda commented 1 month ago

Thanks for reporting! Could you please help me reproduce this?

It seems to work fine for me locally in our Storybook playground. I'm using Jotai@2.9.1 and the latest version of dev tools.

image
axel-havukangas-tt commented 1 month ago

Sure, created a reproduction branch at https://github.com/axel-havukangas-tt/jotai-devtools/tree/158-repro where I updated to jotai version 2.9.1.

Interestingly, in the jotai-devtools sandbox, the warning message doesn't appear for all atoms. But at least appears when pressing the "TOGGLE RESULT" button:

https://github.com/user-attachments/assets/14377e65-91a6-42e3-9213-4862f7ccfce4

Our app has a lot of async initialisations, so might be that the warning messages actually only comes in situations where an async operation updates an atom.

axel-havukangas-tt commented 1 month ago

Investigated the issue a bit more, and seems to be somehow related to https://github.com/jotaijs/jotai-devtools/blob/main/src/utils/useAtomsSnapshot.ts#L110 where the hooks state update is not deferred when the store subscription triggers a callback. Not sure what change has caused the warnings to trigger here though.

axel-havukangas-tt commented 1 month ago

Okay, after some more investigation it seems that the culprit is the different implementations of the composeWithDevTools for the v1 and v2 stores. The v1 store doesn't emit any events on atom get, while the v2 implementation calls subscribes on get events.

Not sure what the reasoning is behind the extended store subscription in the v2 implementation, but one possible solution would be to skip the atom read events in the useAtomsSnapshot hook store subscription.

axel-havukangas-tt commented 1 month ago

If I can get some confirmation that my investigation is sane, and this is a reasonable approach to fix the issue, then I could make a PR next week that would add some event filtering to the store subscriptions so that the atomsSnapshot isn't unnecessarily updated on atom reads (which obviously shouldn't affect the returned value).

arjunvegda commented 1 month ago

Thank you so much for your thorough investigation, @axel-havukangas-tt! I believe we can completely remove this line, as it seems unnecessary now.

Initially, I thought it was required for TimeTravel to sync the history for async atoms, but with async-get in place, it's no longer needed.

Would you like to open a PR?

axel-havukangas-tt commented 1 month ago

Yeah, if it seems like the whole get callback isn't needed then it is probably cleaner to just remove that from the v2 store composition implementation.

Created a PR at https://github.com/jotaijs/jotai-devtools/pull/160