message action sheet: Prep work for showing/hiding edit/delete buttons better.

[chrisbobbe]

I set out to do a quick and easy fix for #2792 and #2793, but then I realized that it'd be best to have #3898 done for those. These are the commits that I did manage to make without that; I think changes like these are fine to make even if we don't get through all those issues right away.

[gnprice]

Thanks @chrisbobbe ! Small comments below.

[gnprice]

nit: let's not mix in one type the style with | … on later lines with the style where a branch of the union is later on the same line -- it makes it easy to miss the first branch.

Hmm, I see we have a lint rule that's forcing that. In general you can always override that with // prettier-ignore and/or /* eslint-disable-next-line */, when you find it's doing something anti-helpful like this. I'll also take a look at removing that lint rule.

[gnprice]

#5092

[chrisbobbe]

Thanks!

[gnprice]

So the existing type in this version is | (number | null) | number. That means it's the type of values which can be a number | null, and can also be a number… which is the same set of possible values as just number | null, because every number is already a valid number | null. So this type is the same type as number | null, and I'd want to write it that way.

The Flow docs on unions are here:
https://flow.org/en/docs/types/unions/
but don't go into this point. But in addition to the "set of possible values" reasoning, you can also see it by thinking through how a type like (number | null) | number interacts with the other characterization of union types (the one that's basically the real definition of what they mean):
https://flow.org/en/docs/types/unions/#toc-union-types-requires-one-in-but-all-out

[chrisbobbe]

Ah, right! So I do understand that this is the same type as number | null. 🙂

I just thought it might be helpful for readability, along with some comments, to describe the two different representations we'll get from the server (before 5.0, and at 5.0+). With the number | null branch really wanting to be number-except-0 | null, I guess. And then, since we have one representation for pre-5.0, and one for 5.0+, unsupporting pre-5.0 would mean we could cut out one of those representations concretely by removing a line of code.

But I don't much mind simplifying (if that's the right word) to number | null, and just having one comment that describes what we get before 5.0 and at 5.0+.

[gnprice]

Cool. Yeah, I think it's clearest to let the type here be in the more simplified/normalized form, and cover the specifics of the variation just in the text.

[gnprice]

nit: this isn't a TODO, because there's no need to do it either now or at some known expected time in the future.

In particular as part of #4659, we'll want to do a search for TODO(#4659) comments to find things it enables us to do, so we can do those as followups. But there won't be anything to do here.

I think simplest is to leave this bit out. If there's a future server API change that alters the semantics of this property, we'll go about handling that however we normally would. As long as server versions older than 5.0 are supported, that'll include handling both the behaviors before and after the 5.0 / 100 change (and the rest of this comment will give us notice that there is that difference to handle.)

[gnprice]

Hmm, is this documented? We generally shouldn't need to rely on the server's source code these days; if we do, that's an API-docs bug 😉

[chrisbobbe]

zulip/zulip#20131

[gnprice]

This kind of thing shouldn't be necessary. If we did rely on it, that would be a bug -- fundamentally any data like this that we have before actual server data is a bad assumption about what the server has.

Fortunately I think the value does not actually matter. It's present only in the initial state, which really represents "we have no server data". And withHaveServerDataGate should prevent us from ever using such a state in a context where we try to interpret it as real data; in particular, one of its checks is that state.realm.user_id is not undefined, which will reject state.realm having its initial state.

So the kind of value I'd like to use in the initial state for server data like this is just a maximally boring, empty-ish value of the right type. Here, 0 would be good.

That way we make it avoid looking like these values are expected to be some sort of guess as to what the real values on the server are, which could be misleading about how this subsystem works.

(Ideally this arrangement would be expressed directly in the actual types. I might attempt that after we have a per-account schema #5006, because it'll fit more naturally in that context. Pending that, I should perhaps add some comments about this state of affairs.)

[chrisbobbe]

I hesitated to pick 0 because it's not a valid value in our representation in the Redux state, which is modeled on what servers 5.0+ send, and I thought that might be confusing or cause problems.

[chrisbobbe]

Maybe 1?

[gnprice]

Sure.

[gnprice]

So our pattern here has been to describe the semantics directly like this, but I'd like to shift that: for a property like these that corresponds to something from the server API, let's instead identify what in the API it corresponds to, and link to the docs for that. When there's a difference between this property and the corresponding bit of API, then we can highlight that difference.

(I think the existing pattern was set at a time when there wasn't much API docs to speak of, so the options were to write it ourselves or do without.)

So e.g.

Suggested change

	* @prop messageContentDeleteLimitSeconds - Messages sent more than this
	* many seconds ago cannot be deleted with this organization's message
	* deletion policy; see
	* https://zulip.com/help/configure-message-editing-and-deletion.
	* A `null` value means no limit: messages can be deleted regardless of
	* how long ago they were sent. A `number` value will not be 0.
	* @prop messageContentEditLimitSeconds - Messages sent more than this many
	* seconds ago cannot be edited with this organization's message edit
	* policy; see
	* https://zulip.com/help/configure-message-editing-and-deletion.
	* @prop messageContentDeleteLimitSeconds - Corresponds to
	* realm_message_content_delete_limit_seconds in initial data.
	* We use the Zulip Server 5.0+ convention that `null` means no limit.
	* @prop messageContentEditLimitSeconds - Corresponds to
	* realm_message_content_edit_limit_seconds in initial data.

and then above all the specific properties, a link to https://zulip.com/api/register-queue for docs on the initial data.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

This looks to be one that actually doesn't correspond neatly to any one thing in the API; the type says it carries more information than the just "integer" for that one field in the API docs, and we assemble it in realmReducer from several initial-data properties like so:

        videoChatProvider: getVideoChatProvider({
          availableProviders: action.data.realm_available_video_chat_providers,
          jitsiServerUrl: action.data.jitsi_server_url,
          providerId: action.data.realm_video_chat_provider,
        }),

So this one is probably best left with the synthesized description we have here, and when the reader wants to trace it back to the server API they're best off looking for where the property is set.

[gnprice]

Cool. Yeah, I think it's clearest to let the type here be in the more simplified/normalized form, and cover the specifics of the variation just in the text.

[gnprice]

Thanks! Just one comment, above.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! Looks good -- merged.

[chrisbobbe]

Thanks!