Clean up "actionCreator" and part of typing-status expiry#4227
Clean up "actionCreator" and part of typing-status expiry#4227chrisbobbe merged 4 commits intozulip:masterfrom
Conversation
|
|
||
| /** Start the typing-status expiry loop, if there isn't one already. */ | ||
| export const ensureTypingStatusExpiryLoop = () => async (dispatch: Dispatch, getState: GetState) => { | ||
| export const ensureTypingStatusExpiryLoop = () => async ( |
There was a problem hiding this comment.
To me, this line-break change suggests that you're also affected by the auto-formatting issue I mentioned here, hmm. IIUC this should have been done in the "ensureTypingStatusExpiryLoop" rename commit.
There was a problem hiding this comment.
Ah indeed, thanks. Pushed a fix.
gnprice
force-pushed
the
pr-expire-typing
branch
from
0bfa842 to
1846019
Compare
|
Thanks! This looks good to me; looks like you fixed that one formatting nit. Did you find any others, though? Currently I've been running |
The existing name was very confusing because it sounded like something that would clear the typing indicators/notifications *now*, not like something that would start a loop to repeatedly check back in the future to expire them as appropriate. Confusing not only in potential but in actual confusion; see: zulip#4216 (comment) https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Typing-state.20client.20timer/near/986619 This also makes a good time to fold it into a module with a more general name, so that the filename isn't tied to the function's name.
The condition about whether there are any existing typing-status indicators is kind of mysterious when read literally, and even seems backward: we only start trying to clear them if there are *none*? The point of it is that that's also the break condition for the loop that otherwise sits around continuously, looking for expirable typing statuses and expiring them. That's an implementation detail of that loop, and kind of a subtle one. Move it to sit next to the loop's own implementation.
The one side effect we currently do here is indifferent to whether it happens before or after we dispatch the event actions. But we have another side effect found in the oddly-named `actionCreator` function, which we'd like to fold into this mechanism, and for that one it's essential that it happens before the events are dispatched. So, reorder to accommodate that. If in the future we have some other side effect that needs to happen after dispatch, and still have this one or another that needs to happen before, then we can have both kinds separately.
This isn't much of an "action creator" at all. In reality it has a very different job -- one we have another mechanism for with a much better name, doEventActionSideEffects. So we can make things clearer by folding its logic into that one.
chrisbobbe
force-pushed
the
pr-expire-typing
branch
from
1846019 to
37c094b
Compare
|
Merged, thanks, @gnprice! (I ran There's a planned followup to this, currently at your
Does that mean we should hold off on the followup until we do that reorganization? |
2 participants
This follows up on #4216 to clean out the very odd
actionCreatorfunction and a bit of the logic around expiring typing status.This doesn't include fixes for all the things that are off about how we expire typing status -- notably the loop at a fixed 15s period, which means that a status might wait as long as 30s before we get around to expiring it. But it should at least isolate all the oddness to specifically the typing-status code, and get it out of the way of the general event-handling code.