-
Notifications
You must be signed in to change notification settings - Fork 7
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
Issue when no wallet is attached to the browser #110
Conversation
This is not the ideal scenario, but for now this seems to fix the problems related to builds. Ready for review @xavikh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments
}, | ||
connectors: [ | ||
walletConnect({ | ||
projectId: PUB_WALLET_CONNECT_PROJECT_ID, | ||
metadata, | ||
showQrModal: false, | ||
}), | ||
injected({ shimDisconnect: true }), | ||
// coinbaseWallet({ appName: metadata.name, appLogoUrl: metadata.icons[0] }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now coinbaseWallet only works without SSR. If we enable it, it fulls the console with errors.
We can enable it if we go to prod, but at least when developing it's quite annoying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't find a fix then we should consider rolling back enabling SSR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning/error appears in general in Next, doesn't matter if SSR in wagmi or not.
To be clear, it does work. It just shows an error in the console because the backend fails to run that piece of the code.
}, | ||
connectors: [ | ||
walletConnect({ | ||
projectId: PUB_WALLET_CONNECT_PROJECT_ID, | ||
metadata, | ||
showQrModal: false, | ||
}), | ||
injected({ shimDisconnect: true }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we want to use the injected one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, it comes by default with the Metamask id. If you add it, it shows metamask twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, nice catch then!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.