messagesReducer: Address a performance TODO for EVENT_NEW_MESSAGE.#4409
messagesReducer: Address a performance TODO for EVENT_NEW_MESSAGE.#4409gnprice merged 4 commits intozulip:masterfrom
Conversation
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks! Small comments below.
| // `narrowsForMessage` will always have at least length 1 because of | ||
| // the home narrow. |
There was a problem hiding this comment.
Why is this fact needed?
(I think the minimum possible length is actually 3, fwiw: either home, stream, and topic, or home, all-PMs, and PM thread.)
There was a problem hiding this comment.
Ah, I suppose that could be removed. I found myself wondering what Array.prototype.some does with an empty array (turns out it returns false for any condition), and I thought it might be helpful to the next reader if they knew they didn't have to wonder about it.
There was a problem hiding this comment.
Got it. I think it's best to see that behavior as the only way that some could reasonably work:
- Its job is to return true just if there's some element that the condition is true for.
- So if there are no elements at all, then there are certainly none that the condition is true for, and the return value should be false.
Or from another angle:
- Taking away elements from an array should only ever turn the return value of
somefrom true to false, never from false to true. - So if there's any array where
somereturns false for a given condition, then taking away all its elements should leave it still false. - Moreover there was just one element the condition was true for and you take that one away, the return value should become false.
- So if you have an array where
somereturns true, and you take a bunch of elements away and you're down to just one left, then even if the return value is true at that point, it should be false after you take that last element away.
There was a problem hiding this comment.
Oh OK, makes sense! Thanks.
There was a problem hiding this comment.
I think it's best to see that behavior as the only way that
somecould reasonably work:
- Its job is to return true just if there's some element that the condition is true for.
- So if there are no elements at all, then there are certainly none that the condition is true for, and the return value should be false.
This is also basically exactly what we want to know in this piece of code, too:
- We want to add the message just if there's some narrow it belongs in where we're caught up.
- So we get an array of narrows this message belongs in, and we ask if any of them is a narrow where we're caught up.
If for some reason there were no narrows this message belongs in, then the answer would be no, there's no narrow this message belongs in where we're caught up. Which is exactly what Array#some gives us.
| // We only need to record the message if we intend to show it right | ||
| // away in any narrow, i.e., before the next MESSAGE_FETCH_COMPLETE. | ||
| // We only intend to show it right away in a narrow if we know it's | ||
| // that narrow's most recent message, i.e., we know we're currently | ||
| // looking at (caught up with) the newest messages in the narrow. |
There was a problem hiding this comment.
I think it's not quite about whether we'll "show it right away" -- we could be caught up simply if the user has previously gone and looked at that narrow, and seen the latest messages in it, and so we've stayed caught up since then.
What's really going on is basically a converse to the warning added below about making sure this contains everything that's in state.narrows: it's that we don't really have a use for anything to be in state.messages if it isn't in state.narrows. The only times we try to access a message from here are when we've found its ID under some narrow in state.narrows.
chrisbobbe
force-pushed
the
pr-immutable-messages-followup
branch
from
bd3dd8a to
0f19fef
Compare
|
Thanks for the review! Revision pushed. |
gnprice
force-pushed
the
pr-immutable-messages-followup
branch
from
0f19fef to
42bd74b
Compare
We don't expect this to happen, and a similar check has been done for the same Redux action in narrowsReducer for some time now. But we haven't yet made it quite clear to Flow that `flags` will normally be present here, and we'll want to do that for an upcoming change.
…vent. Like we did recently, in narrowsReducer, in 47d4f97. Also nudge a TODO down a few lines, closer to where it belongs.
Using the recently added `getNarrowsForMessage`. And leave bidirectional comments with narrowsReducer so that we never end up with messages in `state.narrows` that don't exist in `state.messages`.
gnprice
force-pushed
the
pr-immutable-messages-followup
branch
from
42bd74b to
d7c25bd
Compare
|
Looks good, thanks! Merged. The rebase required a small fix to replace |
Whew—thanks for catching that and doing that. |
|
You can thank Flow for catching it 😉 -- which was probably the trickier part. |
2 participants
Discovered while working on #4390, which was recently merged.
messagesReducer: Address a performance TODO for EVENT_NEW_MESSAGE.
Using the recently added
getNarrowsForMessage.And leave bidirectional comments with narrowsReducer so that we
never end up with messages in
state.narrowsthat don't exist instate.messages.