diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 8248429611a..179061be089 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -1,6 +1,7 @@ /* @flow strict-local */ import deepFreeze from 'deep-freeze'; import { createStore } from 'redux'; +import Immutable from 'immutable'; import type { CrossRealmBot, @@ -27,7 +28,6 @@ import { import rootReducer from '../../boot/reducers'; import { authOfAccount } from '../../account/accountMisc'; import { HOME_NARROW } from '../../utils/narrow'; -import { objectFromEntries } from '../../jsBackport'; /* ======================================================================== * Utilities @@ -391,7 +391,7 @@ export const streamMessage = (args?: {| /** Construct a MessagesState from a list of messages. */ export const makeMessagesState = (messages: Message[]): MessagesState => - objectFromEntries(messages.map(m => [m.id, m])); + Immutable.Map(messages.map(m => [m.id, m])); /* ======================================================================== * Outbox messages diff --git a/src/boot/__tests__/__snapshots__/replaceRevive-test.js.snap b/src/boot/__tests__/__snapshots__/replaceRevive-test.js.snap index dd48f3863dc..d99668b2c18 100644 --- a/src/boot/__tests__/__snapshots__/replaceRevive-test.js.snap +++ b/src/boot/__tests__/__snapshots__/replaceRevive-test.js.snap @@ -1,5 +1,7 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`Stringify emptyMap 1`] = `"{\\"data\\":{},\\"__serializedType__\\":\\"ImmutableMap\\"}"`; + exports[`Stringify fallbackAvatarURL 1`] = `"{\\"data\\":\\"https://zulip.example.org/avatar/1\\",\\"__serializedType__\\":\\"FallbackAvatarURL\\"}"`; exports[`Stringify gravatarURL 1`] = `"{\\"data\\":\\"https://secure.gravatar.com/avatar/3b01d0f626dc6944ed45dbe6c86d3e30?d=identicon\\",\\"__serializedType__\\":\\"GravatarURL\\"}"`; @@ -8,6 +10,8 @@ exports[`Stringify list 1`] = `"{\\"data\\":[1,2,\\"a\\",null],\\"__serializedTy exports[`Stringify map 1`] = `"{\\"data\\":{\\"a\\":1,\\"b\\":2,\\"c\\":3,\\"d\\":4},\\"__serializedType__\\":\\"ImmutableMap\\"}"`; +exports[`Stringify mapNumKeys 1`] = `"{\\"data\\":{\\"1\\":1,\\"2\\":2,\\"3\\":3,\\"4\\":4},\\"__serializedType__\\":\\"ImmutableMapNumKeys\\"}"`; + exports[`Stringify mapWithTypeKey 1`] = `"{\\"data\\":{\\"__serializedType__\\":\\"Object\\",\\"data\\":{\\"a\\":1},\\"__serializedType__value\\":{\\"__serializedType__\\":\\"Object\\",\\"data\\":{\\"b\\":[2]},\\"__serializedType__value\\":{\\"c\\":[3]}}},\\"__serializedType__\\":\\"ImmutableMap\\"}"`; exports[`Stringify plainObjectWithTypeKey 1`] = `"{\\"__serializedType__\\":\\"Object\\",\\"data\\":{\\"a\\":1},\\"__serializedType__value\\":{\\"__serializedType__\\":\\"Object\\",\\"data\\":{\\"b\\":[2]},\\"__serializedType__value\\":{\\"c\\":[3]}}}"`; diff --git a/src/boot/__tests__/replaceRevive-test.js b/src/boot/__tests__/replaceRevive-test.js index fdce976eb80..896c72406c7 100644 --- a/src/boot/__tests__/replaceRevive-test.js +++ b/src/boot/__tests__/replaceRevive-test.js @@ -19,6 +19,8 @@ const data = { [SERIALIZED_TYPE_FIELD_NAME]: { c: [3] }, }, }), + mapNumKeys: Immutable.Map([[1, 1], [2, 2], [3, 3], [4, 4]]), + emptyMap: Immutable.Map([]), zulipVersion: new ZulipVersion('3.0.0'), url: new URL('https://chat.zulip.org'), gravatarURL: GravatarURL.validateAndConstructInstance({ email: eg.selfUser.email }), diff --git a/src/boot/replaceRevive.js b/src/boot/replaceRevive.js index c7269d20c52..d4c79cc3cca 100644 --- a/src/boot/replaceRevive.js +++ b/src/boot/replaceRevive.js @@ -75,8 +75,24 @@ function replacer(key, value) { }; case (Immutable.List.prototype: $FlowFixMe): return { data: value, [SERIALIZED_TYPE_FIELD_NAME]: 'ImmutableList' }; - case (Immutable.Map.prototype: $FlowFixMe): - return { data: value, [SERIALIZED_TYPE_FIELD_NAME]: 'ImmutableMap' }; + case (Immutable.Map.prototype: $FlowFixMe): { + const firstKey = origValue.keySeq().first(); + return { + data: value, + // We assume that any `Immutable.Map` will have + // - all string keys, + // - all numeric keys, or + // - no keys (be empty). + // + // We store string-keyed maps with `ImmutableMap`, + // number-keyed maps with `ImmutableMapNumKeys`, and empty + // maps with either one of those (chosen arbitrarily) because + // the reviver will give the same output for both of them + // (i.e., an empty `Immutable.Map`). + [SERIALIZED_TYPE_FIELD_NAME]: + firstKey && typeof firstKey === 'number' ? 'ImmutableMapNumKeys' : 'ImmutableMap', + }; + } default: { // If the identity of the first item in the prototype chain // isn't good enough as a distinguishing mark, we can put some @@ -138,6 +154,9 @@ function reviver(key, value) { return Immutable.List(data); case 'ImmutableMap': return Immutable.Map(data); + case 'ImmutableMapNumKeys': { + return Immutable.Map(Object.keys(data).map(k => [Number.parseInt(k, 10), data[k]])); + } case 'Object': return { ...data, diff --git a/src/boot/store.js b/src/boot/store.js index 3d7dda1a6af..2a2c01b65db 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -248,6 +248,9 @@ const migrations: { [string]: (GlobalState) => GlobalState } = { ), }), + // Convert `messages` from object-as-map to `Immutable.Map`. + '23': dropCache, + // TIP: When adding a migration, consider just using `dropCache`. }; diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 6014d2938db..a047b7bf892 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -518,13 +518,11 @@ describe('narrowsReducer', () => { }); describe('EVENT_UPDATE_MESSAGE_FLAGS', () => { - const allMessages = { - // Flow doesn't like number literals as keys...but it also wants - // them to be numbers. See https://github.com/facebook/flow/issues/380. - [1]: eg.streamMessage({ id: 1 }) /* eslint-disable-line no-useless-computed-key */, - [2]: eg.streamMessage({ id: 2 }) /* eslint-disable-line no-useless-computed-key */, - [4]: eg.streamMessage({ id: 4 }) /* eslint-disable-line no-useless-computed-key */, - }; + const allMessages = eg.makeMessagesState([ + eg.streamMessage({ id: 1 }), + eg.streamMessage({ id: 2 }), + eg.streamMessage({ id: 4 }), + ]); test('Do nothing if the is:starred narrow has not been fetched', () => { const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 9d2d9deec7b..8dac38d685c 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -23,11 +23,7 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('getMessagesForNarrow', () => { const message = eg.streamMessage({ id: 123 }); - const messages = { - // Flow doesn't like number literals as keys...but it also wants - // them to be numbers. - [123]: message /* eslint-disable-line no-useless-computed-key */, - }; + const messages = eg.makeMessagesState([message]); const outboxMessage = eg.makeOutboxMessage({}); test('if no outbox messages returns messages with no change', () => { @@ -43,7 +39,7 @@ describe('getMessagesForNarrow', () => { const result = getMessagesForNarrow(state, HOME_NARROW); - expect(result).toEqual([state.messages[123]]); + expect(result).toEqual([state.messages.get(123)]); }); test('combine messages and outbox in same narrow', () => { @@ -78,7 +74,7 @@ describe('getMessagesForNarrow', () => { const result = getMessagesForNarrow(state, HOME_NARROW); - expect(result).toEqual([state.messages[123]]); + expect(result).toEqual([state.messages.get(123)]); }); test('do not combine messages and outbox in different narrow', () => { @@ -137,7 +133,7 @@ describe('getLastMessageId', () => { narrows: Immutable.Map({ [HOME_NARROW_STR]: [], }), - messages: {}, + messages: eg.makeMessagesState([]), outbox: [], }); diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index 46970a3e3f7..e019b7d1498 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -30,6 +30,7 @@ import { } from '../utils/narrow'; import { shouldBeMuted } from '../utils/message'; import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects'; +import * as logging from '../utils/logging'; export const outboxMessagesForNarrow: Selector = createSelector( (state, narrow) => narrow, @@ -61,7 +62,16 @@ export const getFetchedMessageIdsForNarrow = (state: GlobalState, narrow: Narrow const getFetchedMessagesForNarrow: Selector = createSelector( getFetchedMessageIdsForNarrow, state => getMessages(state), - (messageIds, messages) => messageIds.map(id => messages[id]), + (messageIds, messages) => + messageIds.map(id => { + const message = messages.get(id); + if (!message) { + const msg = 'getFetchedMessagesForNarrow: message with id is missing in getMessages(state)'; + logging.error(msg, { id }); + throw new Error(msg); + } + return message; + }), ); // Prettier mishandles this Flow syntax. diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js index 51934f4ea5f..2525bb4f49d 100644 --- a/src/message/__tests__/messagesReducer-test.js +++ b/src/message/__tests__/messagesReducer-test.js @@ -1,38 +1,30 @@ +/* @flow strict-local */ import deepFreeze from 'deep-freeze'; +import type { Submessage } from '../../types'; import messagesReducer from '../messagesReducer'; import { FIRST_UNREAD_ANCHOR } from '../../anchor'; import { MESSAGE_FETCH_COMPLETE, - EVENT_NEW_MESSAGE, EVENT_SUBMESSAGE, EVENT_MESSAGE_DELETE, EVENT_UPDATE_MESSAGE, EVENT_REACTION_ADD, EVENT_REACTION_REMOVE, } from '../../actionConstants'; +import * as eg from '../../__tests__/lib/exampleData'; +import { ALL_PRIVATE_NARROW, HOME_NARROW } from '../../utils/narrow'; describe('messagesReducer', () => { - test('handles unknown action and no previous state by returning initial state', () => { - const newState = messagesReducer(undefined, {}); - expect(newState).toBeDefined(); - }); - describe('EVENT_NEW_MESSAGE', () => { test('appends message to state producing a copy of messages', () => { - const prevState = deepFreeze({ - 1: { id: 1 }, - 2: { id: 2 }, - }); - const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { id: 3 }, - }); - const expectedState = { - 1: { id: 1 }, - 2: { id: 2 }, - 3: { id: 3 }, - }; + const message1 = eg.streamMessage({ id: 1 }); + const message2 = eg.streamMessage({ id: 2 }); + const message3 = eg.streamMessage({ id: 3 }); + + const prevState = eg.makeMessagesState([message1, message2]); + const action = deepFreeze({ ...eg.eventNewMessageActionBase, message: message3 }); + const expectedState = eg.makeMessagesState([message1, message2, message3]); const newState = messagesReducer(prevState, action); expect(newState).toEqual(expectedState); expect(newState).not.toBe(prevState); @@ -41,44 +33,67 @@ describe('messagesReducer', () => { describe('EVENT_SUBMESSAGE', () => { test('if the message does not exist do not mutate the state', () => { - const prevState = deepFreeze({ - 1: { id: 1 }, - 2: { id: 2 }, - }); + const message1 = eg.streamMessage({ id: 1 }); + const message2 = eg.streamMessage({ id: 2 }); + + const prevState = eg.makeMessagesState([message1, message2]); const action = deepFreeze({ + id: 1, type: EVENT_SUBMESSAGE, message_id: 3, submessage_id: 2, + sender_id: eg.otherUser.user_id, + msg_type: 'widget', + content: eg.randString(), }); const newState = messagesReducer(prevState, action); expect(newState).toBe(prevState); }); test('if the message exists add the incoming data to `submessages`', () => { - const prevState = deepFreeze({ - 1: { id: 1 }, - 2: { id: 2, submessages: [{ id: 1 }] }, + const message1 = eg.streamMessage({ id: 1 }); + const message2 = eg.streamMessage({ + id: 2, + submessages: [ + { + id: 1, + message_id: 2, + sender_id: eg.otherUser.user_id, + msg_type: 'widget', // only this type is currently available + content: eg.randString(), // JSON string + }, + ], }); + + const prevState = eg.makeMessagesState([message1, message2]); const action = deepFreeze({ + id: 1, type: EVENT_SUBMESSAGE, - content: '{hello: "world"}', - message_id: 2, + message_id: message2.id, submessage_id: 2, + sender_id: eg.otherUser.user_id, + msg_type: 'widget', + content: '{hello: "world"}', }); - const expectedState = { - 1: { id: 1 }, - 2: { - id: 2, + const expectedState = eg.makeMessagesState([ + message1, + { + ...message2, submessages: [ - { id: 1 }, + // We know message2 has `submessages`; we defined it that + // way. + // $FlowFixMe + ...(message2.submessages: $ReadOnlyArray), { id: 2, - content: '{hello: "world"}', message_id: 2, + sender_id: eg.otherUser.user_id, + msg_type: 'widget', + content: '{hello: "world"}', }, ], }, - }; + ]); const newState = messagesReducer(prevState, action); expect(newState).toEqual(expectedState); expect(newState).not.toBe(prevState); @@ -87,35 +102,37 @@ describe('messagesReducer', () => { describe('EVENT_MESSAGE_DELETE', () => { test('if a message does not exist no changes are made', () => { - const prevState = deepFreeze({ 1: { id: 1 } }); - const action = deepFreeze({ - type: EVENT_MESSAGE_DELETE, - messageIds: [2], - }); + const message1 = eg.streamMessage({ id: 1 }); + + const prevState = eg.makeMessagesState([message1]); + const action = deepFreeze({ type: EVENT_MESSAGE_DELETE, messageIds: [2] }); const newState = messagesReducer(prevState, action); expect(newState).toEqual(prevState); + expect(newState).toBe(prevState); }); test('if a message exists it is deleted', () => { - const prevState = deepFreeze({ 1: { id: 1 }, 2: { id: 2 } }); + const message1 = eg.streamMessage({ id: 1 }); + const message2 = eg.streamMessage({ id: 2 }); - const action = deepFreeze({ - type: EVENT_MESSAGE_DELETE, - messageIds: [2], - }); - const expectedState = deepFreeze({ 1: { id: 1 } }); + const prevState = eg.makeMessagesState([message1, message2]); + const action = deepFreeze({ type: EVENT_MESSAGE_DELETE, messageIds: [message2.id] }); + const expectedState = eg.makeMessagesState([message1]); const newState = messagesReducer(prevState, action); expect(newState).toEqual(expectedState); }); test('if multiple messages indicated, delete the ones that exist', () => { - const prevState = deepFreeze({ 1: { id: 1 }, 2: { id: 2 }, 3: { id: 3 } }); + const message1 = eg.streamMessage({ id: 1 }); + const message2 = eg.streamMessage({ id: 2 }); + const message3 = eg.streamMessage({ id: 3 }); + const prevState = eg.makeMessagesState([message1, message2, message3]); const action = deepFreeze({ type: EVENT_MESSAGE_DELETE, - messageIds: [2, 3, 4], + messageIds: [message2.id, message3.id, 4], }); - const expectedState = deepFreeze({ 1: { id: 1 } }); + const expectedState = eg.makeMessagesState([message1]); const newState = messagesReducer(prevState, action); expect(newState).toEqual(expectedState); }); @@ -123,173 +140,183 @@ describe('messagesReducer', () => { describe('EVENT_UPDATE_MESSAGE', () => { test('if a message does not exist no changes are made', () => { - const prevState = deepFreeze({ - 1: { id: 1 }, - 2: { id: 2 }, - }); + const message1 = eg.streamMessage({ id: 1 }); + const message2 = eg.streamMessage({ id: 2 }); + const message3 = eg.streamMessage({ id: 3 }); + + const prevState = eg.makeMessagesState([message1, message2]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE, - messageId: 3, + edit_timestamp: Date.now() - 1000, + message_id: message3.id, + orig_content: eg.randString(), + orig_rendered_content: eg.randString(), + prev_rendered_content_version: 0, + rendered_content: eg.randString(), + subject_links: [], + subject: eg.randString(), + user_id: message3.sender_id, }); const newState = messagesReducer(prevState, action); expect(newState).toBe(prevState); }); - test('when a message exists in state, new state and new object is created with updated message in every key', () => { - const prevState = deepFreeze({ - 1: { id: 1 }, - 2: { id: 2 }, - 3: { id: 3, content: 'Old content' }, - }); + test('when a message exists in state, it is updated', () => { + const message1 = eg.streamMessage({ id: 1 }); + const message2 = eg.streamMessage({ id: 2 }); + const message3Old = eg.streamMessage({ id: 3, content: '

Old content

' }); + const message3New = { + ...message3Old, + content: '

New content

', + edit_history: [ + { + prev_rendered_content: '

Old content

', + prev_rendered_content_version: 1, + timestamp: 123, + user_id: message3Old.sender_id, + }, + ], + last_edit_timestamp: 123, + }; + + const prevState = eg.makeMessagesState([message1, message2, message3Old]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE, - message_id: 3, - orig_rendered_content: '

Old content

', - rendered_content: '

New content

', edit_timestamp: 123, + message_id: message3New.id, + orig_content: '

Old content

', + orig_rendered_content: '

Old content

', prev_rendered_content_version: 1, - user_id: 5, + rendered_content: '

New content

', + subject_links: [], + subject: message3New.subject, + user_id: message3New.sender_id, }); - const expectedState = { - 1: { id: 1 }, - 2: { id: 2 }, - 3: { - id: 3, - content: '

New content

', - last_edit_timestamp: 123, - edit_history: [ - { - prev_rendered_content: '

Old content

', - prev_rendered_content_version: 1, - timestamp: 123, - user_id: 5, - }, - ], - }, - }; + const expectedState = eg.makeMessagesState([message1, message2, message3New]); const newState = messagesReducer(prevState, action); - expect(newState).not.toBe(prevState); expect(newState).toEqual(expectedState); }); test('when event contains a new subject but no new content only subject is updated', () => { - const prevState = deepFreeze({ - 1: { - id: 1, - content: 'Old content', - subject: 'Old subject', - last_edit_timestamp: 123, - subject_links: [], - edit_history: [], - }, + const message1Old = eg.streamMessage({ + id: 1, + content: 'Old content', + subject: 'Old subject', + last_edit_timestamp: 123, + subject_links: [], + edit_history: [], }); - + const message1New = { + ...message1Old, + subject: 'New topic', + last_edit_timestamp: 123, + subject_links: [], + edit_history: [ + { + timestamp: 123, + user_id: message1Old.sender_id, + prev_subject: message1Old.subject, + prev_rendered_content: message1Old.content, + prev_rendered_content_version: 1, + }, + ], + }; + const prevState = eg.makeMessagesState([message1Old]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE, - message_id: 1, - subject: 'New topic', - orig_subject: 'Old subject', edit_timestamp: 123, - user_id: 5, + message_id: message1New.id, + orig_content: message1Old.content, + orig_subject: message1Old.subject, + orig_rendered_content: message1Old.content, + prev_rendered_content_version: 1, + rendered_content: message1New.content, + subject_links: [], + subject: message1New.subject, + user_id: message1Old.sender_id, }); - - const expectedState = { - 1: { - id: 1, - content: 'Old content', - subject: 'New topic', - last_edit_timestamp: 123, - subject_links: [], - edit_history: [ - { - prev_subject: 'Old subject', - timestamp: 123, - user_id: 5, - }, - ], - }, - }; - + const expectedState = eg.makeMessagesState([message1New]); const newState = messagesReducer(prevState, action); - - expect(newState).not.toBe(prevState); expect(newState).toEqual(expectedState); }); test('when event contains a new subject and a new content, update both and update edit history object', () => { - const prevState = deepFreeze({ - 1: { - id: 1, - content: 'Old content', - subject: 'New topic', - last_edit_timestamp: 123, - subject_links: [], - edit_history: [ - { - prev_subject: 'Old subject', - timestamp: 123, - user_id: 5, - }, - ], - }, + const message1Old = eg.streamMessage({ + id: 1, + content: '

Old content

', + subject: 'Old subject', + last_edit_timestamp: 123, + subject_links: [], + edit_history: [ + { + prev_subject: 'Old subject', + timestamp: 123, + user_id: eg.otherUser.user_id, + }, + ], }); + const message1New = { + ...message1Old, + content: '

New content

', + subject: 'New updated topic', + last_edit_timestamp: 456, + subject_links: [], + edit_history: [ + { + prev_rendered_content: '

Old content

', + prev_rendered_content_version: 1, + prev_subject: 'Old subject', + timestamp: 456, + user_id: message1Old.sender_id, + }, + ...message1Old.edit_history, + ], + }; + + const prevState = eg.makeMessagesState([message1Old]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE, - message_id: 1, - orig_rendered_content: '

Old content

', - rendered_content: '

New content

', - subject: 'New updated topic', - orig_subject: 'New topic', - prev_rendered_content_version: 1, edit_timestamp: 456, - user_id: 5, + message_id: message1Old.id, + orig_content: message1Old.content, + orig_rendered_content: message1Old.content, + rendered_content: message1New.content, + subject: message1New.subject, + orig_subject: message1Old.subject, + prev_rendered_content_version: 1, + user_id: message1New.sender_id, subject_links: [], }); - const expectedState = { - 1: { - id: 1, - content: '

New content

', - subject: 'New updated topic', - last_edit_timestamp: 456, - subject_links: [], - edit_history: [ - { - prev_rendered_content: '

Old content

', - prev_rendered_content_version: 1, - prev_subject: 'New topic', - timestamp: 456, - user_id: 5, - }, - { - prev_subject: 'Old subject', - timestamp: 123, - user_id: 5, - }, - ], - }, - }; + const expectedState = eg.makeMessagesState([message1New]); const newState = messagesReducer(prevState, action); - expect(newState).not.toBe(prevState); expect(newState).toEqual(expectedState); }); }); describe('EVENT_REACTION_ADD', () => { test('on event received, add reaction to message with given id', () => { - const prevState = deepFreeze({ - 1: { id: 1, reactions: [] }, - 2: { id: 2, reactions: [] }, - }); + const message1 = eg.streamMessage({ id: 1, reactions: [] }); + const message2 = eg.streamMessage({ id: 2, reactions: [] }); + const reaction = eg.unicodeEmojiReaction; + + const prevState = eg.makeMessagesState([message1, message2]); const action = deepFreeze({ + id: 1, type: EVENT_REACTION_ADD, - message_id: 2, - emoji_name: 'hello', - user_id: 2, + message_id: message2.id, + ...reaction, }); - const expectedState = { - 1: { id: 1, reactions: [] }, - 2: { id: 2, reactions: [{ emoji_name: 'hello', user_id: 2 }] }, - }; + const expectedState = eg.makeMessagesState([ + message1, + { + ...message2, + reactions: [reaction], + }, + ]); const actualState = messagesReducer(prevState, action); expect(actualState).toEqual(expectedState); }); @@ -297,45 +324,52 @@ describe('messagesReducer', () => { describe('EVENT_REACTION_REMOVE', () => { test('if message does not contain reaction, no change is made', () => { - const prevState = deepFreeze({ - 1: { id: 1, reactions: [] }, - }); + const message1 = eg.streamMessage({ id: 1, reactions: [] }); + const reaction = eg.unicodeEmojiReaction; + + const prevState = eg.makeMessagesState([message1]); const action = deepFreeze({ + id: 1, type: EVENT_REACTION_REMOVE, message_id: 1, - emoji_name: 'hello', - user_id: 2, + ...reaction, }); - const expectedState = { - 1: { id: 1, reactions: [] }, - }; + const expectedState = eg.makeMessagesState([message1]); const actualState = messagesReducer(prevState, action); expect(actualState).toEqual(expectedState); }); test('reaction is removed only from specified message, only for given user', () => { - const prevState = deepFreeze({ - 1: { - id: 1, - reactions: [ - { emoji_name: 'hello', user_id: 1 }, - { emoji_name: 'hello', user_id: 2 }, - { emoji_name: 'goodbye', user_id: 1 }, - ], - }, - }); + const reaction1 = { + ...eg.unicodeEmojiReaction, + emoji_code: '1f44b', + emoji_name: 'wave', + user_id: 1, + }; + const reaction2 = { + ...eg.unicodeEmojiReaction, + emoji_code: '1f44b', + emoji_name: 'wave', + user_id: 2, + }; + const reaction3 = { + ...eg.unicodeEmojiReaction, + emoji_code: '1f6e0', + emoji_name: 'working_on_it', + user_id: 1, + }; + + const message1 = eg.streamMessage({ id: 1, reactions: [reaction1, reaction2, reaction3] }); + const prevState = eg.makeMessagesState([message1]); const action = deepFreeze({ + id: 1, type: EVENT_REACTION_REMOVE, - message_id: 1, - emoji_name: 'hello', - user_id: 1, + message_id: message1.id, + ...reaction1, }); - const expectedState = { - 1: { - id: 1, - reactions: [{ emoji_name: 'hello', user_id: 2 }, { emoji_name: 'goodbye', user_id: 1 }], - }, - }; + const expectedState = eg.makeMessagesState([ + { ...message1, reactions: [reaction2, reaction3] }, + ]); const actualState = messagesReducer(prevState, action); expect(actualState).toEqual(expectedState); }); @@ -343,72 +377,82 @@ describe('messagesReducer', () => { describe('MESSAGE_FETCH_COMPLETE', () => { test('fetched messages are added to the state', () => { - const prevState = deepFreeze({ - 1: { id: 1 }, - 2: { id: 2 }, - 4: { id: 4 }, - }); + const message1 = eg.pmMessage({ id: 1 }); + const message2 = eg.pmMessage({ id: 2 }); + const message3 = eg.pmMessage({ id: 3 }); + const message4 = eg.pmMessage({ id: 4 }); + const message5 = eg.pmMessage({ id: 5 }); + + const prevState = eg.makeMessagesState([message1, message2, message3]); const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, - messages: [{ id: 3 }, { id: 4 }, { id: 5 }], + messages: [message3, message4, message5], + narrow: ALL_PRIVATE_NARROW, + anchor: FIRST_UNREAD_ANCHOR, + numBefore: 50, + numAfter: 50, + foundOldest: false, + foundNewest: false, + ownUserId: eg.selfUser.user_id, }); - const expectedState = { - 1: { id: 1 }, - 2: { id: 2 }, - 3: { id: 3 }, - 4: { id: 4 }, - 5: { id: 5 }, - }; + const expectedState = eg.makeMessagesState([ + message1, + message2, + message3, + message4, + message5, + ]); const newState = messagesReducer(prevState, action); expect(newState).toEqual(expectedState); }); test('when anchor is FIRST_UNREAD_ANCHOR common messages are not replaced', () => { - const commonMessages = { 2: { id: 2, timestamp: 4 }, 3: { id: 3, timestamp: 5 } }; - const prevState = deepFreeze({ - 1: { id: 1, timestamp: 3 }, - ...commonMessages, - }); + const message1 = eg.streamMessage({ id: 1, timestamp: 3 }); + const message2 = eg.streamMessage({ id: 2, timestamp: 4 }); + const message3 = eg.streamMessage({ id: 3, timestamp: 5 }); + const prevState = eg.makeMessagesState([message1, message2, message3]); const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, + messages: [message2, message3], + narrow: HOME_NARROW, anchor: FIRST_UNREAD_ANCHOR, - narrow: [], - messages: [{ id: 2, timestamp: 4 }, { id: 3, timestamp: 5 }], + numBefore: 50, + numAfter: 50, + foundOldest: false, + foundNewest: false, + ownUserId: eg.selfUser.user_id, }); const newState = messagesReducer(prevState, action); - expect(newState['2']).toEqual(commonMessages['2']); - expect(newState['3']).toEqual(commonMessages['3']); + expect(newState.get(message2.id)).toEqual(message2); + expect(newState.get(message3.id)).toEqual(message3); }); test('when anchor is FIRST_UNREAD_ANCHOR deep equal is performed to separate common messages', () => { - const commonMessages = { 2: { id: 2, timestamp: 4 }, 3: { id: 3, timestamp: 5 } }; - const changedMessages = { 4: { id: 4, timestamp: 6, subject: 'new topic' } }; - const prevState = deepFreeze({ - 1: { id: 1, timestamp: 3 }, - ...commonMessages, - 4: { id: 4, timestamp: 6, subject: 'some topic' }, - }); + const message1 = eg.streamMessage({ id: 1, timestamp: 3 }); + const message2 = eg.streamMessage({ id: 2, timestamp: 4 }); + const message3 = eg.streamMessage({ id: 3, timestamp: 5 }); + const message4Old = eg.streamMessage({ id: 4, timestamp: 6, subject: 'some topic' }); + const message4New = { ...message4Old, subject: 'new topic' }; + const prevState = eg.makeMessagesState([message1, message2, message3, message4Old]); const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, + messages: [message2, message3, message4New], + narrow: HOME_NARROW, anchor: FIRST_UNREAD_ANCHOR, - narrow: [], - messages: [{ id: 2, timestamp: 4 }, { id: 3, timestamp: 5 }, changedMessages['4']], + numBefore: 50, + numAfter: 50, + foundOldest: false, + foundNewest: false, + ownUserId: eg.selfUser.user_id, }); - - const expectedState = { - ...commonMessages, - ...changedMessages, - }; - const newState = messagesReducer(prevState, action); - - expect(newState['2']).toEqual(expectedState['2']); - expect(newState['3']).toEqual(expectedState['3']); - expect(newState['4']).toEqual(expectedState['4']); + expect(newState.get(message2.id)).toEqual(message2); + expect(newState.get(message3.id)).toEqual(message3); + expect(newState.get(message4New.id)).toEqual(message4New); }); }); }); diff --git a/src/message/messageSelectors.js b/src/message/messageSelectors.js index 1dcefa2ac8c..24e3e42823a 100644 --- a/src/message/messageSelectors.js +++ b/src/message/messageSelectors.js @@ -41,7 +41,7 @@ export const getPrivateMessages: Selector = createSelector( const pmIds = narrows.get(ALL_PRIVATE_NARROW_STR) || NULL_ARRAY; pmIds.forEach(id => { - const msg = messages[id]; + const msg = messages.get(id); if (msg !== undefined) { privateMessages.push(msg); } else { diff --git a/src/message/messagesReducer.js b/src/message/messagesReducer.js index 85bf86b3ef2..31f6a23d21a 100644 --- a/src/message/messagesReducer.js +++ b/src/message/messagesReducer.js @@ -1,5 +1,6 @@ /* @flow strict-local */ import omit from 'lodash.omit'; +import Immutable from 'immutable'; import type { MessagesState, Action } from '../types'; import { @@ -15,126 +16,17 @@ import { EVENT_REACTION_REMOVE, EVENT_UPDATE_MESSAGE, } from '../actionConstants'; -import { NULL_ARRAY, NULL_OBJECT } from '../nullObjects'; -import { groupItemsById } from '../utils/misc'; +import { NULL_ARRAY } from '../nullObjects'; -const initialState: MessagesState = NULL_OBJECT; - -const eventReactionAdd = (state, action) => { - const oldMessage = state[action.message_id]; - if (!oldMessage) { - return state; - } - return { - ...state, - [action.message_id]: { - ...oldMessage, - reactions: oldMessage.reactions.concat({ - emoji_name: action.emoji_name, - user_id: action.user_id, - reaction_type: action.reaction_type, - emoji_code: action.emoji_code, - }), - }, - }; -}; - -const eventReactionRemove = (state, action) => { - const oldMessage = state[action.message_id]; - if (!oldMessage) { - return state; - } - return { - ...state, - [action.message_id]: { - ...oldMessage, - reactions: oldMessage.reactions.filter( - x => !(x.emoji_name === action.emoji_name && x.user_id === action.user_id), - ), - }, - }; -}; +const initialState: MessagesState = Immutable.Map([]); const eventNewMessage = (state, action) => { // TODO: Optimize -- Only update if the new message belongs to at least // one narrow that is caught up. - if (state[action.message.id]) { - return state; - } - return { - ...state, - [action.message.id]: omit(action.message, 'flags'), - }; -}; - -const eventSubmessage = (state, action) => { - const message = state[action.message_id]; - if (!message) { - return state; - } - return { - ...state, - [action.message_id]: { - ...message, - submessages: [ - ...(message.submessages || []), - { - id: action.submessage_id, - message_id: action.message_id, - sender_id: action.sender_id, - msg_type: action.msg_type, - content: action.content, - }, - ], - }, - }; -}; - -const eventMessageDelete = (state, action) => { - if (action.messageIds.every(messageId => !state[messageId])) { + if (state.get(action.message.id)) { return state; } - return omit(state, action.messageIds); -}; - -const eventUpdateMessage = (state, action) => { - const oldMessage = state[action.message_id]; - if (!oldMessage) { - return state; - } - return { - ...state, - [action.message_id]: { - ...oldMessage, - content: action.rendered_content || oldMessage.content, - subject: action.subject || oldMessage.subject, - subject_links: action.subject_links || oldMessage.subject_links, - edit_history: [ - action.orig_rendered_content - ? action.orig_subject - ? { - prev_rendered_content: action.orig_rendered_content, - prev_subject: oldMessage.subject, - timestamp: action.edit_timestamp, - prev_rendered_content_version: action.prev_rendered_content_version, - user_id: action.user_id, - } - : { - prev_rendered_content: action.orig_rendered_content, - timestamp: action.edit_timestamp, - prev_rendered_content_version: action.prev_rendered_content_version, - user_id: action.user_id, - } - : { - prev_subject: oldMessage.subject, - timestamp: action.edit_timestamp, - user_id: action.user_id, - }, - ...(oldMessage.edit_history || NULL_ARRAY), - ], - last_edit_timestamp: action.edit_timestamp, - }, - }; + return state.set(action.message.id, omit(action.message, 'flags')); }; export default (state: MessagesState = initialState, action: Action): MessagesState => { @@ -146,33 +38,102 @@ export default (state: MessagesState = initialState, action: Action): MessagesSt return initialState; case MESSAGE_FETCH_COMPLETE: - return { - ...state, - // $FlowFixMe - Flow bug; should resolve in #4245 - ...groupItemsById( - action.messages.map(message => + return state.merge( + Immutable.Map( + action.messages.map(message => [ + message.id, omit(message, ['flags', 'match_content', 'match_subject']), - ), + ]), ), - }; + ); case EVENT_REACTION_ADD: - return eventReactionAdd(state, action); + return state.update( + action.message_id, + oldMessage => + oldMessage && { + ...oldMessage, + reactions: oldMessage.reactions.concat({ + emoji_name: action.emoji_name, + user_id: action.user_id, + reaction_type: action.reaction_type, + emoji_code: action.emoji_code, + }), + }, + ); case EVENT_REACTION_REMOVE: - return eventReactionRemove(state, action); + return state.update( + action.message_id, + oldMessage => + oldMessage && { + ...oldMessage, + reactions: oldMessage.reactions.filter( + x => !(x.emoji_name === action.emoji_name && x.user_id === action.user_id), + ), + }, + ); case EVENT_NEW_MESSAGE: return eventNewMessage(state, action); case EVENT_SUBMESSAGE: - return eventSubmessage(state, action); + return state.update( + action.message_id, + message => + message && { + ...message, + submessages: [ + ...(message.submessages || []), + { + id: action.submessage_id, + message_id: action.message_id, + sender_id: action.sender_id, + msg_type: action.msg_type, + content: action.content, + }, + ], + }, + ); case EVENT_MESSAGE_DELETE: - return eventMessageDelete(state, action); + return state.deleteAll(action.messageIds); case EVENT_UPDATE_MESSAGE: - return eventUpdateMessage(state, action); + return state.update( + action.message_id, + oldMessage => + oldMessage && { + ...oldMessage, + content: action.rendered_content || oldMessage.content, + subject: action.subject || oldMessage.subject, + subject_links: action.subject_links || oldMessage.subject_links, + edit_history: [ + action.orig_rendered_content + ? action.orig_subject + ? { + prev_rendered_content: action.orig_rendered_content, + prev_subject: oldMessage.subject, + timestamp: action.edit_timestamp, + prev_rendered_content_version: action.prev_rendered_content_version, + user_id: action.user_id, + } + : { + prev_rendered_content: action.orig_rendered_content, + timestamp: action.edit_timestamp, + prev_rendered_content_version: action.prev_rendered_content_version, + user_id: action.user_id, + } + : { + prev_subject: oldMessage.subject, + timestamp: action.edit_timestamp, + user_id: action.user_id, + }, + ...(oldMessage.edit_history || NULL_ARRAY), + ], + last_edit_timestamp: action.edit_timestamp, + }, + ); default: return state; diff --git a/src/reactions/MessageReactionList.js b/src/reactions/MessageReactionList.js index e8f507da7f4..0635814f801 100644 --- a/src/reactions/MessageReactionList.js +++ b/src/reactions/MessageReactionList.js @@ -145,7 +145,7 @@ class MessageReactionList extends PureComponent { export default connect((state, props) => ({ // message *can* be undefined; see componentDidUpdate for explanation and handling. - message: (state.messages[props.route.params.messageId]: Message | void), + message: state.messages.get(props.route.params.messageId), ownUserId: getOwnUser(state).user_id, allUsersById: getAllUsersById(state), }))(MessageReactionList); diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 1daca61ceac..85cdbb26b90 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -161,10 +161,7 @@ export type FlagName = $Keys; * See also `NarrowsState`, which is an index on this data that identifies * messages belonging to a given narrow. */ -export type MessagesState = { - // TODO(flow-v0.126): Should be exact. See note in src/utils/jsonable.js. - [id: number]: Message, -}; +export type MessagesState = Immutable.Map; export type MigrationsState = {| version?: string, diff --git a/src/unread/__tests__/unreadHuddlesReducer-test.js b/src/unread/__tests__/unreadHuddlesReducer-test.js index 51724dee6b8..b381fb2e10e 100644 --- a/src/unread/__tests__/unreadHuddlesReducer-test.js +++ b/src/unread/__tests__/unreadHuddlesReducer-test.js @@ -217,7 +217,7 @@ describe('unreadHuddlesReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: false, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [1, 2, 3], flag: 'star', operation: 'add', @@ -244,7 +244,7 @@ describe('unreadHuddlesReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: false, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [6, 7], flag: 'read', operation: 'add', @@ -271,7 +271,7 @@ describe('unreadHuddlesReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: false, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [3, 4, 5, 6], flag: 'read', operation: 'add', @@ -301,7 +301,7 @@ describe('unreadHuddlesReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: false, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [1, 2], flag: 'read', operation: 'remove', @@ -324,7 +324,7 @@ describe('unreadHuddlesReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: true, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [], flag: 'read', operation: 'add', diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js index 8bfc56ff1b2..80802bcae1b 100644 --- a/src/unread/__tests__/unreadModel-test.js +++ b/src/unread/__tests__/unreadModel-test.js @@ -135,7 +135,7 @@ describe('stream substate', () => { return { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, - allMessages: {}, + allMessages: eg.makeMessagesState([]), all, messages, flag, diff --git a/src/unread/__tests__/unreadPmsReducer-test.js b/src/unread/__tests__/unreadPmsReducer-test.js index d8708a589da..9ab1f95f3b2 100644 --- a/src/unread/__tests__/unreadPmsReducer-test.js +++ b/src/unread/__tests__/unreadPmsReducer-test.js @@ -197,7 +197,7 @@ describe('unreadPmsReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: false, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [1, 2, 3], flag: 'star', operation: 'add', @@ -224,7 +224,7 @@ describe('unreadPmsReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: false, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [6, 7], flag: 'read', operation: 'add', @@ -251,7 +251,7 @@ describe('unreadPmsReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: false, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [3, 4, 5, 6], flag: 'read', operation: 'add', @@ -281,7 +281,7 @@ describe('unreadPmsReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: false, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [1, 2], flag: 'read', operation: 'remove', @@ -304,7 +304,7 @@ describe('unreadPmsReducer', () => { id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, all: true, - allMessages: {}, + allMessages: eg.makeMessagesState([]), messages: [], flag: 'read', operation: 'add', diff --git a/src/utils/misc.js b/src/utils/misc.js index 598aa138bb4..6ecf7fac895 100644 --- a/src/utils/misc.js +++ b/src/utils/misc.js @@ -24,8 +24,4 @@ export function deeperMerge(obj1: { [K]: V }, obj2: { [K]: V }): { [K]: V ); } -export function groupItemsById(items: T[]): { [id: number]: T } { - return objectFromEntries(items.map(item => [item.id, item])); -} - export const isValidEmailFormat = (email: string = ''): boolean => /\S+@\S+\.\S+/.test(email);