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

new API branchStream() #50

Merged
merged 4 commits into from
Sep 23, 2021
Merged

new API branchStream() #50

merged 4 commits into from
Sep 23, 2021

Conversation

staltz
Copy link
Member

@staltz staltz commented Sep 21, 2021

  • implement pull source branchStream()
  • tests
  • Update README
  • Hide/deprecate other APIs?

@staltz staltz marked this pull request as ready for review September 23, 2021 09:54
@staltz
Copy link
Member Author

staltz commented Sep 23, 2021

This PR is ready for review, and it's stable since I'm using it in ssbc/ssb-replication-scheduler#5 .

I took a look at all usages of ssb-meta-feeds and it seems like filter was never used, and in all cases where create is used, we should use findOrCreate instead, so I ended up deprecating (just by hiding from the README) those APIs that are not used. I think it's better to build APIs for actual use cases, than to work for imaginary use cases.

Now that we have loadState, I think find could be repurposed so that it works also on foreign meta feeds. Currently it's a bit confusing that some APIs operate only on your own meta feeds (find, findOrCreate) while other APIs operate on meta feeds from anyone (findByIdSync, branchStream). If we make find work for any meta feed, then it's obvious that findOrCreate is only for your own meta feed due to the name 'create'.

But I could do that change to find in a separate PR because this one is about branchStream, and it's ready.

@staltz staltz requested a review from arj03 September 23, 2021 09:58
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@arj03
Copy link
Member

arj03 commented Sep 23, 2021

This PR is ready for review, and it's stable since I'm using it in ssbc/ssb-replication-scheduler#5 .

This is a bit light on tests for this new api, but since it is used in the other module I guess that counts as testing :)

But I could do that change to find in a separate PR because this one is about branchStream, and it's ready.

Yes! It would be great if find worked on any feed.

Copy link
Member

@arj03 arj03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets get this merged and continue working on the API in another branch. Overall it's great to have this kind of API in here because it makes it similar to ssb-friends and other modules.

@staltz staltz merged commit 888af93 into master Sep 23, 2021
@staltz staltz deleted the tree-api branch September 23, 2021 12:02
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.

2 participants