core: restart relays connection when relaysUrl is updated

[aussedatlo]

Actually, we can't connect with different relays when we update directly relaysUrl props of Provider.

With this modification, connectToRelays() is only triggered if relaysUrl change, this means that we can dynamically update the relay list from the provider.

For example :

const CustomProvider = observer(({ children }) => {
  const { userStore } = useStores();

    return (
      <NostrProvider relayUrls={userStore.relays} debug={true}>
        {children}
      </NostrProvider>
    );

  return <>{children}</>;
});

If the value of userStore.relays is updated, the provider will automatically reconnect to the new relays.

[wds4]

If I follow correctly, you're basically just eliminating the isFirstRender check. Was this not needed? If you go from N relays to N+1, will this cause problems trying to reconnect to the original N?

[aussedatlo]

The isFirstRender check make it impossible to trigger connectToRelays() when props are updated, since it will trigger only on first mount.

With this modification, when you update from N to N+1 relays, it will detect the changes, disconnect from all N relays then reconnect to all N+1 new relays.

Maybe this is not the best optimized solution since it's not needed to disconnect a relay present in N and N+1 list, but it allow to dynamically update the props and reconnect accordingly.

This list isn't going to change all the time so I dont think this is a big deal right now.

Maybe there is an other method for this but in my side I needed to apply this patch for doing it !

[wds4]

I tend to agree that disconnecting and then reconnecting is not a big deal, at least for now. If dynamic management of relays were ever to result in frequent changes, then perhaps it would become a bigger issue.

I will test it as soon as I get the chance! 😄 (today or this weekend, I hope)

[wds4]

I've tested it out. As you can see here, I am using my redux store to define relayUrls.

I find that when I add a relay to my store, the new relay gets connected very quickly, which is nice. However, if I remove a relay from the store, that relay stays connected, which I do not want. I also find that if one or more connections get dropped spontaneously, they don't auto reconnect; however, if I then add a new relay, then all of the dropped connections get reconnected at the same time as the new relay.

So it seems to me that the current behavior of this fork is that whenever a change in the store produces a change in relayUrls, it cycles once through all relays and tries to connect each one, regardless whether a connection already exists. However, it does not disconnect from any of them, except for the ones that are spontaneously dropped. Presumably, trying to connect to a relay that is already connected isn't causing any problems (although if it happens repeatedly, I suppose the relay could interpret that as an attack and could put you on an ignore list, which I suppose is the point of the isFirstRender check in the first place.)

I have a component relaysStatus which is working well for me which uses connectedRelays to monitor which relays are active:

import { useNostr } from 'nostr-react';
...
const { connectedRelays } = useNostr();

Perhaps in place of the isFirstRender check, we need to check with connectedRelays first and avoid reconnecting to an already-active url. In addition, we should cycle through all relays in connectedRelays and make sure to disconnect from any undesired relay.

[aussedatlo]

The bug was reproducible in my side, in fact we need to handle the relays that we don't want anymore.

I have updated my branches with the following modifications:

• Fix dependency array for connectToRelays : we need to use a correct dependency array to limit undesired bugs
• Add disconnectToRelays function to disconnect relays that are not needed anymore.
• Add reference for relayUrls : since connectToRelays is updated correctly, we don't want to retrigger it when one of the dependency has changed. The useEffect will watch changes between old and new relayUrls to connect/disconnect only if changes are detected.

Now, I can correctly add or remove a relay url and only the changes that are needed are applied.

If you have time to test it out, don't hesitate to tell me the result @wds4

[wds4]

I've done a quick test with your updated branch and it appears to be working for me: I can add and remove relays to my store and they are added and removed as expected from connectedRelays. I've tested adding and removing the same relay repeatedly and that also functions as expected. Junk relays (wss://foo.bar) or other relays that do not connect successfully are (effectively) ignored without creating any problems that I have identified. Good job and thank you!

I still have the problem that if a connection gets dropped spontaneously, it does not reconnect unless I make some other (potentially unrelated) change to relayUrls. I'd say that issue could be addressed separately from this pull request, unless you have an idea of how you would like to add it to this PR.

[aussedatlo]

Actually there is no auto reconnection implemented ans this pr effectively does not add this functionnality. However, you could use onDisconnect callback to implement it on your side, something like

  const { onDisconnect } = useNostr();

  const onDisconnectCallback = (relay) => {
    setTimeout(() => {
      relay
        .connect()
        .then(() => console.log(`reconnected: ${relay.url}`))
        .catch(() => console.log(`unable to reconnect: ${relay.url}`));
    }, 30000);
  };

  onDisconnect(onDisconnectCallback);

[wds4]

From a quick implementation, I got onDisconnectCallback to execute as expected with the above code. It's exactly what I've been needing to do to solve this problem- thank you!

For others with the same problem I'll post code once I have time to get it a little more fleshed out and tested in my client. Probably not until later this week. For now my plan is to use this branch with onDisconnectCallback as above.

Edit: I suppose no need for me to post any code for using onDisconnectCallback in addition to what @aussedatlo already posted above. At the risk of showcasing my ignorance I'll just mention one mistake I made at first, which is that I tried to implement onDisconnectCallback in the same function that calls NostrProvider, whereas what I needed to do was nest it inside.

[aussedatlo]

@t4t5 it is possible to integrate this PR ?

[t4t5]

Apologies for the delay here. This looks great, thanks for fixing this @aussedatlo! 🙏 Will merge and create a new release!