api [nfc]: Start relying on server 1.9+ #5192
Conversation
There was a problem hiding this comment.
Looks great, glad to have all this cleaned up!! Please merge at will. I have just one small comment where I noticed something interesting, and I think I understand it.
The CI failure looks to be another symptom of some jcenter operational issues, and can be ignored; discussion here.
| server: string, // settings.EXTERNAL_HOST | ||
| realm_id: number, // server-internal realm identifier | ||
| -server: string, // settings.EXTERNAL_HOST | ||
| -realm_id: number, // server-internal realm identifier |
There was a problem hiding this comment.
Interesting. The - marks these properties as write-only. We do that…not because we'd ever want to write to them (we wouldn't), right, but because we want Flow to shout at us if we try to read them (seeing as they're obsolete)?
There was a problem hiding this comment.
Right. This lets the type still document that they might be there, and it lets them appear when we construct one of these objects, like in tests, but prevents reading them.
We have another example of this on Subscription:
/** (To interpret this value, see `getIsNotificationEnabled`.) */
+push_notifications: null | boolean,
// To meaningfully interpret these, we'll need logic similar to that for
// `push_notifications`. Pending that, the `-` ensures we don't read them.
-audible_notifications: boolean,
-desktop_notifications: boolean,
|};There was a problem hiding this comment.
it lets them appear when we construct one of these objects, like in tests
Mm, right, I forgot about tests.
I guess if we have tests that use one of these objects, and we say that the object is of the ApnsPayload type, we'll have to include these write-only fields. That's because they're not marked as optional. I guess that's how we say that they're always present in the payloads we're getting…though it seems like they don't need to be present at all.
There was a problem hiding this comment.
True. It is kind of nice to have this sort of property in objects we use in test data, because it makes the test data representative of what real servers send... but when it's one that's totally obsolete like this server and realm_id, it also risks just being noise. So if it comes up, I wouldn't be sad to just comment them out, like we do for Message#recipient_id:
/** Deprecated; a server implementation detail not useful in a client. */
// recipient_id: number,Commenting it out makes our test data less representative, so that effectively we're trusting that our code doesn't get confused by an unexpected extra property. In general we don't write code in patterns that would get confused by that -- I'm not even sure offhand what a realistic such pattern would be -- so that's acceptable.
OTOH for something like audible_notifications and desktop_notifications, they totally are something we'll want to handle in the future, and we're just marking them as unusable so one sees the comment before trying to use them. So those have a stronger reason to be present, as -, rather than deleted or commented out.
This doesn't actually end up letting us simplify the code, because of a bug fixed in Zulip 4.0; so we leave a TODO(server-4.0) for that. It does at least simplify the story in the jsdoc, though.
Moving the discussion of why this type isn't different from how it is out into an implementation comment makes it easier for the jsdoc to get to the point. Also clarify how to tell that a code is stable: check the API docs.
The one thing we were doing to accommodate it possibly being missing is a `|| []` in the reducer. Leave that in place, because it's such a small amount of code. In the future we might validate that the data we get from the server actually satisfies the expected types. But without some kind of framework for that, rejecting a result that's missing this would take more code than just falling back to `[]` as we do.
The code below this already rejects APNS messages that don't have these fields. Also simplify the reference to the server commit this description is based on.
The code below already rejects APNs messages without realm_uri. Also give the properties a bit more of a logical/semantic organization, now that their organization isn't dominated by when they were introduced.
This gives this information more room to spread out than it has embedded inside a comment. In particular, this makes it possible to use JSDoc syntax `/** … */`, like we do for other types; that won't work inside a block comment, because JS comments don't nest. Also move this information to src/api/, as like the rest of that subtree it's describing part of the Zulip server API.
We currently never look at this property.
gnprice
force-pushed
the
pr-server-1.9
branch
from
3e4bd90 to
1b03037
Compare
|
Thanks for the review! Merging. |
This follows up on #5100, eliminating the various wrinkles we had for accommodating servers older than 1.9. This comprises:
The changes turn out to be entirely to types and comments. In 401b3a2 (#5100) we removed a significant amount of actual runtime logic that was for pre-1.8 servers, but I guess for this range of server versions we've already taken care of everything like that.
Zulip Server 2.0 will be a bigger deal: in particular it introduced user IDs and stream IDs in a number of places, as alternatives to emails and stream names.