-
Notifications
You must be signed in to change notification settings - Fork 578
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
Batch fetching of labels across posts and actors #1117
Conversation
@@ -70,6 +70,7 @@ export class FeedService { | |||
async getActorViews( | |||
dids: string[], | |||
viewer: string | null, | |||
skipLabels?: boolean, // @NOTE used by composeFeed() to batch label hydration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for non-obvious boolean vals like this, I kinda like wrapping it in an object so it's clear from the caller what you're asking for like
getActorViews(dids, requester, { skipLabels: true })
totally unnecessary, but could be a good convention to establish. I started doing it in some of our services etc. thoughts?
also in cases where you have a bunch of string parameters that could get mis-arranged or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm 100% down with that convention, really just reached for this because I thought it was more standard in our codebase 😆 I will tidy that up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops I forgot about this thread & merged the PR lol 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put it in the quality of life queue lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahaha sounds good!
@@ -341,6 +343,9 @@ export class FeedService { | |||
const post = posts[uri] | |||
const author = actors[post?.creator] | |||
if (!post || !author) return undefined | |||
// If the author labels are not hydrated yet, attempt to pull them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for the comment 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup looks great 👍
* Rename getLabelsForSubjects to getLabelsForUris * Generic method for getting profiles organized by did or aturi subject * Batch label fetching for posts and authors in feeds and threads * Tidy
* Rename getLabelsForSubjects to getLabelsForUris * Generic method for getting profiles organized by did or aturi subject * Batch label fetching for posts and authors in feeds and threads * Tidy
We don't have to land this, but this was a pretty quick/simple change I wanted to at least consider.
Since labels pervade the whole app.bsky API, the query to fetch labels is consistently the most trafficked in the entire application by around a factor of 2. The idea here is to nearly halve the number of label queries needed to satisfy feed and thread views.
The implementation allows opting-in to this behavior.
getLabelsForSubjects()
now works for both aturis and dids, aggregating profile labels for dids. It replacesgetLabelsForProfiles()
.getLabelsForUris()
is now the lower-level method to query for labels without the special profile/did behavior.getActorViews()
to skip fetching labels with theskipLabels
arg.getLabelsForSubjects()
to aggregate labels for both posts and actors.formatPostView()
to assign the actor labels if they are not present (i.e. if they were skipped ingetActorViews()
).