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

Use useEffect to avoid unsafe side effects for useAccount callbacks #1679

Merged
merged 6 commits into from
Feb 2, 2023

Conversation

aj-may
Copy link
Contributor

@aj-may aj-may commented Jan 17, 2023

Description

When using the onConnect and onDisconnect callbacks in an app with react strict mode on, the current implementation ends up causing the app state to become unstable. This has something to do with the double rendering of components when a react app, in strict mode, is running the dev server.

Additional Information

Your ENS/address:
ajmay.eth

Related Issues

I believe these issues are related to this fix:

As these issues usually occur after an onConnect or onDisconnect callback is called and when logging the state, you can see that it reverts on the second render.

@changeset-bot
Copy link

changeset-bot bot commented Jan 17, 2023

🦋 Changeset detected

Latest commit: 7e16062

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wagmi Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jan 17, 2023

@aj-may is attempting to deploy a commit to the wagmi Team on Vercel.

A member of the Team first needs to authorize it.

@aj-may
Copy link
Contributor Author

aj-may commented Jan 17, 2023

@jxom @tmm would be happy to get your thoughts on this one. I got the tests to run locally. I'm getting 3 errors but none related to this functionality. Also, tested in the playground and all seems to be fine.

@bounds
Copy link

bounds commented Jan 18, 2023

Very excited to not need to remove strict mode from projects where rainbow kit is used

@tmm
Copy link
Member

tmm commented Jan 19, 2023

Appreciate the patience! Will have some time to take a look soon.

@tmm
Copy link
Member

tmm commented Jan 20, 2023

@aj-may spent some time looking into this and it doesn't seem like anything is wrong here. The dev playground has used strict mode since April 2022. Can you describe some more what you mean by "app state to become unstable"? I'm seeing onConnect and onDisconnect callbacks fire appropriately in the dev playground and test suite.

(Side note: The issues you linked for RainbowKit are definitely issues, but quickly poked around there and they seem unrelated to wagmi.)

@aj-may
Copy link
Contributor Author

aj-may commented Jan 20, 2023

@tmm Ahh, I don't think this bug will rear its head unless there is a component that is using these callbacks to modify its own state. in that case the dependent components state becomes unstable, or reverts to its previous state on the second render under react strict mode.

Let me see if I can make a quick branch to add something to the playground that can replicate the bug.

@aj-may
Copy link
Contributor Author

aj-may commented Jan 20, 2023

@tmm I was able to add an example to reproduce here: https://github.com/aj-may/wagmi/tree/side-effect-playground

@aj-may
Copy link
Contributor Author

aj-may commented Jan 30, 2023

@tmm Have you had a chance to pull down the branch I created that reproduces the issue? Would love if we can revisit this and potentially merge this in.

@tmm tmm requested a review from jxom January 30, 2023 17:58
@vercel
Copy link

vercel bot commented Jan 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
wagmi ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 30, 2023 at 6:07PM (UTC)

@aj-may
Copy link
Contributor Author

aj-may commented Jan 30, 2023

@tmm The changes you pushed cause the connect and disconnect callbacks to be called twice in dev mode with react strict mode on and still shows some state fluctuations over the multiple renders in the console.

Screenshot 2023-01-30 at 13 34 30

Screenshot 2023-01-30 at 13 34 59

@aj-may
Copy link
Contributor Author

aj-may commented Jan 30, 2023

Prior to that the console looked like this:
Screenshot 2023-01-30 at 13 36 02
Screenshot 2023-01-30 at 13 36 23

These logs are from my test branch here: https://github.com/aj-may/wagmi/tree/side-effect-playground

@tmm tmm removed the request for review from jxom January 30, 2023 21:52
@tmm
Copy link
Member

tmm commented Feb 1, 2023

@aj-may thanks for the feedback! made some updates. try it out and lmk what you think.

@jxom jxom self-requested a review February 1, 2023 02:44
Copy link
Member

@jxom jxom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

@aj-may
Copy link
Contributor Author

aj-may commented Feb 1, 2023

👍 I'll take this for a spin in a couple hours

@aj-may
Copy link
Contributor Author

aj-may commented Feb 2, 2023

Looking good!

image

@tmm tmm merged commit 3cef111 into wevm:main Feb 2, 2023
@github-actions github-actions bot mentioned this pull request Feb 2, 2023
llllvvuu added a commit to llllvvuu/wagmi that referenced this pull request Jul 1, 2023
fixes wevm#1998

fix is mostly re-doing wevm@3cef111 in a more simple way. Have confirmed that:
* The problem fixed in wevm#1679 is still fixed
* The problem in wevm#1998 is fixed (tested example repo working after change)

Also fixes stale closure issue with setting `const config = getConfig()`
inside the callback (this will cause the hook to be subscribed to the
wrong config if the config changes e.g. lazy-loaded config)
jxom added a commit that referenced this pull request Jul 3, 2023
…2664)

* fix: `useAccount` should not unsubscribe from zustand when re-render

fixes #1998

fix is mostly re-doing 3cef111 in a more simple way. Have confirmed that:
* The problem fixed in #1679 is still fixed
* The problem in #1998 is fixed (tested example repo working after change)

Also fixes stale closure issue with setting `const config = getConfig()`
inside the callback (this will cause the hook to be subscribed to the
wrong config if the config changes e.g. lazy-loaded config)

* refactor

---------

Co-authored-by: moxey.eth <jakemoxey@gmail.com>
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 this pull request may close these issues.

4 participants