narrow: Convert typing-status code to all user IDs#4368
Merged
chrisbobbe merged 10 commits intozulip:masterfrom Dec 31, 2020
Merged
narrow: Convert typing-status code to all user IDs#4368chrisbobbe merged 10 commits intozulip:masterfrom
chrisbobbe merged 10 commits intozulip:masterfrom
Conversation
Sadly the `--no-git-tag-version` option, which we use to disable NPM's unconfigurable choice of Git tag name, also disables making a commit.
This makes these functions a more specific type -- it adds an additional promise they make to their callers -- so it's compatible with the existing types of the typing_status library. The improved types we'll take for that library shortly will require this promise, so that they can in turn make the same promise to the caller of `typing_status.update`, a few lines below in this file.
This brings: * A bunch more shared code, which we don't yet use: on emoji, typeahead/autocomplete, and Markdown. * Switching from underscore to lodash. * Some refinements to the typing_status types, which will in particular let us pass a read-only array of recipients.
This adds a TODO comment which we'll resolve in the next commit. The underlying issue there had been here all along, but was only made apparent by removing the mess related to converting from data about emails to user IDs.
There's no need to re-sort this list -- but that's true only because the list is already sorted, in the same way we want to sort it for this key. So, add a helper function that takes advantage of that fact, and put it within `recipient.js` which is the module where that sort of fact is encapsulated.
So that the reducer and selector are computing these keys in a way that clearly matches up. Also add an implementation comment that explains why they actually *do* match up. This version is very straightforwardly doing the same thing as the old code... but it's not so immediately obvious that it does the same as `pmTypingKeyFromPmKeyIds`, or that the old code was doing the same thing in the reducer vs. in the selector.
Next we'll make this more specific about the actual operands expected for the different operators.
Also add the `group-pm-with` operand, and record that `near` and `id` really want a number. (We don't currently use those, so this doesn't require any changes elsewhere.)
chrisbobbe
force-pushed
the
pr-userids
branch
from
327c01d to
302d7eb
Compare
Contributor
|
LGTM, thanks! Merged. |
gnprice
added a commit
to gnprice/zulip-mobile
that referenced
this pull request
Jan 6, 2021
This completes a major objective of the long string of refactoring that appeared in the series of PRs zulip#4330, zulip#4332, zulip#4335, zulip#4339, zulip#4342, then zulip#4346, zulip#4356, zulip#4361, zulip#4364, and most recently zulip#4368. After this change, the portion of zulip#4333 that's about PMs, emails, and user IDs -- aka the portion of zulip#3764 that's about narrows -- is complete. Still open from zulip#4333 is to convert stream and topic narrows from stream names to stream IDs. I'm hopeful that that will be simpler: (a) unlike the situation for PMs, there's just one stream mentioned at a time, so there's no question of sorting, and (b) there isn't the "include self or not" complication that's bedeviled much of our code related to PMs. In other words, stream and topic narrows don't suffer from zulip#4035.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This follows #4364 and the chain of PRs that led to that one.
It eliminates one of the two remaining places we use the emails from a PM narrow, in the typing-status code. Along the way it pulls in zulip/zulip#16987 and some other changes to the code shared from the webapp.
The last remaining consumer of the email data in a PM narrow is an important one: it's where we actually fetch messages from the server, or rather in
apiNarrowOfNarrowwhere we prepare theApiNarrowvalue we'll use to do so. As documented in the tip commit of this PR, the server only started accepting IDs there in Zulip Server 2.1. So we can't eliminate that code, or the code that stores emails intoNarrowvalues in the first place, until we drop support for server versions 2.0.x and older.Zulip Server 2.1 was released a little over a year ago, so it's possible that that's already something that would be reasonable to do. I want to make that decision after getting some data, though, about how many of our users are still on an older server version -- I believe the app currently works with a server as old as 1.8.x, maybe older. We'll get that data in Sentry once the next release is out, containing #4326; it's past time for a release anyway, so that should be soon.
In the meantime (and particularly in case the answer turns out to be that a lot of users are still on older servers), I plan to do some small refactoring so that all the other code that consumes narrows can be completely unaware that they contain emails, other than the few bits that still have to touch them.