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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Powersource
Copy link
Collaborator

@Powersource Powersource commented Feb 10, 2023

for #115

@Powersource
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@Powersource
Copy link
Collaborator Author

ssb-meta-feeds/lookup.js

Lines 279 to 284 in 08ef0ed

msgs = msgs.filter((m) =>
m.value.content.type.startsWith('metafeed/add/')
)
if (msgs.length === 0) {
return cb(null, null)
}

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
Copy link
Collaborator Author

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

@Powersource
Copy link
Collaborator Author

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
Copy link
Member

mixmix commented Feb 13, 2023

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
Copy link
Member

mixmix commented Feb 13, 2023

cc @Powersource

@staltz
Copy link
Member

staltz commented Feb 13, 2023

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
Copy link
Collaborator Author

@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
Copy link
Member

mixmix commented Feb 16, 2023

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
Copy link
Collaborator Author

that PR #120

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.

3 participants