narrow: Store stream IDs instead of names!#5205
Merged
chrisbobbe merged 24 commits intozulip:mainfrom Feb 1, 2022
Merged
Conversation
Member
Author
|
Marking temporarily as draft while I fix #5206 -- there will probably be a rebase conflict, and I want to prioritize getting the regression fixed. Then I'll rebase this atop that fix, and also reread this to look for anyplace that it might introduce a similar regression. |
When you need these, you need them. It doesn't seem helpful to have to add a local eslint-disable line; it's not like these look confusingly like something else.
Using snake_case to make it blend in with the other properties on this JSON blob, which come from the server.
In a notification for a stream message, the server doesn't send us the stream's ID, only its name. We'll soon want to use stream IDs in our Narrow values, which means we'll need the stream's ID up front when the user opens such a notification. So, start looking up the stream in our data structures at this point. There's an open issue for the server to start sending stream IDs here: zulip/zulip#18067 When it does, we can start looking for that and using it when present, but we'll want to keep this lookup as a fallback for a while after. This change causes getNarrowFromNotificationData to start returning null if the notification is for a stream we don't know about, where previously it would go ahead and return a narrow. The old behavior is that we'd navigate to that narrow, and it would show an error message "That conversation doesn't seem to exist" (via InvalidNarrow). The new behavior is that we just won't act on the notification at all. That's less good, but tolerable, and what we already do for 1:1 PMs; leave a TODO comment explaining it.
Conveniently, this code turns out to already be well-typed. The type-checker will help us keep it that way as things change.
This just takes each of the Subscription objects we have, and spreads the corresponding Stream object on top of them. But the way these objects work in the server API is that a Subscription object already contains all the properties of the corresponding Stream in the first place. So the end result should have exactly the same contents as the Subscription object we started from. When this selector was introduced in 27483c9 back in 2017, the motivation was apparently "because streams change events are updated in streams" -- i.e., because we were *not* keeping the Subscription objects up to date when things changed on the stream. But in that case the real bug was that we were keeping around an object with out-of-date data; having only an alternative wrapper off to the side where one can get an up-to-date version is a recipe for an ongoing stream of bugs where we use the version that's in the obvious place, not realizing it can be stale. Fortunately that did get corrected the next year, in 152b06e. Since then, the Subscription objects are just as up to date as the Stream objects, and this selector has no reason to exist. Drop it, and have its callers get the subscription data directly.
Plus some trivial other refactors. Together these will help make the diff where the Narrow constructors start taking stream IDs along with names, coming soon, be as boring and obvious as possible in most of its length.
This will let us make the boring mass change of starting to pass these stream IDs, separately from the more interesting changes involved in actually storing and using them.
This is all made possible by the preceding series of changes where we parsed, tracked, or otherwise made stream IDs available in a variety of places where we'd previously had only a stream name. This leaves just a handful of call sites not passing IDs -- in particular, where the stream we want to refer to comes itself from some Narrow value. Those we'll take care of together with actually storing the stream ID in the Narrow.
In the migration tests, it is kind of messy that we have to update a previous migration's test. That happens because we're running all the migrations to completion. As the comment at the "whole sequence" test says about a related point: this is probably not a great design for continuing to add more migrations in this sequence. But pretty soon we're going to cap it off and write future migrations in a new framework, which will come with its own way of writing tests. So for the moment just live with it. Fixes-partly: zulip#4333
If we actually make it to either of these screens (TopicListScreen or StreamSettingsScreen) with an unknown stream, they'll try to look up our data on the stream and will promptly crash. We're allowing this ChatNavBar component to tolerate unknown streams, because one can sometimes navigate to a narrow in such a stream and we try to represent that as a proper screen: ChatScreen will show InvalidNarrow in that case instead of MessageList, and will show a ChatNavBar as usual. But then let's draw the line at navigating to any other screens. This one already delivers the message that there's nothing here; no need to have other screens handle this case just to deliver the same message. At the moment, these buttons don't do anything in this case anyway: if the stream is unknown, then the callback won't find the stream when it looks it up, and will silently do nothing. But in our migration toward stream IDs, we'll shortly have these callbacks navigate directly by stream ID rather than have to look up the name in our streams data. Without this change, that would result in navigating to the destination screen with an invalid stream.
In almost all the places we were using streamNameOfNarrow, what we really want is just the stream ID, not its name; or an actual Stream or Subscription object. Now that the narrows contain the stream ID, we can get it directly, and look up the Stream or Subscription by that instead of by name. The only exceptions are where we construct new Narrows from old ones, either to go from a stream narrow to a topic narrow or vice versa. After this commit, the only call sites of streamNameForNarrow are those and its own tests. This change should be NFC except in cases where the stream name and ID don't agree -- e.g., because the stream has been renamed. In those cases, it fixes a bug.
We stopped needing this a little while ago, in 8b33321, after we started having stream IDs on StreamOutbox as well as StreamMessage.
This allows these callbacks to not have to change around (and also to not become misleading in their parameters' names) as we rearrange what arguments are given to these callbacks, as part of replacing stream names with stream IDs.
This covers the spots where this conversion is local and trivial. The change should be NFC except in cases where the stream name and ID don't agree (e.g., because the stream's name has been changed since we formed this Narrow object). In those cases, it fixes a bug. To find these, and the other call sites in the next few commits, I manually audited through all calls of the `caseNarrow*` functions. Ideally for this sort of refactor one would like the type-checker to do that work, but sadly here it isn't able to completely do so: in getComposeInputPlaceholder we pass the callback's argument to string interpolation, and in two other places we used it only for `===`. (If there were a `JSON.stringify`, that'd have the same problem.) And those accept numeric IDs just the same as string names.
We still need a stream name here for the actual request to the server, until we start requiring server 2.1. But we can look up the name by ID in the overall state, just as we do for PM narrows. In particular this means that we'll now behave correctly if the stream got renamed since we formed this Narrow object (although until we push IDs all the way through to the server, there'll still be a race if the stream gets renamed concurrently with our request.)
This is nearly NFC, because we've already eliminated all the places that we actually consumed these stream names... other than in `keyFromNarrow`. Those keys are used not only to reconstitute Narrow objects (in `parseNarrow`) but also to uniquely identify a narrow in our data structures. So there is a subtle bug this fixes: if a stream gets renamed, then before this change we could get duplicate data for it, and conversely could fail to find an old draft. We leave a few things to clean up later in larger, boring, NFC commits, in order to keep this interesting change tightly focused: * The constructors streamNarrow and topicNarrow still accept a "stream name" argument, but they ignore it and allow it to be void. * The streamNameOfNarrow function still exists, but returns void. All its call sites have already been eliminated, other than for passing the result directly back to a Narrow constructor. * The `caseNarrow*` functions still pass a "stream name" argument to the `stream` and `topic` callbacks, but it's always void. All callers have already switched to ignoring that argument in favor of the stream ID, other than the few we switch over in this commit. Fixes: zulip#4333
We cover all the same call sites that were switched over to using stream IDs in the preceding few commits, which followed a manual audit of all `caseNarrow*` call sites.
After editing the constructor definitions themselves,
the call sites were updated mostly automatically:
$ perl -i -0pe '
s/\b (streamNarrow|topicNarrow) \( \K .*? , \s*//xsg
' src/**/*.js
$ tools/fmt
with manual fixup just in internalLinks.js, plus deleting a
handful of unused variables pointed out by lint.
This makes its interface a bit simpler, and gets rid of the stream name there.
These were each needed only because of a Narrow constructor, and those no longer take a stream name. This isn't yet enough to let us stop passing stream names *to* these callbacks, because each of these has another use site where the callback does still use the stream name. We'll get to those separately later.
gnprice
force-pushed
the
pr-streamid-narrow
branch
from
8f77dd8 to
97e86e6
Compare
Member
Author
|
OK, done. The changes prompted by thinking about #5206 are: 9c063e9 notif [nfc]: Say
|
This was referenced Jan 29, 2022
Member
Author
|
OK, and filed #5210 (and three other issues, with that as an umbrella issue) for those bugs in the invalid-stream case of ChatScreen. |
Contributor
Member
Author
Ha, yes, thanks -- that one. They're next to each other in the list, and I must have copied the wrong line. |
chrisbobbe
reviewed
Feb 1, 2022
Contributor
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Great, all LGTM, including the changes you highlighted at #5205 (comment). Thanks! Merged.
| }, | ||
| ); | ||
|
|
||
| export const getSubscribedStreams: Selector<$ReadOnlyArray<Subscription>> = createSelector( |
Contributor
There was a problem hiding this comment.
stream [nfc]: Delete redundant, inefficient getSubscribedStreams
Whew! 😂 Thanks for investigating and doing this!
| Object.freeze({ type: 'topic', streamName: stream, topic }); | ||
| export const topicNarrow = (streamName: string, ...args: [string] | [number, string]): Narrow => | ||
| // $FlowFixMe[incompatible-cast] | ||
| Object.freeze({ type: 'topic', streamName, topic: (args[args.length - 1]: string) }); |
Contributor
There was a problem hiding this comment.
Interesting 😅—this stuff with args.length (and the [number, string] tuple type) seems kinda awkward if we were going to leave it here. But I understand it's temporary; it goes away later in the branch.
Member
Author
There was a problem hiding this comment.
Yeah, exactly.
If this were going to last longer -- if this were going to be a longer transition -- then I'd want to take a strategy that would be somewhat more overhead but keep the intermediate states more robust. In particular I'd probably have two different constructors for the two different signatures as long as they coexisted -- so like:
- Rename
topicNarrowto liketopicNarrowOld, ortopicNarrowNoId. - Add new
topicNarrowwith the new signature. (Or even: leave the old name nonexistent, so other branches rebased across the change would definitely get a useful error.) - Go converting things from using the old
topicNarrowNoIdto using the newtopicNarrowWithId. This might go on for a while. - Eventually, delete the old thing.
- Eventually, rename the new thing to the simple name (if it didn't get it at step 2.)
That's the kind of approach that can scale to a larger codebase with lots of different people working in it, or a codebase that isn't all versioned together in one repo. And I've used it for some changes even in zulip-mobile, where the conversions in step 3 are tangly. But it's more work and churn and I was glad this change didn't end up needing it.
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 does for streams and stream IDs what #4346 and #4382 did for users and user IDs: in
Narrowvalues, we stop using stream names and switch to stream IDs instead.This builds on #5183 and #5069. It completes #4333, and accounts for most of the remaining cases of #3918.
Fixes: #4333