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

useAtomsDevtools causes react warnings with latest version of jotai #158

Closed
axel-havukangas-tt opened this issue Aug 1, 2024 · 7 comments · Fixed by #160
Closed

useAtomsDevtools causes react warnings with latest version of jotai #158

axel-havukangas-tt opened this issue Aug 1, 2024 · 7 comments · Fixed by #160

Comments

@axel-havukangas-tt
Copy link
Contributor

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

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

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:

Screen.Recording.2024-08-02.at.7.56.13.mov

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

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

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

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

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

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 #160

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.

2 participants