getAlby / bitcoin-connect

Connecting lightning wallets to your webapp has never been easier. Enable WebLN in all browsers with a single button
https://bitcoin-connect.com
MIT License
90 stars 30 forks source link

onConnected not fired on page load #176

Closed ekzyis closed 9 months ago

ekzyis commented 9 months ago

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 https://github.com/stackernews/stacker.news/pull/715

Here is a showcase video:

https://github.com/getAlby/bitcoin-connect/assets/27162016/294c8506-cc1a-48b5-aca4-e23259ff8f72

rolznz commented 9 months ago

@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 commented 9 months ago

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 https://github.com/stackernews/stacker.news/pull/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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

I talked with @bumi

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).