Make `PmMessage` and `StreamMessage` types. #4506

chrisbobbe · 2021-03-03T16:12:45Z

(Removed some outdated text here as our thinking proceeded.)

There's a site in the last commit where the handling for subject isn't great, and things get worse at that same site when I try to do subject_links; it seems to panic a bit (complaining about things other than subject_links; it's like it's forgotten some things it would otherwise know) unless I remove the code that gives subject_links a new value. So I'll pause on this for today and take a fresh look tomorrow (and you could give it a go if you like 🙂).

But what do you think of this general approach, @gnprice? I think it's similar to what you proposed in CZO but it doesn't use MessageBase; in fact, that can remain unexported, which I kind of like (since it doesn't represent a whole object we'd actually pass around anywhere).

You might find it helpful to start by looking at these two consecutive commits:

5c008bc messagesReducer types: Handle Message being PmMessage | StreamMessage.
68c6742 getMessages types: Handle Message being PmMessage | StreamMessage.

src/api/modelTypes.js

src/api/messages/getMessages.js

gnprice · 2021-03-06T02:23:16Z

src/message/messagesReducer.js

          oldMessage && {
-            ...oldMessage,
+            ...(oldMessage: M),


Similarly, this cast seems not to be needed.

For this one (and likely the others in this file), I find that it's not needed in the commit that introduces it, but it is needed as of the commit that splits type between PmMessage and StreamMessage.

Huh indeed, I reproduce.

Seems like a Flow bug. But the workaround is easy, so 🤷

gnprice · 2021-03-06T02:35:02Z

src/api/messages/getMessages.js

@@ -31,11 +31,15 @@ export type ServerReaction = $ReadOnly<{|
  |}>,
 |}>;

-export type ServerMessage = $ReadOnly<{|


Let's keep the $ReadOnly here.

On ServerMessageOf, right? Or on ServerMessage?

Yep, ServerMessageOf (as you've done in the revision.) I.e. where the object-type syntax is. Then ServerMessage gets the benefit too, as it just uses ServerMessageOf.

gnprice · 2021-03-06T02:41:52Z

src/unread/unreadStreamsReducer.js

 const eventNewMessage = (state, action, globalState) => {
-  if (action.message.type !== 'stream') {
+  if (getOwnUserId(globalState) === action.message.sender_id) {
    return state;
  }

-  if (getOwnUserId(globalState) === action.message.sender_id) {
+  if (action.message.type !== 'stream') {
    return state;


- When we start putting things together to call `addItemsToStreamArray`, we need to be sure `action.message` is a stream message. We were already sure of that; there's already an early return on `action.message.type !== stream`. But Flow seems to think it's done too early, and that something in the intervening code might thwart the job it's doing. So, move that early return a bit later.

Ah, yep. Specifically what's happening is: we check action.message.type, but then we call getOwnUserId -- and that's some function that might go and do anything. So in particular it might go and mutate the same object that we're looking at as action.message. So that object had had a type of 'private', but now it might not and so it might not have a stream_id.

gnprice · 2021-03-06T02:43:53Z

src/api/modelTypes.js

   */
-  display_recipient: string | $ReadOnlyArray<PmRecipientUser>, // `string` for type stream, else PmRecipientUser[]


src/api/modelTypes.js

gnprice · 2021-03-06T02:52:36Z

Thanks @chrisbobbe ! Small comments above, but generally this looks great.

I see also that the last commit is a draft, and there's one more property subject_links which presumably you plan to move before completing this. LMK if there are further questions I can answer to help out.

chrisbobbe · 2021-03-06T02:55:36Z

Thanks for the review!

Yeah, I'd actually appreciate it if you could try splitting subject_links and see what you might do about the resulting Flow error; I posted a few thoughts in the description. I also don't love the solution I landed on for subject; it involves a $FlowFixMe.

…sage`. In fact, just leave it out of `PmMessage` because it's absent on PMs, as the comment says. Flow gives us one substantive bit of feedback and one kind of false alarm: - We shouldn't be putting a `stream_id` on PMs in exampleData; so, stop doing that. - When we start putting things together to call `addItemsToStreamArray`, we need to be sure `action.message` is a stream message. We were already sure of that; there's already an early return on `action.message.type !== stream`. But that early return needs to be just a bit later in order to satisfy Flow that `action.message` can't be a stream message. In particular, as Greg explains [1], """ Specifically what's happening is: we check `action.message.type`, but then we call `getOwnUserId` -- and that's some function that might go and do anything. So in particular it might go and mutate the same object that we're looking at as `action.message`. So that object *had* had a `type` of `'private'`, but now it might not and so it might not have a `stream_id`. """ [1] zulip#4506 (comment) In particular, Flow sees the `getOwnUserId` call, and that's some function that might go and do anything -- including But Flow seems to think it's done too early, and that something in the intervening code might thwart the job it's doing. So, move that early return a bit later.

chrisbobbe · 2021-03-10T01:51:48Z

I've just pushed a new revision, and responded to your comments above; please see some queries at #4506 (comment) and #4506 (comment). 🙂

I haven't yet found the right thing to do for subject and subject_links; I'm trying something just now and will let you know if it works.

Flow gives us one substantive bit of feedback and one kind of false alarm: - We shouldn't be putting a `stream_id` on PMs in exampleData; so, stop doing that. - When we start putting things together to call `addItemsToStreamArray`, we need to be sure `action.message` is a stream message. We were already sure of that; there's already an early return on `action.message.type !== stream`. But that early return needs to be just a bit later in order to satisfy Flow that `action.message` can't be a stream message. In particular, as Greg explains [1], """ Specifically what's happening is: we check `action.message.type`, but then we call `getOwnUserId` -- and that's some function that might go and do anything. So in particular it might go and mutate the same object that we're looking at as `action.message`. So that object *had* had a `type` of `'private'`, but now it might not and so it might not have a `stream_id`. """ [1] zulip#4506 (comment) In particular, Flow sees the `getOwnUserId` call, and that's some function that might go and do anything -- including But Flow seems to think it's done too early, and that something in the intervening code might thwart the job it's doing. So, move that early return a bit later.

chrisbobbe · 2021-03-10T03:07:56Z

OK, and I have indeed found a working approach at that site for subject and subject_links! Revision pushed. 😅

Flow gives us one substantive bit of feedback and one kind of false alarm: - We shouldn't be putting a `stream_id` on PMs in exampleData; so, stop doing that. - When we start putting things together to call `addItemsToStreamArray`, we need to be sure `action.message` is a stream message. We were already sure of that; there's already an early return on `action.message.type !== stream`. But that early return needs to be just a bit later in order to satisfy Flow that `action.message` can't be a stream message. In particular, as Greg explains [1], """ Specifically what's happening is: we check `action.message.type`, but then we call `getOwnUserId` -- and that's some function that might go and do anything. So in particular it might go and mutate the same object that we're looking at as `action.message`. So that object *had* had a `type` of `'private'`, but now it might not and so it might not have a `stream_id`. """ [1] zulip#4506 (comment) In particular, Flow sees the `getOwnUserId` call, and that's some function that might go and do anything -- including But Flow seems to think it's done too early, and that something in the intervening code might thwart the job it's doing. So, move that early return a bit later.

chrisbobbe · 2021-03-10T18:30:14Z

Just rebased this past #4518.

Like we did in 263b3f9.

And just grab the values we want straight from the `message` object itself.

The `avatar_url` and `reactions` fields will overwrite the original values even if we don't use `restMessage`. And it'll make something easier with Flow later in this series.

Above the line that introduces "properties that behave differently for stream vs. private messages". As the comment says, we don't use this property. But if we did, I'm not sure we'd want to describe it as behaving differently for stream messages and PMs. Here's the current doc for it [1]: """ `recipient_id`: integer A unique ID for the set of users receiving the message (either a stream or group of users). Useful primarily for hashing. """ While it's not super clear, it seems like any difference in its behavior between stream and PM messages looks like an implementation detail in the service of being "useful primarily for hashing". If we find that we need to use this, and we also run into problems like unwanted collisions in this field between PM and stream messages...then I suppose we could go back to treating this as different between PM and stream messages. But I imagine the way we'd do that is to put `recipient_id: number` on both `PmMessage` and `StreamMessages` (these types will come later in this series). Which after all wouldn't be helpful for type-checking. [1] https://zulip.com/api/get-messages

This is both unused and (according to the comment) unuseful.

chrisbobbe · 2021-03-31T22:19:04Z

Just rebased again and fixed some conflicts, including some from #4448.

Soon, `Message` will be a union of the two. For now, until we make a few changes to avoid `exponential-spread` errors, just make it `MessageBase`.

`PmMessage` isn't yet distinct from `Message`, but it will be soon.

Like we did in the previous commit for PM-making functions.

…rray. There's no reason we'd want to mutate this array. Given that, and the fact that we're about to make `ServerMessage` a union of `ServerMessageOf<PmMessage>` and `ServerMessageOf<StreamMessage>`, we might as well be courteous to `migrateMessages`' callers by allowing them to pass an array of just one of those types. In other words, we might as well allow this argument, which is an array, to be treated covariantly in the type of its elements. This is only possible when we the array is read-only. See discussion at zulip#4222 (comment), and in particular the example in "What about other types" at the bottom of this article: https://www.stephanboyer.com/post/132/what-are-covariance-and-contravariance.

…ssage`. It shouldn't be unsound if we leave these functions unannotated, or annotate them as `(Message) => Message`. But Flow is wary of having to dive deep into lots of branching conditionals, so we guide it toward a single path of logical reasoning that it's happy to take. As Greg explains [1], """ * The truth is that if the input message is a `PmMessage`, the output message will be a `PmMessage`, and similarly for `StreamMessage`. As a result, so long as the input is one of those, the output will be one of those. * But Flow doesn't want to go diving into a bunch of different cases by splitting up the union. Really it'd be fine in this situation, I think -- there's just the one, two-way, union -- but in general if it had a strategy that did that, it could end up recursively searching through lots of different hypothetical possibilities when trying to type-check something. So it doesn't. * So what we need to do is provide it some kind of guidance to find a single path of logical reasoning, without forking, that leads to the proof that this code is well-typed. """ [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60PmMessage.60.2C.20.60StreamMessage.60.20types.3F/near/1126672

In the same basic way as in the previous commit, but with a bit of added complexity: the function isn't `(Message) => Message`; it's `(ServerMessage) => Message`. So, start by expressing how `ServerMessage` relates to `Message`, in a way that applies uniformly to `PmMessage` and `StreamMessage`. Then, annotate the function's param by applying that expression to `M`, which is a subtype of `Message`, and leave the return value as `M`. See discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60PmMessage.60.2C.20.60StreamMessage.60.20types.3F/near/1126688.

This is the first thing that makes `PmMessage` and `StreamMessage` differ. The fact that we don't see Flow errors at this commit is a reasonable sign that we're ready to proceed with giving this treatment to the other pm- and stream-message-specific fields. Flow will likely give us substantive feedback when we do that, so we'll take it one field at a time.

Flow gives us one substantive bit of feedback: we shouldn't be putting a `stream_id` on PMs in exampleData. So, stop doing that.

…ssage. The "Notes from studying the server code" section would likely be helpful in the `StreamMessage` case, if we hadn't decided to deprecate the field on stream messages in favor of using `stream_id`. Since we have, no need to copy that over there.

In particular, not only do some fields, like `display_recipient`, have different types on `PmMessage` and `StreamMessage`, but also, some fields just plain won't exist on one of them, while they will exist on the other. Like `subject_links`. To handle a field like `subject_links`, we can't just make a simple tweak like ``` subject_links: oldMessage.type === 'stream' ? action.subject_links || oldMessage.subject_links : undefined, ``` because an object having `subject_links: undefined` isn't the same as an object not having a `subject_links` at all. `PmMessage` isn't supposed to have `subject_links` at all. A solution like ``` ...(oldMessage.type === 'stream' ? { subject_links: action.subject_links || oldMessage.subject_links } : undefined), ``` might be technically correct, but Flow has a hard time following it, and it panics with an unrelated error about `display_recipient`, and I don't really blame it. :) A solution like this seems to work, while still letting us avoid repeating ourselves by spelling out all the common fields twice.

…ge`.

The last field we need to do this for! :)

gnprice · 2021-04-01T00:15:16Z

Thanks @chrisbobbe ! All looks great -- merging, with a fix to just a nit I spotted in the jsdoc, which stood out with the help of Git's "color moved" feature:

Probably a small conflict-resolution error on rebasing past #4518.

chrisbobbe · 2021-04-01T00:17:47Z

Great, thanks for the review! 🎉

Like we did with `PmMessage` and `StreamMessage` in zulip#4506. We don't actually make `PmOutbox` any different from `StreamOutbox` in this commit; that'll come later.

chrisbobbe requested a review from gnprice March 3, 2021 16:12

chrisbobbe force-pushed the draft-separate-pm-stream-message-types branch 3 times, most recently from 38531d4 to 5d78aa8 Compare March 5, 2021 02:34

chrisbobbe commented Mar 5, 2021

View reviewed changes

src/api/modelTypes.js Show resolved Hide resolved

chrisbobbe force-pushed the draft-separate-pm-stream-message-types branch from 5d78aa8 to ccd8086 Compare March 5, 2021 02:45

gnprice reviewed Mar 6, 2021

View reviewed changes

chrisbobbe force-pushed the draft-separate-pm-stream-message-types branch from ccd8086 to 2fb36c7 Compare March 10, 2021 01:49

chrisbobbe force-pushed the draft-separate-pm-stream-message-types branch from 2fb36c7 to a5aeb4c Compare March 10, 2021 03:07

chrisbobbe force-pushed the draft-separate-pm-stream-message-types branch from a5aeb4c to 1a98482 Compare March 10, 2021 18:29

chrisbobbe changed the title ~~[draft] Demonstrate starting to separate PmMessage and StreamMessage.~~ Make PmMessage and StreamMessage types. Mar 10, 2021

chrisbobbe marked this pull request as ready for review March 10, 2021 18:29

chrisbobbe added 6 commits March 31, 2021 17:44

fetchActions tests [nfc]: Drop an omit that we don't need anymore.

00efd3a

Like we did in 263b3f9.

getMessages [nfc]: Stop using some variables we've defined.

3dbfb3f

And just grab the values we want straight from the `message` object itself.

getMessages [nfc]: Stop using restMessage in migrateMessages.

cb43516

The `avatar_url` and `reactions` fields will overwrite the original values even if we don't use `restMessage`. And it'll make something easier with Flow later in this series.

getMessages [nfc]: Straighten up after a recent refactor.

c65b5b7

modelTypes: Remove recipient_id from Message.

cd4820d

This is both unused and (according to the comment) unuseful.

chrisbobbe force-pushed the draft-separate-pm-stream-message-types branch from 1a98482 to e9593f0 Compare March 31, 2021 22:18

chrisbobbe added 2 commits March 31, 2021 16:57

modelTypes: Start separating PmMessage and StreamMessage.

09383f9

Soon, `Message` will be a union of the two. For now, until we make a few changes to avoid `exponential-spread` errors, just make it `MessageBase`.

exampleData types: Annotate PM-making functions as using PmMessage.

349bfae

`PmMessage` isn't yet distinct from `Message`, but it will be soon.

chrisbobbe added 11 commits March 31, 2021 16:57

exampleData types: Annotate streamMessage as using StreamMessage.

65de873

Like we did in the previous commit for PM-making functions.

modelTypes: Make Message a union of PmMessage and StreamMessage.

d011b98

modelTypes: Move .stream_id from Message to StreamMessage.

d6d5109

Flow gives us one substantive bit of feedback: we shouldn't be putting a `stream_id` on PMs in exampleData. So, stop doing that.

modelTypes: Split Message.subject into PmMessage and `StreamMessa…

fad245b

…ge`.

modelTypes: Move .subject_links from Message to StreamMessage.

ce55fbf

The last field we need to do this for! :)

gnprice force-pushed the draft-separate-pm-stream-message-types branch from e9593f0 to ce55fbf Compare April 1, 2021 00:15

gnprice merged commit ce55fbf into zulip:master Apr 1, 2021

chrisbobbe mentioned this pull request Apr 7, 2021

Baby steps toward detangling ComposeBox. #4505

Merged

chrisbobbe deleted the draft-separate-pm-stream-message-types branch April 14, 2021 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `PmMessage` and `StreamMessage` types. #4506

Make `PmMessage` and `StreamMessage` types. #4506

chrisbobbe commented Mar 3, 2021 •

edited

Loading

gnprice Mar 6, 2021

chrisbobbe Mar 10, 2021 •

edited

Loading

gnprice Apr 1, 2021

gnprice Mar 6, 2021

chrisbobbe Mar 10, 2021

gnprice Apr 1, 2021

gnprice Mar 6, 2021

gnprice Mar 6, 2021

gnprice commented Mar 6, 2021

chrisbobbe commented Mar 6, 2021 •

edited

Loading

chrisbobbe commented Mar 10, 2021

chrisbobbe commented Mar 10, 2021

chrisbobbe commented Mar 10, 2021

chrisbobbe commented Mar 31, 2021 •

edited

Loading

gnprice commented Apr 1, 2021

chrisbobbe commented Apr 1, 2021

		*/
		display_recipient: string \| $ReadOnlyArray<PmRecipientUser>, // `string` for type stream, else PmRecipientUser[]

Make PmMessage and StreamMessage types. #4506

Make PmMessage and StreamMessage types. #4506

Conversation

chrisbobbe commented Mar 3, 2021 • edited Loading

gnprice Mar 6, 2021

Choose a reason for hiding this comment

chrisbobbe Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

gnprice Apr 1, 2021

Choose a reason for hiding this comment

gnprice Mar 6, 2021

Choose a reason for hiding this comment

chrisbobbe Mar 10, 2021

Choose a reason for hiding this comment

gnprice Apr 1, 2021

Choose a reason for hiding this comment

gnprice Mar 6, 2021

Choose a reason for hiding this comment

gnprice Mar 6, 2021

Choose a reason for hiding this comment

gnprice commented Mar 6, 2021

chrisbobbe commented Mar 6, 2021 • edited Loading

chrisbobbe commented Mar 10, 2021

chrisbobbe commented Mar 10, 2021

chrisbobbe commented Mar 10, 2021

chrisbobbe commented Mar 31, 2021 • edited Loading

gnprice commented Apr 1, 2021

chrisbobbe commented Apr 1, 2021

Make `PmMessage` and `StreamMessage` types. #4506

Make `PmMessage` and `StreamMessage` types. #4506

chrisbobbe commented Mar 3, 2021 •

edited

Loading

chrisbobbe Mar 10, 2021 •

edited

Loading

chrisbobbe commented Mar 6, 2021 •

edited

Loading

chrisbobbe commented Mar 31, 2021 •

edited

Loading