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

onConnected not fired on page load #176

Closed
ekzyis opened this issue Dec 28, 2023 · 5 comments · Fixed by #185
Closed

onConnected not fired on page load #176

ekzyis opened this issue Dec 28, 2023 · 5 comments · Fixed by #185
Assignees

Comments

@ekzyis
Copy link

ekzyis commented Dec 28, 2023

Afaict, I used onConnected as described in the docs (inside useEffect) but it does not fire on page load.

The docs mention this usage:

useEffect(() => {
  // init bitcoin connect to provide webln
  const {onConnected} = await import('@getalby/bitcoin-connect-react').then(
    (mod) => mod.onConnected
  );
  const unsub = onConnected((provider) => {
    window.webln = provider;
  });

  return () => {
    unsub();
  };
}, []);

See my code in stackernews/stacker.news#715

Here is a showcase video:

2023-12-28.23-42-06.mp4
@rolznz
Copy link
Collaborator

rolznz commented Dec 29, 2023

@ekzyis thanks for bringing this up - the example does not consider the case where the user is already connected on the site, and where Bitcoin Connect has already connected before the React effect runs.

I think we need something like this:

useEffect(() => {
  // init bitcoin connect to provide webln
  const [isConnected, onConnected, requestProvider] = await import('@getalby/bitcoin-connect-react').then(
    (mod) => [mod.isConnected, mod.onConnected, requestProvider]
  );
  
  // handle case where Bitcoin Connect is already connected
  // requestProvider will not launch a modal because a provider is already available.
  if (isConnected()) {
    (async () => {
      window.webln = await requestProvider();
    })();
  }

  // standard case (when user connects sometime in the future)
  const unsub = onConnected((provider) => {
    window.webln = provider;
  });
  
  return () => {
    unsub();
  };
}, []);

@bumi @reneaaron do you have any thoughts? (Note: this is not about specifically about setting the global webln object, but handling both cases for getting a provider without unexpectedly opening any modals)

@ekzyis
Copy link
Author

ekzyis commented Jan 1, 2024

Your code example works great!

Nitpick: As your code is, it wouldn't work since you use await in a non-async function.

And since useEffect also can't take async functions as arguments, you have to create an async function inside useEffect and call it. This is the code I am running in stackernews/stacker.news#715 (e06e862) now:

useEffect(() => {
  const unsub = []
  async function effect () {
    const [isConnected, onConnected, onDisconnected, requestProvider] = await import('@getalby/bitcoin-connect-react').then(
      (mod) => [mod.isConnected, mod.onConnected, mod.onDisconnected, mod.requestProvider]
    )
    if (isConnected()) {
      // requestProvider will not launch a modal because a provider is already available.
      // TODO but it might for wallets that must be unlocked?
      const provider = await requestProvider()
      setProvider(provider)
    }
    unsub.push(onConnected(async (provider) => {
      setProvider(provider)
      const info = await provider.getInfo()
      setInfo(info)
    }))
    unsub.push(onDisconnected(() => {
      setProvider(null)
      setInfo(null)
    }))
  }
  effect()

  return () => unsub.forEach(fn => fn())
}, [setProvider])

Also see my TODO:

// TODO but it might for wallets that must be unlocked?

I haven't tested this yet but wouldn't it try to unlock? Or would it not since a locked wallet is not considered connected?

@rolznz
Copy link
Collaborator

rolznz commented Jan 4, 2024

@ekzyis regarding the TODO: Bitcoin Connect saves your last connection to localStorage, and will automatically call enable on the provider as soon as the library is initialized, so you will get a popup anyway. Until enable() is called and the user allows any prompts, isConnected will be false.

@rolznz
Copy link
Collaborator

rolznz commented Jan 4, 2024

I will review this with Bumi and Rene and see if we go ahead with this updated documentation, or make a simpler way for devs to achieve this functionality.

@rolznz
Copy link
Collaborator

rolznz commented Jan 6, 2024

I talked with @bumi

  • we cannot rely on onConnected because the connector may or may not be connected before onConnected is even available to be called.
  • isConnected is somewhat unreliable if called after page load because it depends on how long the connector takes to be enabled (which depends on the connector and other factors)

I propose to change onConnected to immediately fire the callback if a provider is already available, and completely remove the isConnected function (deprecate it until v4 and then remove it).

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 a pull request may close this issue.

2 participants