reown-com / appkit

The full stack toolkit to build onchain app UX
https://reown.com/appkit
Apache License 2.0
4.9k stars 1.39k forks source link

[bug] Conflict between multiple @walletconnect/ethereum-provider instances when pre-building for cdn #905

Closed hlopes-ledger closed 1 year ago

hlopes-ledger commented 1 year ago

Link to minimal reproducible example

https://63f7a54331abfc008b46e5ca--ledger-w3o-vite-demo.netlify.app/

Summary

The linked demo is a build of the vite example app from the web3-onboard repo.

Both the Ledger connector (under development) and the WalletConnect connector use the ethereum-provider package. If you connect with one and then try to connect with the other while the connection is active, they will both use the same connection. But if you open the modal for one and close it without connecting, or disconnect before trying to connect with the other, the second one will fail with the error below

Uncaught (in promise) DOMException: Failed to execute 'define' on 'CustomElementRegistry': the name "w3m-box-button" has already been used with this registry

... since it will try to create a new component with the same name. Might need to check before creating it.

List of related npm package versions

"@walletconnect/ethereum-provider": "2.4.3"

xzilja commented 1 year ago

Thank you for reporting this, I was able to replicate the issue 👍 will try to isolate concrete error and see if we can fix it on our end, but it would also be a good idea to open similar issue up with web3-onboard team, could be the case where we might not be able to fix this, thus they'll likely need to destroy / recreate instances of ethereum-provider when switching between different connection methods.

hlopes-ledger commented 1 year ago

yes, we're currently updating our web3-onboard Ledger connector for WalletConnect v2 and I'll be mentioning this

hlopes-ledger commented 1 year ago

We don't even use the modal on our side and set showQrModal: false on EthereumProvider.init, so it might be a matter of not even instantiating it in ethereum-provider in that case.

muratangin187 commented 1 year ago

I am also facing the same issue. In my setup, I have both web3modal and https://github.com/xmtp/xmtp-js I guess the same conflict happens in this case too. Did you find any workaround?

hlopes-ledger commented 1 year ago

We're now implementing WC v2 support in wagmi, and as it loads all the connectors at app startup instead of only when you press a connector button like on web3-onboard, we get an error on the initial page load and only the first instantiated connector will work.

Unhandled Promise Rejection: NotSupportedError: Cannot define multiple custom elements with the same tag name

demo here: https://640a13fb3e6f7d0456b70c50--ledger-next-wagmi-demo.netlify.app

hlopes-ledger commented 1 year ago

I find it strange that if I only instantiate a Ledger connector that initialises the ethereum-provider with showQrCode: false, and so @web3modal/standalone is not imported, if I do const web3m = window.customElements.get('w3m-box-button') in the console in my app I still get a class instance... not sure where the element is being created if web3modal is not supposed to have been imported

xzilja commented 1 year ago

Which version of @walletconnect/ethereum-provider is ledger's connector using?

hlopes-ledger commented 1 year ago

I'm testing with the latest v2 branch, locally linked, so 2.4.10 currently

xzilja commented 1 year ago

Hmm, that's super weird, it shouldn't even import the package, let alone initialise it as per https://github.com/WalletConnect/walletconnect-monorepo/blob/v2.0/providers/ethereum-provider/src/EthereumProvider.ts#L465-L472

Do you mind sharing your repo / branch for me to investigate if possible? With some instructions on how to run it locally.

xzilja commented 1 year ago

@hlopes-ledger Just created https://github.com/WalletConnect/walletconnect-monorepo/pull/2105/files

I think this might help with modal becoming a peerDependency, will update this issue once we publish

hlopes-ledger commented 1 year ago

Thanks, looks like that might solve the issue on our side. Are you able to build it? Just checked it out but get errors since you still have this import at the top

import type { Web3Modal } from "@web3modal/standalone";

xzilja commented 1 year ago

You need to add it as a devDependency if you are building from source (not an issue with published dist code, since types are stripped away)

hlopes-ledger commented 1 year ago

It seems this change does not affect the way we are doing things.

We are serving our Connect Kit library from a CDN and bundling the ethereum-provider libs within, using this method: https://github.com/WalletConnect/walletconnect-monorepo/issues/341#issuecomment-1079579976.

From the rollup config on ethereum-provider I see that peerDependencies are being bundled on the UMD file, which we import. I've tried removing peerDependencies on that line but they still get included in the bundle when building ethereum-provider.

Do you mind sharing your repo / branch for me to investigate if possible? With some instructions on how to run it locally.

I'll document this.

hlopes-ledger commented 1 year ago

sorry that this is so involved, but here are the steps to reproduce the issue locally; let me know if something is not clear

I've also local liked ethereum-provider and web3modal, debugged the flow and confirmed that the web3modal import does not happen, so still don't understand why the conflict occurs.

hlopes-ledger commented 1 year ago

An update on this.

Our problem is that we're using the bundled UMD version of ethereum-provider, and in that case rollup converts all dynamic imports into inline ones, so it always bundles and instantiates web3modal. That means that the w3m-box-button component will always be registered even though we don't make use of it, and it will conflict with other instancies that are imported.

We've found a solution on our side, although not the most maintainable one. We copied the EthereumProvider class and the needed types from WalletConnect to our Connect Kit library and removed the references to web3modal. So now we bundle our own and depend on the universal-provider and @walletconnect/utils.

xzilja commented 1 year ago

I see. We made ample changes around this:

  1. Decoupled and made modal a peerDependency of ethereum-provider
  2. Relaxed hard version pins for the packages within our codebase and partner ones (wagmi)
  3. Set default to not show qrcode modal within ethereum-provider so dynamic import is never hit in nodejs env

I'm not sure if we can do much more here, I think my only suggestion would be moving to esm builds that respect modern features that we utilise, but also understand that this might not be an option in some scenarios.

I'll close this issue, but feel free to comment and @ me if you find some solution that we should review.