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

nicer loadFeed API #60

Closed
mixmix opened this issue May 26, 2022 · 9 comments
Closed

nicer loadFeed API #60

mixmix opened this issue May 26, 2022 · 9 comments

Comments

@mixmix
Copy link
Member

mixmix commented May 26, 2022

current gripes with ssb.metafeeds.findOrCreate API:

  • it's painfully verbose
    • why do I have to add this "visit" thing, is this a special custom filter..... isn't this extra for experts
  • once you have the feed what do you do with it?
  • as a beginner I'm forced to know too much stuff, I'm scared I'm gonna fuck something up
    • give me sensible defaults!

Proposal

const loadFeed = await LoadFeed(ssb)
const chessFeed = await loadFeed({ purpose: 'chess' })
const chessCommentary = await loadFeed({ purpose: 'chess-commentary', metafeed: chessFeed })

NOTE: you never need or want to engage specifically with the rootFeed right?
Don't make it easy for me to publish trash to that feed accidentally. Just give me what I generally need for building and make access of dangerous things harder

async function LoadFeed (ssb) {
  const rootFeed = await p(ssb.metafeeds.findOrCreate)()
  console.log('Root Feed:', rootFeed)

  return async function loadFeed (details) {
    if (!details.metafeed) details.metafeed = rootFeed
    if (!details.format) details.format = 'classic'

    const feed = await p(ssb.metafeeds.findOrCreate)(
      details.metafeed,
      IsFeed(details),
      {
        feedpurpose: details.purpose,
        feedformat: details.format,
        metadata: details.metadata
      }
    )
    console.log(`${details.purpose} Feed:`, feed)

    feed.publish = (content, cb) => ssb.db.publishAs(feed.keys, content, cb)

    return feed
  }

  function IsFeed (details) {
    return (feed) => {
      return (
        feed.feedpurpose === details.purpose &&
        feed.feedformat === details.format
      )
    }
  }
}
@mixmix
Copy link
Member Author

mixmix commented May 26, 2022

Other ideas:

const root = await LoadFeed(ssb)
const chess = await root.loadFeed({ purpose: 'chess' })
const chessSettings = await chess.loadFeed({ purpose: 'chess-settings' })
chess.publish // handles publishAs
chess.loadFeed // handles subfeed

@staltz
Copy link
Member

staltz commented May 26, 2022

@mixmix Interesting feedback, this is nice to know. What I aimed at doing with the API is to make it minimal and to be useful, not necessarily to make it easy. Easy is important, but perhaps that can be added on top of this API which is what you could call "low level".

For example, the visit function. It's like Array.prototype.find (makes sense, right? array.find and metafeeds find are similar). It lets you find a subfeed based on any criteria you can imagine. Like if I want to find a subfeed where the metadata has score greater than 7:

sbot.metafeeds.findOrCreate(
  rootMF, 
  (feed) => feed.metadata && feed.metadata.score > 7, 
  {feedpurpose: 'mygame', feedformat: 'classic', metadata: {score: 8}},    
  (err, subfeed) => {
    console.log(subfeed)
  }
)

You couldn't do that with an object-based "visit" API.

And these APIs return the subfeed metadata and keys because that's serializable over muxrpc (like, in Manyverse, you can call sbot.metafeeds.findOrCreate on the frontend side). If we would return functions or objects with functions (like your feed.publish idea), this would be a useless API over a muxrpc boundary.

You can easily build a nicer API on top of our current API, but you couldn't achieve these two things I mentioned above with a nicer API. This means that the current API is good. It's a low-level API that allows us to build whatever we want with metafeeds, it's not a high-level API that allows beginners to easily build with metafeeds.

If you have other API improvement ideas that preserve the utility of the API, of course those are easily mergeable improvements.

@mixmix
Copy link
Member Author

mixmix commented May 26, 2022

Oh yeah interesting @staltz I notice that I don't use the muxrpc connection for UI functionality that much. My setup for the past 3+ years has always been using the API locally, and have it driven over an http API (graphql), so no muxrpc....and that was mainly because teaching front end people to use ssb queries and pull-streams was too much.... but now there are things like ssb-crut I think that could be worth re-checking.


my main reflection is that the signature is currently

findOrCreate (
  metafeed, // only needed for "create"
  find, // a.k.a. visit, only used for "find" in findOrCreate
  details, // only used in create
  cb
)

why not make it

```js
findOrCreate (
  find, // find an exiting one that matches
  createDetails  // or use these details to create one
)

// createDetails {
//   purpose: String,
//   type: String,          (defualt: bendybutt-v1)
//   metaFeed: Feed,
// }

Or if you prefer to read code

function findOrCreate (find, createDetails, cb) {
  // findOrCreate()
  if (find === undefined && createDetails === undefined) {
    createDetails = {
      metaFeed: null,
      purpose: 'root',
      type: 'bendybutt-v1'
    }
    find = defaultFind(createDetails)
  }

  // findOrCreate(createDetails)
  if (createDetails === undefined) {
    createDetails = find
    find = defaultFind(createDetails)
  }

  const exisitngFeed = feeds.find(find)   // feeds: Array
  if (exisitngFeed) return cb(null, exisitngFeed)
  else return createNewFeed(createDetails, cb)
}

function defaultFind (createDetails) {
  return (feed) => (
    feed.purpose === createDetails.purpose &&
    feed.type === createDetails.type
  )
}

@mixmix
Copy link
Member Author

mixmix commented May 26, 2022

I guess I don't have a sense of how often the visit function will be an interesting function. The example given in the documentation doesn't use a function that's special - and that looks good to me. Like when would I want to do that thing with the score. I'm assuming it's finding the first non-tombstoned feed that's a match right?

oh I can see a use-case - multiple users on the same log. Would probably only be useful if it were multiple personas on the log, as encryption on a shared device.... is not well resolved for different identities
But yeah with the find being function based you could have two "persona" subfeeds which then each have their own "chess" subfeeds....

but that case is also solved because you pass metaFeed into the createDetails ... so you can still have multiple purpose: "chess" as they would have different metaFeeds.

But all of this is moot (just frivolous argument) because you can supply a find /visit function, I'm just saying I suspect it might be a rare thing that's needed.... and if that's the case then making it optional would be good

sorry I'm verbose I'm tired so weaving is a bit messier

@staltz
Copy link
Member

staltz commented May 26, 2022

findOrCreate (
  find, // find an exiting one that matches
  createDetails  // or use these details to create one
)

This doesn't quite work, because the metafeed is used not only for creating, it's used for finding too. E.g. find a subfeed under "groupsRoot" (which by the way is under "root") that fits this and this criteria. And that subfeed may already exist, not requiring a create operation.

@staltz
Copy link
Member

staltz commented May 26, 2022

PS: I'm not a fan of APIs that are all flexible and work no matter what you toss at them. It makes it much harder to extend the API, makes the implementation more complex, and overall doesn't have a well defined contract. I prefer clearly defined contracts: if you provide me X in this shape, I will give you Y. Otherwise you're violating the contract and things will either crash or throw errors or worse. I also heavily dislike APIs that have a variable number of arguments if some of them are undefined ("in what slot am I supposed to actually put this argument? why are you shuffling my arguments around??").

To me that's separation of responsibilities and ends up simplifying the architecture of codebases. My library code isn't trying to do more than it should, it's trying to do it's clearly defined job.

@mixmix
Copy link
Member Author

mixmix commented May 26, 2022

yeah I'm not a fan of the shuffle either, but I guess I saw an opening in that you are accepting the method called when there's no arguments at all. Guess that's a little different.


This doesn't quite work, because the...

I don't quite see it, can we not just make the defaultFInd this:

function defaultFind (createDetails) {
  return (feed) => (
    feed.metaFeed.keys.id === createDetails.metaFeed.keys.id &&
    feed.purpose === createDetails.purpose &&
    feed.type === createDetails.type
  )

@mixmix
Copy link
Member Author

mixmix commented May 26, 2022

How about a signature like this:

findOrCreate(
  {
    metaFeed,             // optional, (default: rootFeed)
    purpose: 'chess',
    type,                 // optional, (default: bendybutt)
    find                  // optional  (default: fn which matches metaFeed, purpose, type)
  },
  cb
)

then change the root getting findOrCreate to

// findOrCreate(cb)  // formerly
findOrCreate({ metaFeed: null }, cb)

this shouldn't really need to be called much, but makes the signature totally uniform

@mixmix
Copy link
Member Author

mixmix commented Sep 28, 2022

I think this is solved with new API

@mixmix mixmix closed this as completed Sep 28, 2022
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

No branches or pull requests

2 participants