apibara / starknet-react

A collection of React providers and hooks for StarkNet
https://starknet-react.com
MIT License
368 stars 147 forks source link

Auto connect fails to connect in production builds #69

Closed tarrencev closed 1 year ago

tarrencev commented 2 years ago

Do you have any thoughts around supporting auto connect? It would support reconnecting when a connection has already been approved previously. Currently it seems most people do a hard connect, which prompts the user as soon as they land on a page, which I think is an anti-pattern.

fracek commented 2 years ago

Hey, yes I think we should have it. Argent added starknet.isPreauthorized() for this use-case. I like the wagmi api for this where you pass a autoConnect prop to the StarknetProvider at the root.

ltoussaint commented 2 years ago

Hello @fracek I have a problem with autoConnect with the following error

ConnectorNotFoundError: Connector not found

The problem is at the very beginning starknet is not yet injected into the website, so globalThis['starknet'] is undefined. It appears when I refresh the page (without forcing the cache to refresh) on a simple app which load very quickly. Do you have any idea how we could wait for starknet to be injected? (knowing that ArgentX may not be installed at all)

Here is a reproduction project https://codesandbox.io/s/agitated-sun-irox49 Here is the steps to reproduce the :

In the meantime I will force my app to wait before trying to autoConnect (maybe it's the final solution)

fracek commented 2 years ago

I cannot reproduce exactly your issue, but it's true that with a production build auto connect doesn't work. Will fix it.

tarrencev commented 2 years ago

Hmm a simple solution for the race condition that might be worth trying is to wrap the access to globalThis['starknet'] in a setTimeout(..., 0) so it is placed at the end on the event loop. I'm not sure what the order of operations looks like for making argent available on the global context

fracek commented 2 years ago

Even with setTimeout(..., 0) the issue persist. Adding a 100ms delay fixes this issue but it's kinda horrible.

tarrencev commented 2 years ago

Hmm yeah thats not ideal. I wonder if this is something argent needs to address? It doesn't look like an issue with metamask/wagmi. Perhaps there is something different about globalThis vs window and we should try accessing it on window instead?

Edit: It looks like globalThis['starknet'] is set here: https://github.com/argentlabs/argent-x/blob/7dc6a3d01442318004f380a0775892f286052dc1/packages/get-starknet/src/index.ts#L29

We could try adding a getStarknet call before checking the globalThis value here: https://github.com/auclantis/starknet-react/blob/main/packages/core/src/connectors/injected.ts#L23

fracek commented 2 years ago

That does solve the issue. It feels like the argent detection code could be improved, but I don't know exactly how.

ltoussaint commented 2 years ago

As answered here https://github.com/auclantis/starknet-react/pull/73#discussion_r849561400, this fix does not work for me.

I was thinking maybe I could test to create a second content_script from the extension. The only role will be to inject a function to retrieve, in an async way, the content of globalThis['starknet'] I just hope this content script will be fast enough to fix the issue.

What do you think?

tarrencev commented 2 years ago

@ltoussaint have you tried reproducing with all other extensions disabled (or in an incognito window with only argentx enabled)? just trying to understand the race condition better. i wonder if it depends on the number of extensions installed

ltoussaint commented 2 years ago

@tarrencev on chrome with only ArgentX as extension, it's still not working.

Thing is, it's a minimalistic example, most of the time dApps will be bigger, take more time to load and so let starknet inject its content into globalThis

fracek commented 2 years ago

Interesting, it looks like your issue is that it takes much longer to inject starknet in the window. This does look odd and I can't reproduce it.

Can you try add a setTimeout(tryAutoConnect(...), TIMEOUT) and check for which values of TIMEOUT it works?

ltoussaint commented 2 years ago

Ok so I found some interesting things.

In development mode, with the chrome developer console open, it start to fail with a timeout around 50ms. But with the chrome developer console closed, it never fails at 50ms.

At 0ms, it fails every time.

Now I built the project and run it locally With the developer console open, it fails almost every time With the developer console closed, it almost never fails

I will stop investigate on this for now as I have to focus on other stuff in the next few weeks, but I will come back and try to find a good solution to this.

fracek commented 2 years ago

It looks odd indeed. Let's ship the workaround that fixes the issue sometimes but let's keep this issue open.

Th0rgal commented 2 years ago

Hey, I seem to have the same issue when publishing my projects in production. You can reproduce it by cloning https://github.com/starknet-id/starknet.id and deploying it on vercel. It works fine in local. If you click on "claim your starknet.id" it should connect you automatically or ask you to chose a wallet if you have many, but if you refresh the page, it won't detect any wallet. image

fracek commented 1 year ago

I believe this issue has been fixed after switching this react-query.