Page MenuHomePhabricator

[lib] Add function allowing extracting ThreadInfo from updates
ClosedPublic

Authored by tomek on Aug 13 2024, 5:41 AM.
Tags
None
Referenced Files
F2766968: D13057.id43492.diff
Thu, Sep 19, 3:56 PM
F2766963: D13057.id43506.diff
Thu, Sep 19, 3:55 PM
F2766914: D13057.id43333.diff
Thu, Sep 19, 3:49 PM
F2766907: D13057.id43505.diff
Thu, Sep 19, 3:49 PM
F2763751: D13057.diff
Thu, Sep 19, 11:43 AM
Unknown Object (File)
Tue, Sep 17, 1:42 PM
Unknown Object (File)
Mon, Sep 16, 3:18 AM
Unknown Object (File)
Sun, Sep 15, 9:07 AM
Subscribers

Details

Summary

We need this later in the stack to extract updated thread info from the result of operations processing.

https://linear.app/comm/issue/ENG-8930/unify-the-approach-to-replies-count-and-unread-status-updates

Depends on D13055

Test Plan

Flow, tested with the rest of the stack.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Aug 13 2024, 5:58 AM

One thing we could do here is add a separate update type for unread status. This way the logic from D13059 that needs getUpdatedThreadInfo would not be needed, and so getUpdatedThreadInfo would not be needed. But this might be too much work.

This revision is now accepted and ready to land.Aug 19 2024, 6:46 AM
In D13057#370009, @inka wrote:

One thing we could do here is add a separate update type for unread status. This way the logic from D13059 that needs getUpdatedThreadInfo would not be needed, and so getUpdatedThreadInfo would not be needed. But this might be too much work.

Yeah, I'm still not sure if it is a good idea. This change would greatly simplify this logic, but has some downsides: introducing a whole new update type would require implementing its spec, which contains e.g. keyserver db layer - and we don't need that, but skipping it would also be weird. If we introduce this new update type, we will need to answer a question as to why we don't introduce similar updates for other props of the thread info... and it seems like we should do that - introduce a new update info that allows updating any property of thread info in isolation. But that might take some time. I don't think it is worth doing now.

Add a function to the read status spec

lib/shared/updates/update-thread-read-status-spec.js
117 ↗(On Diff #43492)

We need this for Flow

Having threadInfo as a parameter doesn't make sense because in order to call this function we would need to know which part of the update represents the thread ID, which defeats the purpose of encapsulating this logic inside the spec.

In D13057#370009, @inka wrote:

One thing we could do here is add a separate update type for unread status. This way the logic from D13059 that needs getUpdatedThreadInfo would not be needed, and so getUpdatedThreadInfo would not be needed. But this might be too much work.

Yeah, I'm still not sure if it is a good idea. This change would greatly simplify this logic, but has some downsides: introducing a whole new update type would require implementing its spec, which contains e.g. keyserver db layer - and we don't need that, but skipping it would also be weird. If we introduce this new update type, we will need to answer a question as to why we don't introduce similar updates for other props of the thread info... and it seems like we should do that - introduce a new update info that allows updating any property of thread info in isolation. But that might take some time. I don't think it is worth doing now.

I'm confused about this discussion... don't we already have updateTypes.UPDATE_THREAD_READ_STATUS?

In D13057#370009, @inka wrote:

One thing we could do here is add a separate update type for unread status. This way the logic from D13059 that needs getUpdatedThreadInfo would not be needed, and so getUpdatedThreadInfo would not be needed. But this might be too much work.

Yeah, I'm still not sure if it is a good idea. This change would greatly simplify this logic, but has some downsides: introducing a whole new update type would require implementing its spec, which contains e.g. keyserver db layer - and we don't need that, but skipping it would also be weird. If we introduce this new update type, we will need to answer a question as to why we don't introduce similar updates for other props of the thread info... and it seems like we should do that - introduce a new update info that allows updating any property of thread info in isolation. But that might take some time. I don't think it is worth doing now.

I'm confused about this discussion... don't we already have updateTypes.UPDATE_THREAD_READ_STATUS?

Yes, we have. But this discussion was about introducing a new update for the replies count - which was the goal of this whole diff stack.

I think this comes from the comment

One thing we could do here is add a separate update type for unread status.

Which contains a mistake. The new update, we were considering, should be for the replies count.