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

hard deprecate find() and filter() #52

Merged
merged 3 commits into from
Sep 29, 2021
Merged

hard deprecate find() and filter() #52

merged 3 commits into from
Sep 29, 2021

Conversation

staltz
Copy link
Member

@staltz staltz commented Sep 23, 2021

So, I began by thinking how to make find(metafeed, visit, cb) be usable for foreign peers. It wasn't so obvious, because find could be used in so many ways:

  • find(null, null, cb) to get your own root mf
  • find(myRoot, visit, cb) to get my own subfeeds
  • find(somethingConfusing, visit, cb) to get foreign subfeeds

In the third case, what if you just wanted to pass a visit function and you don't care about the 1st arg? That seems ambiguous because if the 1st arg is null, it means you want to find something that you own. And actually find(null, null, cb) is also ambiguous because it looks like you want to get any subfeed.


So I side stepped this whole headache when I realized that branchStream can already be used as an API to find any subfeeds. You just need to use standard pull-stream filter and take.

So this PR makes this change to the API:

-filter
-find
-create
 findOrCreate
+getRoot
 findById
 findByIdSync
 loadState
 ensureLoaded
 branchStream
-filterTombstoned
-findTombstoned

I intend to update branchStream so that it gets opts.tombstoned as a boolean, and this should cover our needs for filterTombstoned and findTombstoned. But I'll do that in another PR (anyway, we need a new API to create a tombstone).

The reason why I added getRoot is that there is one use case in ssb-replication-scheduler where we just want to check if we have a root meta feed, but not create it.

@staltz staltz requested a review from arj03 September 23, 2021 13:33
@staltz
Copy link
Member Author

staltz commented Sep 23, 2021

Hmm, now I'm not sure. To implement tombstone, I need to have the full keys object for the to-be-tombstoned subfeed, because the keys will sign the bendy butt message content. So I need some way of "finding and tombstoning" because you wouldn't want to findOrCreate a subfeed just in order to tombstone it.

So I have a few options:

  • findAndTombstone(metafeed, visit, cb)
  • Re-introduce find so the developer can do find + tombstone
    • Should find be called findOwn or should find be made as generic as possible? argh

Thoughts?

@arj03
Copy link
Member

arj03 commented Sep 24, 2021

I like findAndTombstone(metafeed, visit, cb) best. Also, even if you can just do a filter on branchStream, it might still be nice to a find like function that filters for you.

@staltz
Copy link
Member Author

staltz commented Sep 24, 2021

Thanks for the thoughts, I think I'd build findAndTombstone indeed. If we're going to have find, can you suggest arguments for it? See e.g. my confusion with the 3 bullet points at the top of this PR

@arj03
Copy link
Member

arj03 commented Sep 24, 2021

Can't we make it work like:

sbot.metafeeds.filter(visit, cb)

And then it would find anything where visit returns true?

@staltz
Copy link
Member Author

staltz commented Sep 24, 2021

We could, but depending on the use case, it wouldn't be enough. Like if you want to find one of your own meta feeds, then it's quite difficult to detect the difference with foreign meta feeds (given the branch, get the root from the first item, then look for other branches that have the same root but have the metafeed/announce for the main too).

I'd rather build APIs with a real-world use case in mind, and I haven't found a case where we strictly need find or filter

@staltz staltz marked this pull request as ready for review September 29, 2021 14:29
@staltz
Copy link
Member Author

staltz commented Sep 29, 2021

@arj03 Okay, this PR now also adds findAndTombstone.

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.

This looks great. Much simpler api and a lot of minor things fixed I can see.

@staltz staltz merged commit 6083f07 into master Sep 29, 2021
@staltz staltz deleted the newfind branch September 29, 2021 14:58
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.

None yet

2 participants