Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: make PublishError.InsufficientPeers more self-explanatory #487

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

achingbrain
Copy link
Collaborator

People continually mistake the InsufficientPeers error for something that can be "worked around" by setting allowPublishToZeroPeers to true, and they then expect other network nodes to still recieve their messages.

This PR renames the option to allowPublishToZeroTopicPeers - this makes it clear(er) that it's not that you don't have any peers, it's that no peers you have are listening on the topic.

It also improves the JSDoc comment on the option and changes the Error message from PublishError.InsufficientPeers to PublishError.NoPeersSubscribedToTopic which (I hope) more accurately describes what's wrong.

BREAKING CHANGE: The allowPublishToZeroPeers option has been renamed to allowPublishToZeroTopicPeers

Fixes #472

People continually mistake the `InsufficientPeers` error for something
that can be "worked around" by setting `allowPublishToZeroPeers` to true,
and they then expect other network nodes to still recieve their messages.

This PR renames the option to `allowPublishToZeroTopicPeers` - this makes
it clear(er) that it's not that you don't have any peers, it's that no
peers you have are listening on the topic.

It also improves the JSDoc comment on the option and changes the `Error`
message from `PublishError.InsufficientPeers` to `PublishError.NoPeersSubscribedToTopic`
which (I hope) more accurately describes what's wrong.

BREAKING CHANGE: The `allowPublishToZeroPeers` option has been renamed to `allowPublishToZeroTopicPeers`

Fixes #472
@achingbrain achingbrain requested a review from a team as a code owner February 2, 2024 13:25
@achingbrain
Copy link
Collaborator Author

achingbrain commented Feb 2, 2024

I'm not sure this is worth a major bump on it's own, maybe it should be batched up with #468?

@codecov-commenter
Copy link

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (b4e6a8d) 81.40% compared to head (435e8dd) 81.42%.

Files Patch % Lines
src/index.ts 91.66% 1 Missing ⚠️
test/compliance.spec.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #487      +/-   ##
==========================================
+ Coverage   81.40%   81.42%   +0.02%     
==========================================
  Files          48       48              
  Lines       12325    12340      +15     
  Branches     1301     1301              
==========================================
+ Hits        10033    10048      +15     
  Misses       2292     2292              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wemeetagain wemeetagain merged commit 1958aab into master Feb 27, 2024
9 checks passed
@wemeetagain wemeetagain deleted the fix/improve-zero-peers-error branch February 27, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make PublishError.InsufficientPeers a bit more self-explanatory?
3 participants