feat: initial implementation

[vasco-santos]

In the context of creating js-libp2p-gossipsub, we extracted the base protocol of js-libp2p-floodsub to its own module.

[jacobheun]

I think we should add more documentation, and potentially an example for extending pubsub.

Looking through the code, it's not clear to me what methods I should be overriding, what I should avoid overriding, and if there are any methods I must call super on.

If I override _addPeer will bad things happen? Are there minimum methods I need to override to implement a custom pubsub algorithm? The usage section might be sufficient for describing this, but perhaps method comments there would be appropriate? My thoughts are trying to convey something similar to the nodejs stream implementation guidelines, for example the MUST NOT language at https://nodejs.org/docs/latest-v10.x/api/stream.html#stream_writable_write_chunk_encoding_callback_1

[vasco-santos]

Updated @jacobheun

Can you have a look?

[Mikerah]

I noticed that the updated docs say that you must not override add_peer and remove_peer. I think these should be functions that you can, at the very least, call super on in order to make changes to how that particular pubsub variant handles adding/removing peers. For example, in gossipsub, upon removing a peer, you need not only make sure that no other peer is connect to it, you need to remove that peer from the mesh, fanout and other data structures in the gossipsub router state.

[vasco-santos]

Thanks for your feedback @Mikerah !

You are right and I will change it. Perhaps document that add_peer and remove_peer may be overwritten if the pubsub implementation needs to add custom logic for peers to be added and removed.

From your perspective, do you think the rest of the documentation is clear enough and suit gossipsub needs?

[Mikerah]

I think adding more JS docs to the functions might be helpful. It took me a while to figure out what each function took as input and what exactly they returned.

Also, what's the reasoning for creating a separate peer class when you can use a combination of peerInfo and js-libp2p-switch? It seems like these 2 modules provide the same functionality.

Another thing that I noticed is that in the Go pubsub router, it contains this variable topics that maps topics to sets of peers. You can see it here. This has been used in both Floodsub and Gossipsub routers implemented in Go. I'm not sure if this is needed in the JS implementation as well but it's worth noting.

[vasco-santos]

I think adding more JS docs to the functions might be helpful. It took me a while to figure out what each function took as input and what exactly they returned.

I added docs as @jacobheun suggested. Did you see the last version? You can point me to the ones that are less clear and we can try to improve them.

Also, what's the reasoning for creating a separate peer class when you can use a combination of peerInfo and js-libp2p-switch? It seems like these 2 modules provide the same functionality.

From my quick analysis, we use it essentially for combining the PeerInfo data swith the pubsub context state and to handle the rpc messages. I am not sure if this is the best solution or not (this codebase has more than a year), I just extracted the code from floodsub for now. I avoided diving deep into this code base for this migration, as we will go with the async iterators refactor during this quarter and I intend to be more critic about the current logic of some modules when I go through the refactor. It will be nice if you open issues when you get into things that you do not agree, enabling us to improve our codebase.

Another thing that I noticed is that in the Go pubsub router, it contains this variable topics that maps topics to sets of peers. You can see it here. This has been used in both Floodsub and Gossipsub routers implemented in Go. I'm not sure if this is needed in the JS implementation as well but it's worth noting.

I believ it should be useful in JS land too! I believe that it may be useful across all implementations. Can you open an issue for that?

[Mikerah]

What I mean by extra documentation is to add @params and @types to function definitions and variables. The Peer class provides this. I think it should be added to the BaseProtocol class. I think also private methods should be documented as well, even though they are not exposed in the API to other developers.

[jacobheun]

I think also private methods should be documented as well, even though they are not exposed in the API to other developers.

I agree. I think once we have those documented we can merge this and then iterate from there.

[vasco-santos]

Totally agree with you @Mikerah

One of our goals is to add better documentation (as you suggested) to all libp2p modules. This is a task that we will be working during this quarter and the status tracking is in libp2p/js-libp2p#308.

We should move this PR forward now and tackle the documentation improvements later. If you are willing to help, we are open to receive PRs.

[vasco-santos]

Meanwhile, I added the documentation for private functions.

We should add examples and a better documentation in the README in a next step

[jacobheun]

This is good to merge, we can iterate from here.