action types: Sync update-message event type with doc #5155
Conversation
chrisbobbe
added
a-data-sync
|
See two currently unresolved questions at https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60update_message.60.20event/near/1292941 . |
| // TODO is it really right that just one of the orig_* is optional? | ||
| orig_content: string, | ||
|
|
||
| // TODO: The doc for this field isn't yet correct; it turns out that |
There was a problem hiding this comment.
The second TODO is resolved with Tim's clarifications in in
8295bb918.
Let's clarify what repo that commit ID is in, like zulip/zulip@8295bb918.
src/actionTypes.js
Outdated
| // This is current to feature level 109. | ||
| type EventUpdateMessageAction = {| |
There was a problem hiding this comment.
This is probably a good occasion to add a link to the API doc. (Which didn't exist when this type was first written down.)
src/actionTypes.js
Outdated
| |}; | ||
| type TopicLinks = $ReadOnlyArray<{| +text: string, +url: string |}> | $ReadOnlyArray<string>; | ||
| // This is current to feature level 109. | ||
| type EventUpdateMessageAction = {| |
There was a problem hiding this comment.
nit: blank line between these definitions
(Not sure why there wasn't one between the existing two; seems like an oversight.)
src/actionTypes.js
Outdated
| // TODO(server-3.0): Added in feat. 46, replacing subject_links | ||
| // TODO(server-4.0): Changed in feat. 1 to array-of-objects shape, from string[] | ||
| topic_links?: TopicLinks, | ||
|
|
||
| // TODO(server-3.0): Replaced in feat. 46 by topic_links | ||
| // TODO(server-4.0): Changed in feat. 1 to array-of-objects shape, from string[] | ||
| subject_links?: TopicLinks, |
There was a problem hiding this comment.
I think the feature levels here are swapped.
Also, as I read the doc, subject_links stops existing after the rename; it's not simultaneously present e.g. for compatibility. So when present, it's always the old form.
src/actionTypes.js
Outdated
|
|
||
| // TODO(server-3.0): Added in feat. 46, replacing subject_links |
There was a problem hiding this comment.
I think this TODO can be dropped, because there's not actually anything to do at this spot when dropping support for these old servers: the field is optional even in the latest version.
|
Oh, and on the partial version of this that's in #4980 I see there's one other comment that's relevant here: #4980 (comment) (Plus #4980 (comment) , which I covered above but more tersely.) |
chrisbobbe
force-pushed
the
pr-update-message-action-type-sync
branch
from
102c0f0 to
772fc52
Compare
|
Thanks for the review, revision pushed! Please note this before merge:
|
|
Confirmed in chat that we should expect |
For the first TODO, see discussion at https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60update_message.60.20event/near/1292747 The second TODO is resolved with Tim's clarifications in zulip/zulip@8295bb918.
For how we got the possible values of `propagate_mode`, see https://chat.zulip.org/#narrow/stream/378-api-design/topic/openapi.3A.20.60enum.60.20not.20rendering/near/1292921 .
gnprice
force-pushed
the
pr-update-message-action-type-sync
branch
from
772fc52 to
4ce9e20
Compare
|
Thanks! This looks good -- merging. Those two unresolved questions look to not affect these types, because they're both about the conditions under which a particular optional field is or isn't present; the fields are optional regardless. |
This should help with #4840. Inheriting P1 from that issue.