-
Notifications
You must be signed in to change notification settings - Fork 28
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
Disable published activities pooling #802
base: main
Are you sure you want to change the base?
Conversation
e60e78d
to
e321ea0
Compare
485fd6c
to
c77dfd1
Compare
[non-blocking consideration] |
c77dfd1
to
f0ef879
Compare
@@ -42,11 +42,12 @@ const createDebouncedActivity = ({ userId, shelfId }: createActivityParams) => a | |||
} | |||
|
|||
async function _createDebouncedActivity ({ userId, shelfId }: createActivityParams) { |
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.
The point of debouncing was to pool activities, so if [edited] user.poolActivities === true
user.poolActivities === false
the activities shouldn't be debounced, no?
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 don't get it, for me, your comment says:
debounce === pool
so if (pool === true)
then debounce
should be false
Can you see the contradiction I see?
If we pool, activities are debounced, no? As it was the case before this PR. Or I don't understand at all what's going on here?
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 messed up the logic in my comment, sorry, see edited version
@@ -68,12 +70,11 @@ async function _createDebouncedActivity ({ userId, shelfId }: createActivityPara | |||
actor: { name }, | |||
object: { items: { since, until: Date.now() } }, |
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.
As noted in your PR introductory comment, if [edited] user.poolActivities === true
user.poolActivities === false
, no need to create this activity doc: the item doc itself has all we need (just like entity activities are built from patch docs, without needing to store an activity doc). I would be in favor to try doing that in this PR, as having a single item activity with since and until is really weird
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 explored how to do what you suggest. After a few hours of refactoring, I think it's not worth it anymore.
Because poolActivities
is used quite deeply in the current code: at the very formatting of the activity (formatUserItemsActivities
) and it's used by the outbox
endpoint, the activity
endpoint and the postActivity
function.
So extracting poolActivities
(necessary to pass directly an items
array to formatUserItemsActivities
, what you say with "the item doc itself has all we need") leaves an early if
which then have functions that almost look like each other (ie.
formatUserItemsActivities({ activitiesDocs, user })
andformatUserItemsActivities({ items, user })
)
This duck typing is more difficult to read, and introduces different behaviours using different objects (items
& activities
), it feels it will be even harder to refactor later IMO. (Plus, all the refactoring have to be done with the shelves
part too)
Your comment/diagnosis is legit, but the intervention is trickier than expected. The main reason behind is that we now want to keep both behaviours available (pooling and not pooling). The fact that it is now unpooled by default, makes it easy to check later is this pooling feature is actually activated by people. What about leave the PR like this, wait to see if the pooling feature is actually used, and then decide to remove or keeping it once we figured it out ?
Note: after the exploration, I ended up with the feeling that if the pooling feature is here to not spam other instances, what about only having pooling when sending create activities, but not on outbox
and activity
endpoints ? This would induce to remove the possibility to send request to the activity
endpoint and directly build the link in the pooled activity only with the first item created.
Note2: The more I think about it, the more I think we should get rid of the complexity and remove pooling possibility all together.
f0ef879
to
c258dce
Compare
d63e856
to
f76df8e
Compare
This is an attempt to "unpool" recent activities.
While having to deal with past decisions (storing in db
since
anduntil
inside one activity, seems rigid to our current needs...), this implementation is still relying on those stored activities, to avoid having to re-write/touch on the current activities stored in db.Choosing this implementation has the advantages of fully reusing existing functions (refactoring for directly formatting items could be done later)
To-do later: delete any pooling possibilities, and stop storing
since
andthen
activitiesSolves #574
Client PR #526
To be merged after #801