Don't reset state when onError changes
#637
Conversation
🦋 Changeset detectedLatest commit: a704afb The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
|
Instead, let's try to store the actual error handler in a ref, update that ref in an effect, so then we can remove the handler as a dep of |
steveluscher
left a comment
steveluscher
left a comment
There was a problem hiding this comment.
"dependencies" amirite?
|
|
|
Yeah, sounds good. Will update and test. |
|
Wrote a failing test for you: 2c17fc6. |
|
Ooh! This is even better. When you fix the bug, you'll find that another test breaks, pointing to a separate real problem that this was masking. I don't want to steal it from you, but please do let me know if you want me to take this over. |
|
Yeah, turned out we need that state reset when the adapter changes, at minimum to wipe out the |
jordaaash
changed the title
adapter or onError changesonError changes
| @@ -1,4 +1,4 @@ | |||
| import type { WalletName } from '@solana/wallet-adapter-base'; | |||
| import { type WalletName } from '@solana/wallet-adapter-base'; | |||
There was a problem hiding this comment.
It was getting annoying to have two imports for the same dependencies everywhere. 😅
| }, [adapter, setWalletName]); | ||
| return ( | ||
| <WalletProviderBase | ||
| {...props} |
There was a problem hiding this comment.
y tho? I did this so that any additions to WalletProvider would get caught as TS errors if they were forgotten in Base.
There was a problem hiding this comment.
Yeah, I could see that, but it makes things a bit more complicated for the sake of something that almost never happens. When do we add props to the WalletProvider, and how likely are we to forget to use them if we need them?
| onAutoConnectRequest, | ||
| onConnectError, | ||
| onError, | ||
| onError = (error, adapter) => { |
There was a problem hiding this comment.
I think this will result in a new copy of this function on every render, which will thrash the effect.
There was a problem hiding this comment.
Ahh, you're probably right.
| handleErrorRef.current = (error, adapter) => { | ||
| if (!isUnloadingRef.current) { | ||
| if (onError) { | ||
| onError(error, adapter); |
There was a problem hiding this comment.
If you make the call to handleErrorRef.current here, all of the rest of the code gets way simpler.
There was a problem hiding this comment.
Good point, I wasn't happy with how I did this.
Fixes #636
Now that we're not wiping the state when the adapter changes, we'll need to check the logic in all the other places this should happen.