redux: Store state.messages as an Immutable.Map.

An instance of zulip#3949 and zulip#3950, like 17bd752. One important difference from 17bd752 is that the `state.messages` map has numeric keys. We've handled the replace/revive logic for that, in the previous commit. But care must be taken whenever we use an `Immutable.Map(...)` call when we want a map with numeric keys, If you pass an object-as-map to `Immutable.Map`, it's impossible for the resulting `Immutable.Map`'s keys to be numbers, because the object-as-map's keys are necessarily strings, and that's up to JavaScript. The solution is to pass an array of key-value pairs, but unfortunately, Flow won't catch it if you forget to do this.
chrisbobbe · Jan 7, 2021 · 4a82bac · 4a82bac
1 parent fa29620
commit 4a82bac
Show file tree

Hide file tree

Showing 9 changed files with 88 additions and 104 deletions.
diff --git 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/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__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js
@@ -39,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', () => {
@@ -74,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', () => {

diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js
@@ -64,7 +64,7 @@ const getFetchedMessagesForNarrow: Selector<Message[], Narrow> = createSelector(
   state => getMessages(state),
   (messageIds, messages) =>
     messageIds.map(id => {
-      const message = messages[id];
+      const message = messages.get(id);
       if (!message) {
         const msg = 'getFetchedMessagesForNarrow: message with id is missing in getMessages(state)';
         logging.error(msg, { id });

diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js
@@ -423,8 +423,8 @@ describe('messagesReducer', () => {
 
       const newState = messagesReducer(prevState, action);
 
-      expect(newState[message2.id]).toEqual(message2);
-      expect(newState[message3.id]).toEqual(message3);
+      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', () => {
@@ -446,9 +446,9 @@ describe('messagesReducer', () => {
         foundNewest: false,
       });
       const newState = messagesReducer(prevState, action);
-      expect(newState[message2.id]).toEqual(message2);
-      expect(newState[message3.id]).toEqual(message3);
-      expect(newState[message4New.id]).toEqual(message4New);
+      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
@@ -41,7 +41,7 @@ export const getPrivateMessages: Selector<Message[]> = 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
@@ -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,110 @@ 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 initialState: MessagesState = Immutable.Map([]);
 
 const eventReactionAdd = (state, action) => {
-  const oldMessage = state[action.message_id];
+  const oldMessage = state.get(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,
-      }),
-    },
-  };
+  return state.set(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];
+  const oldMessage = state.get(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),
-      ),
-    },
-  };
+  return state.set(action.message_id, {
+    ...oldMessage,
+    reactions: oldMessage.reactions.filter(
+      x => !(x.emoji_name === action.emoji_name && x.user_id === action.user_id),
+    ),
+  });
 };
 
 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]) {
+  if (state.get(action.message.id)) {
     return state;
   }
-  return {
-    ...state,
-    [action.message.id]: omit(action.message, 'flags'),
-  };
+  return state.set(action.message.id, omit(action.message, 'flags'));
 };
 
 const eventSubmessage = (state, action) => {
-  const message = state[action.message_id];
+  const message = state.get(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,
-        },
-      ],
-    },
-  };
+  return state.set(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 (action.messageIds.every(messageId => !state.get(messageId))) {
     return state;
   }
-  return omit(state, action.messageIds);
+  return state.deleteAll(action.messageIds);
 };
 
 const eventUpdateMessage = (state, action) => {
-  const oldMessage = state[action.message_id];
+  const oldMessage = state.get(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,
-              }
-          : {
+  return state.set(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,
-            },
-        ...(oldMessage.edit_history || NULL_ARRAY),
-      ],
-      last_edit_timestamp: action.edit_timestamp,
-    },
-  };
+            }
+        : {
+            prev_subject: oldMessage.subject,
+            timestamp: action.edit_timestamp,
+            user_id: action.user_id,
+          },
+      ...(oldMessage.edit_history || NULL_ARRAY),
+    ],
+    last_edit_timestamp: action.edit_timestamp,
+  });
 };
 
 export default (state: MessagesState = initialState, action: Action): MessagesState => {
@@ -146,15 +131,14 @@ 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);

diff --git a/src/reactions/MessageReactionList.js b/src/reactions/MessageReactionList.js
@@ -181,7 +181,7 @@ class MessageReactionList extends PureComponent<Props> {
 
 export default connect<SelectorProps, _, _>((state, props) => ({
   // message *can* be undefined; see componentDidUpdate for explanation and handling.
-  message: (state.messages[props.navigation.state.params.messageId]: Message | void),
+  message: (state.messages.get(props.navigation.state.params.messageId): Message | void),
   ownUserId: getOwnUser(state).user_id,
   allUsersById: getAllUsersById(state),
 }))(MessageReactionList);
diff --git a/src/reduxTypes.js b/src/reduxTypes.js
@@ -162,10 +162,7 @@ export type FlagName = $Keys<FlagsState>;
  * 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<number, Message>;
 
 export type MigrationsState = {|
   version?: string,
-Original file line number
+Diff line change
@@ Expand Up @@
         ),
       }),
+      // Convert `messages` from object-as-map to `Immutable.Map`.
+      '23': dropCache,
       // TIP: When adding a migration, consider just using `dropCache`.
     };
@@ Expand Down @@