unread [nfc]: Add getUnreadCountForTopic#4805
Conversation
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! This LGTM overall; see one small question below.
|
|
||
| /** The total number of unreads in the given topic. */ | ||
| export const getUnreadCountForTopic = ( | ||
| unread: UnreadState, |
There was a problem hiding this comment.
Is it fine for a selector to take a substate (like UnreadState) as the input state, or should all selectors start with GlobalState?
There was a problem hiding this comment.
Yeah, perhaps calling this a "selector" is confusing and I should label it something else. Maybe "getter"?
The reason I want it taking just the unread state is so that we can conveniently use it in a context like #4635, in code where we have a BackgroundData style of object but don't have the whole Redux state.
More generally, a direction I'm interested in taking our data-model code is toward having each subsystem's model be more of a self-contained thing with its own identity and interface, vs. the status quo which kind of tries (but inconsistently so, because this doesn't really work if you try to push it hard) to treat them all as undifferentiated implementation details of the global state. For example, we'd have the whole "unreads" model as one such subsystem. So having a getter like this directly on that model's state is a small experiment in that direction.
There was a problem hiding this comment.
I think the name "selector" is probably fine, and the use case makes sense. Thanks for explaining, and I'm excited to see progress in getting our data-model code structured more sensibly. 🙂
We'd call both rootReducer and messagesReducer "reducers", and I think it's fine to call both this function and those that take GlobalState "selectors".
This will let us start passing around the unread state as a whole, instead of its various pieces like the part for streams or 1:1 PMs. That in turn will help us start to better encapsulate the specifics of the unread state's data structures, which will make it easier to modify them.
This will let us get this information in a convenient way from code that's entirely outside of the unreads code -- like this call site, in `getTopicsForStream` which is a UI-oriented selector that exists to drive the topics-list screen -- without having to dig into the details of the data structures we use for the unreads data. It's possible to get a similar effect already by invoking `getUnreadCountForNarrow`. But (a) that interferes with the caching on that selector, which is really meant to be used only for the one current narrow that's foregrounded in the UI; and (b) that requires an entire `GlobalState` object, whereas we'd like to be able to get this effect from code that has only some relevant pieces of the state, in the spirit of `BackgroundData`. Because we generally say "selector" to mean something that requires the global state as its input, we file this function under the more general term "getter".
This is a bit cleaner anyway than using three different selectors for different pieces; plus it'll let us use the new, encapsulated `getUnreadCountForTopic`.
Now this spot where we look into the details of the unreads data structure is encapsulated inside the unreads model.
|
Thanks for the review! Revision pushed. |
|
Great, thanks! Merged. |
2 participants
This is a quick refactoring inspired by #4635 (comment) .
There's a number of useful things we could do further in this direction, but stopping here for now in order to get this out.