Remove needs of `waitForRemotePeer`

fryorcraken commented 1 year ago

This is a change request

Problem

Currently, a user needs to use waitForRemotePeer before they are able to use their Waku node. Whether it is to send message via Light Push, subscribe via Filter or retrieve via Store.

waitForRemotePeer hosts a complex logic which creates a number of code smells (maintainability is C).

Acceptance Criteria

[ ] Remove the need of waitForRemotePeer to make the API easier to use and also reduce complexity of js-waku.
[ ] Mark waitForRemotePeer as deprecated and review developer feedback if any
[ ] Remove or replace waitForRemotePeer based on feedback above.

Proposed Solutions

The solution would be to embed the waitForRemotePeer logic directly in the protocols. However, because it is embedded than the logic would be simpler as we would expect only one libp2p protocol codec.

For example, when calliing LightPush.push, if no peer is available, wait for a peer using libp2p events and send the message using said peer as soon as the peer connect/protocol change event is emitted.

Notes

Another step to improve the API could be to remove the Waku.start() call, as recently done in libp2p.
An option such as failFast could be added to the API to not wait for a peer connection.

weboko commented 1 year ago

To remove waitForRemotePeer as explicit dependency is a good idea but I think the title is a bit broader than the problem proposed and we should also consider preferring event based approach over async/await for other places, not able to name yet but this one can be an example.

fryorcraken commented 1 year ago

I think the title is a bit broader than the problem proposed

Good point, title changed.

we should also consider preferring event based approach over async/await for other places, not able to name yet

We can track one issue at a time where we can see benefit of using event based pattern.

fryorcraken commented 1 year ago

See https://github.com/waku-org/js-waku/pull/1305#issuecomment-1514086900

fryorcraken commented 1 year ago

Discussion:

This API is used by some consumers, this is a breaking change and we would need to provide equivalent functionality
No strong reason to change the API right now, only see it as a "nice to have" to amke the API smoother (save one line)
Worried about breaking single purpose function principle by having protocols api wait for connection and possibly time out
As we enhance connection manager functionality and extend its API, we may be able to extract equivalent functionality at some point in the future.

waku-org / js-waku