From 2ddb3f8e8078847da458bdbdcbc0daced5c0f4e3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 23 May 2021 23:33:59 -0700 Subject: [PATCH 1/4] types: Distinguish PmOutbox vs. StreamOutbox more fully This expresses that the types of these properties `type`, `subject`, and `display_recipient` vary together, and that they do so in the same way as on `Message`. That in turn means that when we have a condition like `type === 'stream'`, Flow can infer that `display_recipient` will be a string (the stream name) rather than an array, and so on. --- src/__tests__/lib/exampleData.js | 7 ++++--- src/outbox/outboxActions.js | 9 +++++---- src/outbox/outboxReducer.js | 4 ++-- src/types.js | 29 ++++++++++++++++------------- 4 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 6027bc5bb3a..b9037d71e1d 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -452,16 +452,13 @@ const outboxMessageBase: $Diff = isSent: false, avatar_url: selfUser.avatar_url, content: '

Test.

', - display_recipient: stream.name, // id: ..., markdownContent: 'Test.', reactions: [], sender_email: selfUser.email, sender_full_name: selfUser.full_name, sender_id: selfUser.user_id, - subject: 'test topic', // timestamp: ..., - type: 'stream', }); /** @@ -476,6 +473,10 @@ export const streamOutbox = (data: $Shape<$Diff>) const outputTimestamp = timestamp ?? makeTime() / 1000; return deepFreeze({ ...outboxMessageBase, + type: 'stream', + display_recipient: stream.name, + subject: 'test topic', + ...data, id: outputTimestamp, timestamp: outputTimestamp, diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index 9efcc393641..8833fd33e21 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -9,6 +9,8 @@ import type { GlobalState, Narrow, Outbox, + PmOutbox, + StreamOutbox, UserOrBot, UserId, Action, @@ -124,10 +126,9 @@ const recipientsFromIds = (ids, allUsersById, ownUser) => { return result; }; -type DataFromNarrow = SubsetProperties< - Outbox, - {| type: mixed, display_recipient: mixed, subject: mixed |}, ->; +type DataFromNarrow = + | SubsetProperties + | SubsetProperties; const extractTypeToAndSubjectFromNarrow = ( narrow: Narrow, diff --git a/src/outbox/outboxReducer.js b/src/outbox/outboxReducer.js index 87143987ce1..82ab0199b81 100644 --- a/src/outbox/outboxReducer.js +++ b/src/outbox/outboxReducer.js @@ -31,8 +31,8 @@ export default (state: OutboxState = initialState, action: Action): OutboxState return messageSendStart(state, action); case MESSAGE_SEND_COMPLETE: - return state.map(item => - item.id !== action.local_message_id ? item : { ...item, isSent: true }, + return state.map((item: O) => + item.id !== action.local_message_id ? item : { ...(item: O), isSent: true }, ); case DELETE_OUTBOX_MESSAGE: diff --git a/src/types.js b/src/types.js index 6d22c1bb586..5be194f5f83 100644 --- a/src/types.js +++ b/src/types.js @@ -167,11 +167,6 @@ export type TopicExtended = {| * Properties in common among the two different flavors of a * `Outbox`: `PmOutbox` and `StreamOutbox`. */ -// TODO: This distinction between `PmOutbox` and `StreamOutbox` doesn't yet -// function as fully as `PmMessage` and `StreamMessage`: for properties -// like `type` and `display_recipient` where the types differ between -// `PmMessage` and `StreamMessage`, it currently loses the information -// that those differences are connected to each other. export type OutboxBase = $ReadOnly<{| /** Used for distinguishing from a `Message` object. */ isOutbox: true, @@ -196,20 +191,14 @@ export type OutboxBase = $ReadOnly<{| // `Message` but potentially separately. Message, {| - // TODO: Some of these have different types on `PmMessage` vs. - // `StreamMessage`; move those to `PmOutbox` and `StreamOutbox` - // respectively, to match that distinction here. avatar_url: mixed, content: mixed, - display_recipient: mixed, id: mixed, reactions: mixed, sender_id: mixed, sender_email: mixed, sender_full_name: mixed, - subject: mixed, timestamp: mixed, - type: mixed, |}, >, |}>; @@ -217,7 +206,14 @@ export type OutboxBase = $ReadOnly<{| export type PmOutbox = $ReadOnly<{| ...OutboxBase, - ...SubsetProperties, + ...SubsetProperties< + PmMessage, + {| + type: mixed, + display_recipient: mixed, + subject: mixed, + |}, + >, |}>; export type StreamOutbox = $ReadOnly<{| @@ -230,7 +226,14 @@ export type StreamOutbox = $ReadOnly<{| // argument passed to `SubsetProperties` of `StreamMessage`, below. stream_id?: number, - ...SubsetProperties, + ...SubsetProperties< + StreamMessage, + {| + type: mixed, + display_recipient: mixed, + subject: mixed, + |}, + >, |}>; /** From 8a17841a849fd04a360a5edf02c2c110f9992626 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 23 May 2021 23:35:33 -0700 Subject: [PATCH 2/4] types [nfc]: Demo using the new Outbox pm/stream type distinction These two asserts just confirmed that the `type` and `display_recipient` properties varied together in the expected way, in order to assure Flow that after checking `type` we'd get the right type of value out of `display_recipient`. Now that we've expressed that tandem variation directly in the actual types, we can drop the asserts. --- src/utils/recipient.js | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/utils/recipient.js b/src/utils/recipient.js index 5da1cbb2357..9dc3bee2142 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -1,5 +1,4 @@ /* @flow strict-local */ -import invariant from 'invariant'; // $FlowFixMe[untyped-import] import isEqual from 'lodash.isequal'; @@ -12,9 +11,7 @@ export const streamNameOfStreamMessage = (message: Message | Outbox): string => if (message.type !== 'stream') { throw new Error('streamNameOfStreamMessage: got PM'); } - const { display_recipient: streamName } = message; - invariant(typeof streamName === 'string', 'message type / display_recipient mismatch'); - return streamName; + return message.display_recipient; }; /** The recipients of a PM, in the form found on Message. Throws if a stream message. */ @@ -24,9 +21,7 @@ export const recipientsOfPrivateMessage = ( if (message.type !== 'private') { throw new Error('recipientsOfPrivateMessage: got stream message'); } - const { display_recipient: recipients } = message; - invariant(typeof recipients === 'object', 'message type / display_recipient mismatch'); - return recipients; + return message.display_recipient; }; /** From 4b92719e6d600f8c2986231f4cdd16c8e2a3a496 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 23 May 2021 23:53:03 -0700 Subject: [PATCH 3/4] example data: Use $Rest, not $Shape $Shape is not very sound -- Flow will sometimes treat the result as if it definitely has all the original object type's properties. Instead, use `$Rest` here, just as we do for `streamMessage` and others in this file. In fact, as a bonus simplification, fuse this use of `$Rest` with an inexact object type (for making everything optional) with the existing use of `$Diff` with an exact object type (for excluding a specific property), resulting in a single `$Rest` with a combined type. --- src/__tests__/lib/exampleData.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index b9037d71e1d..c4b8d1cec1e 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -467,7 +467,7 @@ const outboxMessageBase: $Diff = * * `.id` is always identical to `.timestamp` and should not be supplied. */ -export const streamOutbox = (data: $Shape<$Diff>): StreamOutbox => { +export const streamOutbox = (data: $Rest): StreamOutbox => { const { timestamp } = data; const outputTimestamp = timestamp ?? makeTime() / 1000; From 14fb5e1ce78540ea1bcd1c9b09edef2478c5a13b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 23 May 2021 23:43:32 -0700 Subject: [PATCH 4/4] types [nfc]: Unexport OutboxBase This is really an internal bit of how we go about constructing the types PmOutbox and StreamOutbox; it doesn't make much sense for types elsewhere in the application to refer to it. There was one place we'd been referring to it, at `outboxMessageBase` in `exampleData`, and that illustrates the point. Drop that type annotation entirely. Like `messagePropertiesBase`, this value is an internal helper used narrowly just within this module; its real type, in terms of the promise made in its interface, is "whatever the call site needs". Flow inherently checks that it satisfies *that* type. (Try it, after this change: delete one of the properties in the `outboxMessageBase` definition, and see an error appear where it's used in `streamOutbox`. This only works after the preceding commit which fixed `streamOutbox`'s use of the unsound `$Shape`.) And on the other hand, writing out what the type works out to explicitly is kind of verbose and just a recipe for confusion. --- src/__tests__/lib/exampleData.js | 9 +++------ src/types.js | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index c4b8d1cec1e..a5a05f65c6c 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -29,7 +29,7 @@ import type { MessagesState, RealmState, } from '../../reduxTypes'; -import type { Auth, Account, OutboxBase, StreamOutbox } from '../../types'; +import type { Auth, Account, StreamOutbox } from '../../types'; import { UploadedAvatarURL } from '../../utils/avatar'; import { ZulipVersion } from '../../utils/zulipVersion'; import { @@ -444,21 +444,18 @@ export const makeMessagesState = (messages: Message[]): MessagesState => */ /** - * Properties in common among PM and stream outbox messages, with no - * interesting data. + * Boring properties common across example outbox messages. */ -const outboxMessageBase: $Diff = deepFreeze({ +const outboxMessageBase = deepFreeze({ isOutbox: true, isSent: false, avatar_url: selfUser.avatar_url, content: '

Test.

', - // id: ..., markdownContent: 'Test.', reactions: [], sender_email: selfUser.email, sender_full_name: selfUser.full_name, sender_id: selfUser.user_id, - // timestamp: ..., }); /** diff --git a/src/types.js b/src/types.js index 5be194f5f83..0efc7a7e99c 100644 --- a/src/types.js +++ b/src/types.js @@ -167,7 +167,7 @@ export type TopicExtended = {| * Properties in common among the two different flavors of a * `Outbox`: `PmOutbox` and `StreamOutbox`. */ -export type OutboxBase = $ReadOnly<{| +type OutboxBase = $ReadOnly<{| /** Used for distinguishing from a `Message` object. */ isOutbox: true,