api: Start relying more on non-ancient servers #5100
Conversation
We use |
Good to know, thanks. For greps indeed I tend to gloss over the paren by writing a dot: and that works well. If there were something it sweeps up that isn't this syntax, then it's probably a malformed version of it that I wanted to find anyway. I use the parens syntax because it's what I see some other projects doing; I think it was popularized as a Googleism. Compared to the slash idea, it's a bit more annoying to type, but OTOH it lends itself well to contents that themselves contain a slash (like the URL of an issue thread, when it's not in our own tracker.) I do think having a specific version reference is a helpful element for the compatibility flavor of these TODO comments. That's what makes it possible to focus on the really old compatibility items that are already actionable, while ignoring the ones that are recent and addressed to the future. |
| @@ -30,10 +30,11 @@ export type InitialDataBase = $ReadOnly<{| | |||
| // `fetch_event_types` and remove this comment. | |||
|
|
|||
| /** | |||
| * Added in server version 2.2, feature level 1. | |||
| * Added in server version 3.0, feature level 1. | |||
There was a problem hiding this comment.
Eep, thanks for catching this!
There was a problem hiding this comment.
Yeah, to be clear, this wasn't wrong, just outdated -- 3.0 is the name we used for the next major release after 2.1, what was originally planned as 2.2. (That's the point at which we shortened the version numbers, following the industry-wide trend of recent years.)
| @@ -713,8 +713,8 @@ export const action = Object.freeze({ | |||
| anchor: 0, | |||
| numBefore: 50, | |||
| numAfter: 50, | |||
| foundNewest: undefined, | |||
| foundOldest: undefined, | |||
| foundNewest: false, | |||
There was a problem hiding this comment.
[…] We may at some point want to have the app
outright refuse to operate on sufficiently ancient servers, where
it's just not going to be a good experience. […]
I've filed #5102 for this task.
We've recently started to actually drop old conditionals that added complexity and were needed for supporting old versions of the server. To do more of that, it's good to have a systematic format we use when marking that something depends on the server version, so that as we desupport an old version we can come sweep through and find all the new opportunities to simplify. We've generally tried to make these references reasonably clear, but hadn't had a concise form to use consistently. A year ago in e47aa00, I introduced a form of comments like this: // TODO(server-2.0): switch to numeric user IDs, not emails. and I think that works well. Sweep through now, using a variety of less-precise search patterns to try to find existing comments of this kind, and convert them all to that standard form. Many of these refer to server versions we already don't support -- with ServerCompatBanner we tell users their server is unsupported if it's older than 2.1. So a lot of these are opportunities we can already take. But we'll leave those for separate commits. Also make small related comment fixes: * The projected "2.2" was renamed 3.0 before release. * Drop a TODO that isn't a TODO.
In particular that caveat in the implementation comments should really be noted as part of the interface, because it's something to be aware of when adding a new callsite.
I added a new selector here in 172764c, just after making it the case (in 31e5da3) that this information is always present when we have server data. I was focusing on the feature level, rather than the version number, and apparently didn't notice that there was already an existing selector for the version number. So I had the new selector use that new invariant, but the existing selector's existing callers didn't get the benefit of it. Fix that, and simplify the callsites accordingly. All three of them already assume we have server data. We consolidate on the name getServerVersion, rather than getZulipVersion, because the mobile app itself is of course also "Zulip" and has its own version number. The name "Zulip version" appears in the server's own API because in that context it makes sense, and it's leaked into parts of our codebase from there, but we can refrain from propagating it further.
Servers that sent this property are now long obsolete -- 1.6.0 was released 2017-06-06, well over four years ago -- so there's not likely any debugging value in even keeping it around commented out. This appears to be our only TODO(server-1.6). We're now fully upgraded to take advantage of the Zulip Server 1.6.0 release!
Huzzah! Look at that negative diffstat. Also make a note next to where we record `minSupportedVersion`, in ServerCompatBanner.js. We may at some point want to have the app outright refuse to operate on sufficiently ancient servers, where it's just not going to be a good experience. Until then, it seems helpful to track what we know about what the consequences are likely to be (though actually *testing* on such ancient servers would be significantly more effort and not worth it.)
gnprice
force-pushed
the
pr-server-upgrades
branch
from
4305e3b to
401b3a2
Compare
The change here that caused me to look into this today was this one, after #5052 (comment) :
024853c caught-up: Drop legacy inference for servers pre-1.8
That turns out to delete for us a surprising amount of code! Especially in tests -- that was some subtle logic.
But then I also went and did a sweep to help lay the groundwork for other such changes, to make more use than we do of the assumption that the server is not ancient:
a0d86af api [nfc]: Use the greppable
TODO(server-N.M)form consistently.We've recently started to actually drop old conditionals that added
complexity and were needed for supporting old versions of the server.
To do more of that, it's good to have a systematic format we use when
marking that something depends on the server version, so that as we
desupport an old version we can come sweep through and find all the
new opportunities to simplify.
We've generally tried to make these references reasonably clear, but
hadn't had a concise form to use consistently. A year ago in
e47aa00, I introduced a form of comments like this:
// TODO(server-2.0): switch to numeric user IDs, not emails.
and I think that works well. Sweep through now, using a variety of
less-precise search patterns to try to find existing comments of this
kind, and convert them all to that standard form.
Many of these refer to server versions we already don't support --
with ServerCompatBanner we tell users their server is unsupported if
it's older than 2.1. So a lot of these are opportunities we can
already take. But we'll leave those for separate commits.
We don't need to rush to sweep through all of those cleanup opportunities, particularly as some are more work than others, some (like dropping that legacy caught-up inference, using the "new"
found_oldest/found_newestproperties) have more of a benefit to us than others, and some are more likely to break things for users than others. E.g., that caught-up inference is pretty high on the scale of likely breakage to users… to the extent users are on that version, which is very little.But I may do a sweep through these soon, to do the easier and/or higher-value items and triage the rest.