ssbc / ssb-meta-feeds

10 stars 0 forks source link

findById can't find root, add failing test #116

Open Powersource opened 1 year ago

Powersource commented 1 year ago

for https://github.com/ssbc/ssb-meta-feeds/issues/115

Powersource commented 1 year ago

both new tests are failing. the iterating one only fails in the last step, where it gets the root. it doesn't error though, it just returns null

Powersource commented 1 year ago

what's praxis here, do we merge this now or later once we have a fix in this branch?

Powersource commented 1 year ago

https://github.com/ssbc/ssb-meta-feeds/blob/08ef0edeff00fa9bf3a1f9b9b6bd77589647e859/lookup.js#L279-L284

so does this only accept feeds that have been announced by other feeds? a bit confused by the db2 query. and does it have to return cb(null, null)? :(

Powersource commented 1 year ago

cc @mixmix although i think i might not need this fixed for my tribes2 pr atm so maybe no rush

Powersource commented 1 year ago

the docs for findById say

Given a feedId that is presumed to be a subfeed of some meta feed

does that mean this is intended behaviour? would maybe be nice with an error instead in that case.

mixmix commented 1 year ago

Correct @Powersource it only finds subfeeds. This is because of how the query is written. (currently this draws on lookup.js#findByFeedId)

  function findById(feedId, cb) {
    // ...

    sbot.db.query(
      where(subfeed(feedId)),    //  <<<<<<<<<<<<<<<<
      toCallback((err, msgs) => {
        if (err) return cb(err)

         // blah blah blah
      })
    )
  }

we can change the behaviour here but would like to understand the use case first , e.g. are you only wanting to find foreign root feeds?

mixmix commented 1 year ago

cc @Powersource

staltz commented 1 year ago

what's praxis here, do we merge this now or later once we have a fix in this branch?

Later when we have a fix in this branch, because master branch should always have CI green.

Powersource commented 1 year ago

@mixmix i needed to find someone's root feed id from a message they posted (group init msg) so i think i ended up not needing this fixed in the end. Maybe it's enough to clarify the readme and/or return an err when i'm using it wrong.

mixmix commented 1 year ago

I've written an isRootFeedId method which can be used in here if we want this fixed. It's in another PR that I've pinged you on

Powersource commented 1 year ago

that PR https://github.com/ssbc/ssb-meta-feeds/pull/120