From 78e00a938afa938d889376e40c3dbf2aa7b9db4e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 11:35:45 -0700 Subject: [PATCH 01/10] eventActions [nfc]: Remove unnecessary checks in `responseToActions`. Typechecking will ensure that we don't get an undefined `action` or `action.type` here, so these runtime checks are unnecessary. Might as well simplify by removing them. --- src/events/eventActions.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/events/eventActions.js b/src/events/eventActions.js index 09b4f49c345..0e2126cc6e8 100644 --- a/src/events/eventActions.js +++ b/src/events/eventActions.js @@ -27,8 +27,8 @@ export const responseToActions = ( return false; } - if (!action || !action.type || action.type === 'unknown') { - console.log('Can not handle event', action.event); // eslint-disable-line + if (action.type === 'unknown') { + console.log('Cannot handle event', action.event); // eslint-disable-line return false; } From ef3c7fd37ed4173802005c8fd7998f9d4dddbec3 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 12:26:02 -0700 Subject: [PATCH 02/10] eventToAction: Make shell a little crunchier for `message` events. This is a very small step, but it addresses the following at the level of the event object itself. From our crunchy-shell doc [1]: """ [A]ny properties we don't actually use, we simply wouldn't look at and wouldn't store in the internal object. """ We're about to consolidate another piece of crunchy-shell code for `message` events so that it lives here. [1] https://github.com/zulip/zulip-mobile/blob/master/docs/architecture/crunchy-shell.md --- src/events/eventToAction.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index 3ad7c616911..bf451ce44fe 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -82,8 +82,10 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { case 'message': return { - ...event, type: EVENT_NEW_MESSAGE, + id: event.id, + message: event.message, + local_message_id: event.local_message_id, caughtUp: state.caughtUp, ownEmail: getOwnEmail(state), }; From f2b5b7146844c81454757a92536dc49e0cc3a587 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 13:24:15 -0700 Subject: [PATCH 03/10] eventMiddleware: Stop mutating events. Greg points out a big problem with `eventMiddleware` [1]: """ - It mutates the event object. This is confusing type-wise -- [...] it doesn't make a distinction between the types before and after, even though what it does to the object does indeed reshape it. This logic all morally belongs in `eventToAction`, or in a helper invoked directly from there from the case for the specific event type. """ So, take the part that mutated events and put it in `eventToAction`. Also, remove the test file, since it was only testing the logic we just transplanted. We could transplant these tests, to confirm that `eventToAction` does what we want it to. If we do, we should be careful of hitting diminishing returns when testing crunchy shell logic like that: - There's not much reason for the implementation to be subtle or complicated. Subtlety and complication come from not having crunchy-shell logic in the first place. - The implementation is expected to grow in proportion to the number of changes that happen on the server and the client. But that growth should not happen without clear inline comments (linking to specific versions/commits/etc.), which can be directly compared to the implementation. Trying to update the tests to match seems like it would invite discrepancies that would take work to resolve, even when the implementation was simple and correct all along. - We could test against extraordinary inputs for quite a long time without catching any bugs. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/965896 --- src/events/__tests__/eventMiddleware-test.js | 27 -------------------- src/events/eventMiddleware.js | 15 +++-------- src/events/eventToAction.js | 7 ++++- 3 files changed, 9 insertions(+), 40 deletions(-) delete mode 100644 src/events/__tests__/eventMiddleware-test.js diff --git a/src/events/__tests__/eventMiddleware-test.js b/src/events/__tests__/eventMiddleware-test.js deleted file mode 100644 index 4ec315c6e17..00000000000 --- a/src/events/__tests__/eventMiddleware-test.js +++ /dev/null @@ -1,27 +0,0 @@ -import eventMiddleware from '../eventMiddleware'; -import { NULL_ARRAY } from '../../nullObjects'; - -describe('eventMiddleware', () => { - test('if `event.flags` key exists, move it to `event.message.flags`', () => { - const state = { session: {} }; - const event = { - type: 'message', - flags: ['mentioned'], - message: {}, - }; - eventMiddleware(state, event); - - expect(event.flags).not.toBeDefined(); - expect(event.message.flags).toEqual(['mentioned']); - }); - - test('if `event.flags` does not exist, set message.flags to an empty array', () => { - const state = { session: {} }; - const event = { - type: 'message', - message: {}, - }; - eventMiddleware(state, event); - expect(event.message.flags).toEqual(NULL_ARRAY); - }); -}); diff --git a/src/events/eventMiddleware.js b/src/events/eventMiddleware.js index 24801f91763..9ccf194b144 100644 --- a/src/events/eventMiddleware.js +++ b/src/events/eventMiddleware.js @@ -10,19 +10,9 @@ import { NULL_ARRAY } from '../nullObjects'; /** * React to incoming `MessageEvent`s. - * - * Beware: this function modifies its `event` argument, and has other side - * effects besides! */ const messageEvent = (state: GlobalState, event: MessageEvent): void => { - // Move `flags` key from `event` to `event.message` for consistency, and - // default to an empty array if `event.flags` is not set. const flags = event.message.flags ?? event.flags ?? NULL_ARRAY; - if (!event.message.flags) { - // $FlowFixMe: Message is readonly to serve our use of it in Redux. - event.message.flags = flags; - delete event.flags; - } if (AppState.currentState !== 'active') { return; @@ -48,8 +38,9 @@ const messageEvent = (state: GlobalState, event: MessageEvent): void => { }; /** - * React to, and possibly normalize, incoming Zulip server events (from - * `/events` polling). + * React to incoming Zulip server events (from `/events` polling). + * + * Inputs are not mutated. */ export default (state: GlobalState, event_: GeneralEvent) => { switch (event_.type) { diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index bf451ce44fe..43480262a2e 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -84,7 +84,12 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { return { type: EVENT_NEW_MESSAGE, id: event.id, - message: event.message, + message: { + ...event.message, + // Move `flags` key from `event` to `event.message` for consistency, and + // default to an empty array if `event.flags` is not set. + flags: event.message.flags ?? event.flags ?? [], + }, local_message_id: event.local_message_id, caughtUp: state.caughtUp, ownEmail: getOwnEmail(state), From 871742a9f5f3abe543f11af8415462d735e4a22e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 13:46:47 -0700 Subject: [PATCH 04/10] eventMiddleware: Rename; run on actions instead of events. Following from discussion [1]. In a recent commit, we took out the parts of `eventMiddleware` that mutate event objects, and we started doing the same crunchy-shell conversions in `eventToAction` instead, and we had them not mutate anything. But `eventToAction` still takes care of running some side effects that don't mutate the inputs, like playing a sound. Playing a sound is the only thing it currently does, but it's plausible that we'll want a central place like this to do more side effects, as long as they don't mutate the inputs. (In fact, we'll soon move the only call site of `clearTypingNotification` into here.) We also take a further step to strengthen `eventToAction`'s claim to be the crunchy shell [2] for data we get from events: we bring `eventToAction` closer to the edge by having it run before the code in `eventMiddleware`, and we rename `eventMiddleware` because it's not in the middle anymore (and it was too vaguely named anyway). The function now takes an EventAction as an input instead of an event. An EventAction should be considered more trustworthy than an untyped event object. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/965896 [2] https://github.com/zulip/zulip-mobile/blob/master/docs/architecture/crunchy-shell.md --- ...dleware.js => doEventActionSideEffects.js} | 31 ++++++++++--------- src/events/eventActions.js | 17 ++++++---- 2 files changed, 28 insertions(+), 20 deletions(-) rename src/events/{eventMiddleware.js => doEventActionSideEffects.js} (51%) diff --git a/src/events/eventMiddleware.js b/src/events/doEventActionSideEffects.js similarity index 51% rename from src/events/eventMiddleware.js rename to src/events/doEventActionSideEffects.js index 9ccf194b144..cd452b56ead 100644 --- a/src/events/eventMiddleware.js +++ b/src/events/doEventActionSideEffects.js @@ -2,7 +2,9 @@ // import { Vibration } from 'react-native'; import { AppState } from 'react-native'; -import type { GeneralEvent, GlobalState, MessageEvent } from '../types'; +import type { GlobalState, Message } from '../types'; +import type { EventAction } from '../actionTypes'; +import { EVENT_NEW_MESSAGE } from '../actionConstants'; import { isHomeNarrow, isMessageInNarrow } from '../utils/narrow'; import { getActiveAccount, getChatScreenParams, getOwnEmail } from '../selectors'; import { playMessageSound } from '../utils/sound'; @@ -11,14 +13,14 @@ import { NULL_ARRAY } from '../nullObjects'; /** * React to incoming `MessageEvent`s. */ -const messageEvent = (state: GlobalState, event: MessageEvent): void => { - const flags = event.message.flags ?? event.flags ?? NULL_ARRAY; +const messageEvent = (state: GlobalState, message: Message): void => { + const flags = message.flags ?? NULL_ARRAY; if (AppState.currentState !== 'active') { return; } - const isPrivateMessage = Array.isArray(event.message.display_recipient); + const isPrivateMessage = Array.isArray(message.display_recipient); const isMentioned = flags.includes('mentioned') || flags.includes('wildcard_mentioned'); if (!(isPrivateMessage || isMentioned)) { return; @@ -28,9 +30,10 @@ const messageEvent = (state: GlobalState, event: MessageEvent): void => { const { narrow } = getChatScreenParams(state); const isUserInSameNarrow = activeAccount - && (narrow !== undefined // chat screen is not at top - && (!isHomeNarrow(narrow) && isMessageInNarrow(event.message, narrow, activeAccount.email))); - const isSenderSelf = getOwnEmail(state) === event.message.sender_email; + && narrow !== undefined // chat screen is not at top + && !isHomeNarrow(narrow) + && isMessageInNarrow(message, narrow, activeAccount.email); + const isSenderSelf = getOwnEmail(state) === message.sender_email; if (!isUserInSameNarrow && !isSenderSelf) { playMessageSound(); // Vibration.vibrate(); @@ -38,15 +41,15 @@ const messageEvent = (state: GlobalState, event: MessageEvent): void => { }; /** - * React to incoming Zulip server events (from `/events` polling). + * React to actions dispatched for Zulip server events. * - * Inputs are not mutated. + * To be run after the event actions are dispatched. Inputs are not + * mutated. */ -export default (state: GlobalState, event_: GeneralEvent) => { - switch (event_.type) { - case 'message': { - // $FlowFixMe This expresses our unchecked assumptions about `message` events. - messageEvent(state, (event_: MessageEvent)); +export default (state: GlobalState, action: EventAction) => { + switch (action.type) { + case EVENT_NEW_MESSAGE: { + messageEvent(state, action.message); break; } default: diff --git a/src/events/eventActions.js b/src/events/eventActions.js index 0e2126cc6e8..c0d9eceb556 100644 --- a/src/events/eventActions.js +++ b/src/events/eventActions.js @@ -1,12 +1,13 @@ /* @flow strict-local */ import { batchActions } from 'redux-batched-actions'; +import type { EventAction } from '../actionTypes'; import type { Action, Dispatch, GeneralEvent, GetState, GlobalState } from '../types'; import * as api from '../api'; import { logout } from '../account/accountActions'; import { deadQueue } from '../session/sessionActions'; import eventToAction from './eventToAction'; -import eventMiddleware from './eventMiddleware'; +import doEventActionSideEffects from './doEventActionSideEffects'; import { tryGetAuth } from '../selectors'; import actionCreator from '../actionCreator'; import { BackoffMachine } from '../utils/async'; @@ -16,12 +17,9 @@ import { ApiError } from '../api/apiErrors'; export const responseToActions = ( state: GlobalState, events: $ReadOnlyArray, -): Action[] => +): EventAction[] => events - .map(event => { - eventMiddleware(state, event); - return eventToAction(state, event); - }) + .map(event => eventToAction(state, event)) .filter(action => { if (action.type === 'ignore') { return false; @@ -78,6 +76,13 @@ export const startEventPolling = (queueId: number, eventId: number) => async ( actionCreator(dispatch, actions, getState()); dispatchOrBatch(dispatch, actions); + actions.forEach(action => { + // These side effects should not be moved to reducers, which + // are explicitly not the place for side effects (see + // https://redux.js.org/faq/actions). + doEventActionSideEffects(getState(), action); + }); + lastEventId = Math.max.apply(null, [lastEventId, ...events.map(x => x.id)]); } catch (e) { if (e.httpStatus === 401) { From 2c5d03c2955e35932feab0c791842f3718973cb8 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 15:02:50 -0700 Subject: [PATCH 05/10] eventActions [nfc]: Rename `responseToActions` to `eventsToActions`. It's a pure function of state and some events; it doesn't "respond" to them anymore. In particular, it takes some events and gives back some actions. --- src/events/__tests__/eventActions--test.js | 10 +++++----- src/events/eventActions.js | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/events/__tests__/eventActions--test.js b/src/events/__tests__/eventActions--test.js index df2854e31aa..ee5d1106d0b 100644 --- a/src/events/__tests__/eventActions--test.js +++ b/src/events/__tests__/eventActions--test.js @@ -1,18 +1,18 @@ -import { responseToActions } from '../eventActions'; +import { eventsToActions } from '../eventActions'; console.log = () => {}; // eslint-disable-line -describe('responseToActions', () => { +describe('eventsToActions', () => { test('empty response return no actions', () => { const events = []; - const actions = responseToActions({}, events); + const actions = eventsToActions({}, events); expect(actions).toBeEmpty(); }); test('filter out unknown event types and some known ones', () => { const events = [{ type: 'some unknown type' }, { type: 'heartbeat' }]; - const actions = responseToActions({}, events); + const actions = eventsToActions({}, events); expect(actions).toBeEmpty(); }); @@ -20,7 +20,7 @@ describe('responseToActions', () => { test('when known events process and return actions', () => { const event = { type: 'presence' }; const events = [event]; - const actions = responseToActions({}, events); + const actions = eventsToActions({}, events); expect(actions).toHaveLength(1); }); diff --git a/src/events/eventActions.js b/src/events/eventActions.js index c0d9eceb556..62f34fad207 100644 --- a/src/events/eventActions.js +++ b/src/events/eventActions.js @@ -14,7 +14,7 @@ import { BackoffMachine } from '../utils/async'; import { ApiError } from '../api/apiErrors'; /** Convert an `/events` response into a sequence of our Redux actions. */ -export const responseToActions = ( +export const eventsToActions = ( state: GlobalState, events: $ReadOnlyArray, ): EventAction[] => @@ -71,7 +71,7 @@ export const startEventPolling = (queueId: number, eventId: number) => async ( break; } - const actions = responseToActions(getState(), events); + const actions = eventsToActions(getState(), events); actionCreator(dispatch, actions, getState()); dispatchOrBatch(dispatch, actions); From bf504a939517178205377bf89d615ca9e50ff5ac Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 3 Aug 2020 14:46:16 -0700 Subject: [PATCH 06/10] doEventActionSideEffects [nfc]: Turn into an action creator. We're about to have this dispatch an action as a side effect. This makes me think that the Redux Middleware API [1] is another quite plausible place this stuff could go. But that might tempt a too broad range of changes, such as those that have nothing to do with server events. (If we ran these side effects from Redux middleware, they would be in a different "middle" than was meant by this function's previous name, `eventMiddleware`. Redux middleware is run after you've put together a well-formed action, not before.) [1] https://redux.js.org/api/applymiddleware/ --- src/events/doEventActionSideEffects.js | 8 ++++---- src/events/eventActions.js | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/events/doEventActionSideEffects.js b/src/events/doEventActionSideEffects.js index cd452b56ead..51578c63f96 100644 --- a/src/events/doEventActionSideEffects.js +++ b/src/events/doEventActionSideEffects.js @@ -2,7 +2,7 @@ // import { Vibration } from 'react-native'; import { AppState } from 'react-native'; -import type { GlobalState, Message } from '../types'; +import type { GlobalState, GetState, Dispatch, Message } from '../types'; import type { EventAction } from '../actionTypes'; import { EVENT_NEW_MESSAGE } from '../actionConstants'; import { isHomeNarrow, isMessageInNarrow } from '../utils/narrow'; @@ -43,10 +43,10 @@ const messageEvent = (state: GlobalState, message: Message): void => { /** * React to actions dispatched for Zulip server events. * - * To be run after the event actions are dispatched. Inputs are not - * mutated. + * To be dispatched after the event actions are dispatched. */ -export default (state: GlobalState, action: EventAction) => { +export default (action: EventAction) => async (dispatch: Dispatch, getState: GetState) => { + const state = getState(); switch (action.type) { case EVENT_NEW_MESSAGE: { messageEvent(state, action.message); diff --git a/src/events/eventActions.js b/src/events/eventActions.js index 62f34fad207..bce0efc25b5 100644 --- a/src/events/eventActions.js +++ b/src/events/eventActions.js @@ -80,7 +80,7 @@ export const startEventPolling = (queueId: number, eventId: number) => async ( // These side effects should not be moved to reducers, which // are explicitly not the place for side effects (see // https://redux.js.org/faq/actions). - doEventActionSideEffects(getState(), action); + dispatch(doEventActionSideEffects(action)); }); lastEventId = Math.max.apply(null, [lastEventId, ...events.map(x => x.id)]); From 6217f891e959b2d01b23b0293492ce096f39013e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 29 Jul 2020 16:23:03 -0700 Subject: [PATCH 07/10] fetchActions tests: Mock raw server message for fetch response. It's not quite correct to mock a Message object as the response from the message fetch; it's not in the Message form until we've run it through `migrateMessages`. We want our mocked object to have the ServerMessage type. So, do that, like we did in migrateMessages-test.js. --- src/message/__tests__/fetchActions-test.js | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 96f2f2926e8..5c19ba49116 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -1,11 +1,14 @@ /* @flow strict-local */ import configureStore from 'redux-mock-store'; import thunk from 'redux-thunk'; +import omit from 'lodash.omit'; import type { GlobalState } from '../../reduxTypes'; import type { Action } from '../../actionTypes'; import { isFetchNeededAtAnchor, fetchMessages, fetchOlder, fetchNewer } from '../fetchActions'; import { FIRST_UNREAD_ANCHOR } from '../../anchor'; +import type { Message } from '../../api/modelTypes'; +import type { ServerMessage } from '../../api/messages/getMessages'; import { streamNarrow, HOME_NARROW, HOME_NARROW_STR } from '../../utils/narrow'; import { navStateWithNarrow } from '../../utils/testHelpers'; import * as eg from '../../__tests__/lib/exampleData'; @@ -73,8 +76,6 @@ describe('fetchActions', () => { describe('fetchMessages', () => { const message1 = eg.streamMessage({ id: 1 }); - const message2 = eg.streamMessage({ id: 2 }); - const message3 = eg.streamMessage({ id: 3 }); const baseState = eg.reduxState({ ...navStateWithNarrow(HOME_NARROW), @@ -86,8 +87,20 @@ describe('fetchActions', () => { describe('success', () => { beforeEach(() => { + type CommonFields = $Diff; + + // A message exactly as we receive it from the server, before + // our own transformations. + // + // TODO: Deduplicate this logic with similar logic in + // migrateMessages-test.js. + const serverMessage1: ServerMessage = { + ...(omit(message1, 'reactions'): CommonFields), + reactions: [], + }; + const response = { - messages: [message1, message2, message3], + messages: [serverMessage1], result: 'success', }; fetch.mockResponseSuccess(JSON.stringify(response)); @@ -133,7 +146,7 @@ describe('fetchActions', () => { expect(actions[1].type).toBe('MESSAGE_FETCH_COMPLETE'); - expect(returnValue).toEqual([message1, message2, message3]); + expect(returnValue).toEqual([message1]); }); }); From eb2fd331622b978e9ccf75a8e2240d582bc9f2fb Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 29 Jul 2020 17:15:49 -0700 Subject: [PATCH 08/10] eventTypes [nfc]: Synchronize EventTypes class with `eventToAction`. It looks like we're using this class to store string constants for the `type` property the server uses to label events. We haven't used the constants in as many places as we could, but might as well bring them up-to-date. It now contains all the event types that we handle in `eventToAction`. --- src/api/eventTypes.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/api/eventTypes.js b/src/api/eventTypes.js index cc2b85f0377..d9f552d53ba 100644 --- a/src/api/eventTypes.js +++ b/src/api/eventTypes.js @@ -13,12 +13,26 @@ import type { Message, Stream, UserPresence } from './modelTypes'; export class EventTypes { + static alert_words: 'alert_words' = 'alert_words'; + static delete_message: 'delete_message' = 'delete_message'; static heartbeat: 'heartbeat' = 'heartbeat'; static message: 'message' = 'message'; + static muted_topics: 'muted_topics' = 'muted_topics'; static presence: 'presence' = 'presence'; + static reaction: 'reaction' = 'reaction'; + static realm_bot: 'realm_bot' = 'realm_bot'; + static realm_emoji: 'realm_emoji' = 'realm_emoji'; + static realm_filters: 'realm_filters' = 'realm_filters'; + static realm_user: 'realm_user' = 'realm_user'; static stream: 'stream' = 'stream'; static submessage: 'submessage' = 'submessage'; + static subscription: 'subscription' = 'subscription'; + static typing: 'typing' = 'typing'; + static update_display_settings: 'update_display_settings' = 'update_display_settings'; + static update_global_notifications: 'update_global_notifications' = 'update_global_notifications'; + static update_message: 'update_message' = 'update_message'; static update_message_flags: 'update_message_flags' = 'update_message_flags'; + static user_group: 'user_group' = 'user_group'; static user_status: 'user_status' = 'user_status'; } From 99f959f6203f882b7e5cd7226769f8a9dfc8a603 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 29 Jul 2020 17:30:26 -0700 Subject: [PATCH 09/10] migrateMessages tests [nfc]: Make type alias for something we write twice. This type is about to grow another exception besides 'reactions': 'avatar_url'. --- src/api/messages/__tests__/migrateMessages-test.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/api/messages/__tests__/migrateMessages-test.js b/src/api/messages/__tests__/migrateMessages-test.js index 9f6339f679f..75019185d96 100644 --- a/src/api/messages/__tests__/migrateMessages-test.js +++ b/src/api/messages/__tests__/migrateMessages-test.js @@ -20,11 +20,13 @@ describe('migrateMessages', () => { }, }; + type CommonFields = $Diff; + const serverMessage: ServerMessage = { // The `omit` shouldn't be necessary with Flow v0.111: "Spreads // now overwrite properties instead of inferring unions" // (https://medium.com/flow-type/spreads-common-errors-fixes-9701012e9d58). - ...(omit(eg.streamMessage(), 'reactions'): $Diff), + ...(omit(eg.streamMessage(), 'reactions'): CommonFields), reactions: [serverReaction], }; @@ -33,7 +35,7 @@ describe('migrateMessages', () => { const expectedOutput: Message[] = [ { // The `omit` shouldn't be necessary with Flow v0.111. - ...(omit(serverMessage, 'reactions'): $Diff), + ...(omit(serverMessage, 'reactions'): CommonFields), reactions: [ { user_id: reactingUser.user_id, From c72ec2c68c47c8d70d19f9dc771db62eac14f3f1 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 29 Jul 2020 17:35:37 -0700 Subject: [PATCH 10/10] getMessages [nfc]: Plumb `identity` down to `migrateMessages`. Soon, we'll need this to construct AvatarURL instances as part of our validation-at-the-edge in the crunchy shell pattern [1]. In particular, it gives us the realm, which we'll use to point to uploaded avatars. [1] https://github.com/zulip/zulip-mobile/blob/master/docs/architecture/crunchy-shell.md --- src/api/messages/__tests__/migrateMessages-test.js | 3 ++- src/api/messages/getMessages.js | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/api/messages/__tests__/migrateMessages-test.js b/src/api/messages/__tests__/migrateMessages-test.js index 75019185d96..83d60bc9e5c 100644 --- a/src/api/messages/__tests__/migrateMessages-test.js +++ b/src/api/messages/__tests__/migrateMessages-test.js @@ -2,6 +2,7 @@ import omit from 'lodash.omit'; import { migrateMessages } from '../getMessages'; +import { identityOfAuth } from '../../../account/accountMisc'; import * as eg from '../../../__tests__/lib/exampleData'; import type { ServerMessage, ServerReaction } from '../getMessages'; import type { Message } from '../../modelTypes'; @@ -47,7 +48,7 @@ describe('migrateMessages', () => { }, ]; - const actualOutput: Message[] = migrateMessages(input); + const actualOutput: Message[] = migrateMessages(input, identityOfAuth(eg.selfAuth)); test('Replace user object with `user_id`', () => { expect(actualOutput.map(m => m.reactions)).toEqual(expectedOutput.map(m => m.reactions)); diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index ae81c76fb74..9af24c7cae8 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -1,8 +1,10 @@ /* @flow strict-local */ import type { Auth, ApiResponseSuccess } from '../transportTypes'; +import type { Identity } from '../../types'; import type { Message, Narrow } from '../apiTypes'; import type { Reaction } from '../modelTypes'; import { apiGet } from '../apiFetch'; +import { identityOfAuth } from '../../account/accountMisc'; type ApiResponseMessages = {| ...ApiResponseSuccess, @@ -41,7 +43,7 @@ type ServerApiResponseMessages = {| |}; /** Exported for tests only. */ -export const migrateMessages = (messages: ServerMessage[]): Message[] => +export const migrateMessages = (messages: ServerMessage[], identity: Identity): Message[] => messages.map(message => { const { reactions, ...restMessage } = message; return { @@ -56,11 +58,11 @@ export const migrateMessages = (messages: ServerMessage[]): Message[] => }; }); -const migrateResponse = response => { +const migrateResponse = (response, identity: Identity) => { const { messages, ...restResponse } = response; return { ...restResponse, - messages: migrateMessages(messages), + messages: migrateMessages(messages, identity), }; }; @@ -91,5 +93,5 @@ export default async ( apply_markdown: true, use_first_unread_anchor: useFirstUnread, }); - return migrateResponse(response); + return migrateResponse(response, identityOfAuth(auth)); };