It's not clear to me why this code was necessary,
and I assume it was either originally written
with a bit of misunderstanding of how zjquery
works or it became unnecessary with some refactoring
of the "real" code.
We move some of the data setup to the top of the file.
We also remove some get_sub() calls that aren't really
necessary now that peer_data and stream_data are more
independent.
The maybe_clear_subscribers() function was an artifact of
when we used to attach subscribers to the "sub" records in
stream_data.js. I think it was basically a refactoring
shim, and due to some other recent cleanup, it was only
used in test code.
We also change how we validate stream ids.
Going forward, peer_data just looks up stream_ids with the
normal stream_data API when it's trying to warn about
rogue stream_ids coming in. As I alluded to in an earlier
commit, some of the warning code here might be overly
defensive, but at least it's pretty self-contained.
We now use the same code in all places to
get the bucket of user_ids that correspond
to a stream, and we consistently treat
a stream as having zero subscribers, not
an undefined number of subscribers, in
the hypothetical case of us asking about
a stream that we're not tracking.
The behavior for untracked streams has
always been problematic, since if a
stream is untracked, all bets are off.
So now if we don't "track" the stream,
the subscriber count is zero. None of
our callers distinguish between undefined
and zero.
And we just consider the stream to be subscribed
by a user when add_subscriber is called,
even if we haven't been told by stream_data
to track the stream. (We also stop
returning true/false from add_subscriber,
since only test code was looking at it.)
We protect against the most likely source
of internal-to-the-frontend bugs by adding
the assert_number() call.
We generally have to assume that the server
is sending us sensible data at page load
time, or all bets are off.
And we have good protections in place
for unknown ids in our dispatch code
for peer_add/peer_remove events.
We also streamline some of the error handling code
by doing everything up front. This will prevent
scenarios where a single bad stream_id/user_id causes a
bunch of the same warnings in an inner loop.
This removes a bit of complexity. If a piece of
settings code needs to render a stream with
subscribers, it just asks for it.
We no longer have the brittle, action-at-a-distance
mechanism of mutating the subscriber count on to
the stream_data version of a sub.
Stream subs are pretty small, so making copies of
them is cheap, and the blueslip timings from the
previous commit can help confirm that.
There is some discussion of putting `subscriber_count`
on the Stream model, which may eventually get us
away from tracking it in `peer_data.js`, but we will
cross that bridge when we get there. See
https://github.com/zulip/zulip/issues/17101 for
more details.
The weekly stream traffic is a better tiebreaker
for stream typeaheads than subscriber count, as
it's more directly a measure of a stream's current
relevance.
Normally stream traffic and subscriber counts are
closely correlated, but a good example for me is
the #twitter feed on czo, which only has 80 subscribers,
but which gets more traffic than our #integrations
stream (with 16k subscribers). I would rather
see #twitter win the tiebreaker (if it even got
to the tiebreaker).
The main motivation behind this fix, though, is
to break our dependency on peer_data, which has
some upcoming changes that will introduce some
performance tradeoffs, and I want one less place
to audit.
Also, it will be easier long term to share this
code with mobile if we don't require mobile
to pull in our peer_data dependency. (The webapp
has different forces than mobile that dicate
our data structures.)
We use day_old calculated based on day instead of hours to
render last seen values. This fixes us incorrectly quoting
anything 24 - 48 hours ago as Yesterday and
incorrectly quoting `time` that are Yesterday
but < 24 hours ago in 'x hours ago' format.
We were adding `expanded` class to left-sidebar when searching
for streams even if the left-sidebar was not in the popover state.
This cased confusion with popovers.any_active returning true,
when actually it is not.
topic_generator previously included an entire lazy generator
combinator library that was used four times. These straightforward
equivalent loops might not be as fun but they are way simpler.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
After this change all peer_data functions consistently
use stream_id rather than some "sub" object whose
data type is complicated by all sort of fields that
don't really concern how we track subscribers.
The goal here is to make all our peer_data functions
basically work in id space. Passing a full `sub`
to these functions is a legacy of when subscriber
info was attached to a full stream "sub" object,
but we don't care about anything sub-related
(color, description, name, etc.) when we are
dealing with subscriptions.
When callers pass in stream_id, you can be more
confident in a quick skim of the code that we're
not mutating anything in the "sub".
This de-clutters stream_data a bit. Since our
peer data is our biggest performance concern,
I want to contain any optimizations to a fairly
well-focused module.
The name `peer_data` is a bit of a compromise,
since we already have `subs.js` and we use
`sub` as a variable name for stream records
throughout our code, but it's consistent with
our event nomenclature (peer/add, peer/remove)
and it's short while still being fairly easy
to find with grep.
This sets us up to use better system-wide data structures
for tracking subscribers.
Basically, instead of storing subscriber data on the
"sub" objects in stream_data.js, we instead have a
parallel data structure called stream_subscribers.
We also have stream_create, stream_edit, and friends
use helper functions rather than accessing
sub.subscribers directly.
Anders Kaseorg
Steve Howell
Ganesh Pawar
Rohitt Vashishtha
Aman Agrawal