feat: self emit option

[vasco-santos]

I found a behavioral inconsistency between the libp2p-floodsub implementation and this implementation.

In libp2p-floodsub, when a peer A subscribes a topic, if that same peer A publishes for that topic, the topic is also emitted to self, which does not happen in this implementation. I added this behavior behind a flag, which defaults to false right now, in order to keep the behavior you previously added. I chose false for now, as you had tests that verified if the self was not emitted to self.

In libp2p-floodsub we currently do emit to self all the times. I also agree that there are several use cases, where we may not want that, and in this context, I will also add this option in libp2p-floodsub.

I will open an issue on libp2p/js-libp2p-pubsub for discussing which should be the default behavior from js-libp2p users, as I discussed this previously with @jacobheun . If you have no reason to not do this emit self by default, I would prefer to set it true for now in this PR, as we do in floodsub (will be a breaking change!). Otherwise, I will make it true by default from js-libp2p, until we decide if we should turn it into a false default.

NOTE: This PR also contains #39 commits, as I want to use this branch for testing the integration with js-ipfs. So, once #39 is merged, those 2 extra commits will be removed.

[GregTheGreek]

Out of curiosity, what is the use case for emitting to self?

[GregTheGreek]

LGTM

[vasco-santos]

Quoting @jacobheun

the advantage of emitting to yourself is that you don't have to handle your message, and then publish it. you just publish, and just maintain one handler
So it simplifies things a bit for consumers.

So, for example a real time dapp would benefit from publishing data, and it appears on the UI, through the same handler where it receives messages from the other peers.

[GregTheGreek]

Thats very curious

[jacobheun]

I am unsure of the actual intent for originally doing this, it's an assumption I made for the use case for users. I'd personally prefer to just take it out as part of moving pubsub out of experimental (alongside the gossipsub support release of libp2p), but there are potential impacts to end users, which is what this PR aims to avoid.

Since the config is changing we could remove it from floodsub and just be clear about this in the docs and release notes for libp2p.

[GregTheGreek]

I am unsure of the actual intent for originally doing this, it's an assumption I made for the use case for users. I'd personally prefer to just take it out as part of moving pubsub out of experimental (alongside the gossipsub support release of libp2p), but there are potential impacts to end users, which is what this PR aims to avoid.

Since the config is changing we could remove it from floodsub and just be clear about this in the docs and release notes for libp2p.

Might be worth reaching out to people whom are using the package to see if it causes a headache first? I know the go libp2p did a change that broke a fair bit of stuff a few weeks ago, happy to take this one slowly

[vasco-santos]

So @jacobheun in your opinion, we default to false both here and in js-libp2p-floodsub, and allow on js-libp2p the users to set if to true, if they intend?

I think that taking into account the breaking change we will do in js-libp2p, it is fine to do that with a release note

[vasco-santos]

Also, this affects js-ipfs tests, which expect the self emit to happen.

[jacobheun]

I think the right thing to do is to make it compatible with how it is now, as this PR proposes, and then try to move away from it after some community discussion. I'd rather have this in there and get Gossipsub out without needing to worry about this level of implementation detail.

[GregTheGreek]

makes sense to me - we'll wait for @wemeetagain review

[wemeetagain]

LGTM, we're currently relying on emitSelf being false.

Somewhat unrelated, I think we'll want to be able to also control when we forward messages in the future. Right now, we auto-forward received messages in subscribed topics, but we will also want to switch that off.

[codecov-io]

Codecov Report

Merging #40 into master will decrease coverage by 1.63%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
- Coverage   85.45%   83.82%   -1.64%     
==========================================
  Files           8        8              
  Lines         495      513      +18     
==========================================
+ Hits          423      430       +7     
- Misses         72       83      +11

Impacted Files	Coverage Δ
src/index.js	81.6% <ø> (ø)	⬆️
src/pubsub.js	87.84% <50%> (-5.41%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b99aad4...bf17585. Read the comment docs.

[vasco-santos]

@wemeetagain @GregTheGreek

Rebased the PR after the message validation PR got merged. I think this is good to go

Merge #39 previously than this

[GregTheGreek]

LGTM