search

holepunchto / hyperswarm

A distributed networking stack for connecting peers.
https://docs.holepunch.to
MIT License
1.03k stars 84 forks source link

Make boolean argument explicit in `peerInfo.ban(banStatus)` readme #138

Closed lejeunerenard closed 1 year ago

lejeunerenard commented 1 year ago

The docs seemed to imply that just calling the function would ban the peer but when I looked at the function itself it just sets this.banned = !!val. So the function actually requires a boolean to be passed which is the case in tests. Currently the default is actually to unban the peer if no argument is passed. Perhaps it should default to true?

Finally I noticed that peerInfo.ban() was called after a firewall check but wasn't passed true, so I fixed that as well.

LuKks commented 1 year ago

Nice catch! Probably should be this.banned = val !== false

LuKks commented 1 year ago

Ah except it can be considered a breaking change so it's wise as you did

lejeunerenard commented 1 year ago

Yeah. I can see an argument either way. If we're worried about breaking existing code, they may have assumed that it defaults to true as the docs implied. Even hyperswarm's own codebase makes this assumption. If we decide to default to true, the only code that will have a breaking change is code that intentionally relied on it defaulting to unbanning instead of banning. Which I would assume is a rare case, but I assume people at holepunch might know more about how the library's ecosystem looks like.

mafintosh commented 1 year ago

Thanks, in 4.5.0