From a67c782ca66199f6051cc4bb8898bee513445e7f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 20 Apr 2020 16:06:17 -0700 Subject: [PATCH] redux: Store `state.narrows` as an Immutable.Map, not object-as-map. An instance of #3949 and #3950. When I wrote the commit message for e834f335d, I was most interested in the exciting new ability to serialize/deserialize custom, hand-made data types: in particular, ZulipVersion instances. An important thing that *also* happened in that commit was the new ability to store data structures from Immutable in Redux, without any additional setup! `remotedev-serialize` comes with off-the-shelf support for this. I gave this fact a brief shout-out near the end of the commit message for a4c29e979. So, do this, for the first time, paying careful attention to the instructions in a4c29e979 for writing a migration (copied here, as that commit message is quite long): """ For making a value "live", where it wasn't before, the migration needs to: 1) As input, take the previous shape of the data. Don't confuse this with the *current* way of storing the "dead" shape. Just like any other migration, the previous shape is the input. 2) As output, give the "live" form of the data. Once it's in Redux, the replacer will take care of persisting it in the correct "dead" form. """ The previous shape of the data was an object-as-map, which `Immutable.Map` takes as input quite cheerfully, and gives us our "live" Immutable.Map instance. --- src/boot/store.js | 6 + src/chat/__tests__/narrowsReducer-test.js | 174 ++++++------------ src/chat/__tests__/narrowsSelectors-test.js | 34 ++-- src/chat/narrowsReducer.js | 48 +++-- src/chat/narrowsSelectors.js | 2 +- src/message/__tests__/fetchActions-test.js | 47 ++--- src/message/messageSelectors.js | 2 +- .../pmConversationsSelectors-test.js | 14 +- src/reduxTypes.js | 6 +- src/topics/__tests__/topicsSelectors-test.js | 8 +- 10 files changed, 147 insertions(+), 194 deletions(-) diff --git a/src/boot/store.js b/src/boot/store.js index f3691147bb0..14ef49e6871 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -197,6 +197,12 @@ const migrations: { [string]: (GlobalState) => GlobalState } = { })), }), + // Convert `narrows` from object-as-map to `Immutable.Map`. + '16': state => ({ + ...state, + narrows: Immutable.Map(state.narrows), + }), + // 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 29717049f5e..d96194dc754 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -1,5 +1,6 @@ /* @flow strict-local */ import deepFreeze from 'deep-freeze'; +import Immutable from 'immutable'; import narrowsReducer from '../narrowsReducer'; import { @@ -30,9 +31,7 @@ describe('narrowsReducer', () => { describe('EVENT_NEW_MESSAGE', () => { test('if not caught up in narrow, do not add message in home narrow', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); const message = eg.streamMessage({ id: 3, @@ -50,17 +49,13 @@ describe('narrowsReducer', () => { const newState = narrowsReducer(initialState, action); - const expectedState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - }); + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); expect(newState).toEqual(expectedState); }); test('appends message to state producing a copy of messages', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); const message = eg.streamMessage({ id: 3, flags: [] }); @@ -75,9 +70,9 @@ describe('narrowsReducer', () => { }, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3], - }; + }); const newState = narrowsReducer(initialState, action); @@ -86,9 +81,7 @@ describe('narrowsReducer', () => { }); test('if new message key does not exist do not create it', () => { - const initialState = deepFreeze({ - [topicNarrowStr]: [1, 2], - }); + const initialState = Immutable.Map({ [topicNarrowStr]: [1, 2] }); const message = eg.streamMessage({ id: 3, @@ -105,17 +98,15 @@ describe('narrowsReducer', () => { const newState = narrowsReducer(initialState, action); - const expectedState = { + const expectedState = Immutable.Map({ [topicNarrowStr]: [1, 2], - }; + }); expect(newState).toEqual(expectedState); }); }); test('if new message is private or group add it to the "allPrivate" narrow', () => { - const initialState = deepFreeze({ - [ALL_PRIVATE_NARROW_STR]: [], - }); + const initialState = Immutable.Map({ [ALL_PRIVATE_NARROW_STR]: [] }); const message = eg.pmMessage({ id: 1, display_recipient: [{ email: 'me@example.com' }, { email: 'a@a.com' }, { email: 'b@b.com' }], @@ -129,9 +120,9 @@ describe('narrowsReducer', () => { [ALL_PRIVATE_NARROW_STR]: { older: true, newer: true }, }, }); - const expectedState = { + const expectedState = Immutable.Map({ [ALL_PRIVATE_NARROW_STR]: [1], - }; + }); const actualState = narrowsReducer(initialState, action); @@ -139,10 +130,7 @@ describe('narrowsReducer', () => { }); test('message sent to topic is stored correctly', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - [topicNarrowStr]: [2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2], [topicNarrowStr]: [2] }); const message = eg.streamMessage({ id: 3, @@ -165,10 +153,10 @@ describe('narrowsReducer', () => { }, }, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2], [topicNarrowStr]: [2, message.id], - }; + }); const newState = narrowsReducer(initialState, action); @@ -177,10 +165,7 @@ describe('narrowsReducer', () => { test('message sent to self is stored correctly', () => { const narrowWithSelfStr = JSON.stringify(privateNarrow('me@example.com')); - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [], - [narrowWithSelfStr]: [], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [], [narrowWithSelfStr]: [] }); const message = eg.pmMessage({ id: 1, @@ -198,10 +183,10 @@ describe('narrowsReducer', () => { }, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [message.id], [narrowWithSelfStr]: [message.id], - }; + }); const newState = narrowsReducer(initialState, action); @@ -210,7 +195,7 @@ describe('narrowsReducer', () => { }); test('appends stream message to all cached narrows that match and are caught up', () => { - const initialState = deepFreeze({ + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2], [ALL_PRIVATE_NARROW_STR]: [1, 2], [streamNarrowStr]: [2, 3], @@ -236,14 +221,14 @@ describe('narrowsReducer', () => { }, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, message.id], [ALL_PRIVATE_NARROW_STR]: [1, 2], [streamNarrowStr]: [2, 3, message.id], [topicNarrowStr]: [2, 3, message.id], [privateNarrowStr]: [2, 4], [groupNarrowStr]: [2, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -252,9 +237,7 @@ describe('narrowsReducer', () => { }); test('does not append stream message to not cached narrows', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1] }); const message = eg.streamMessage({ id: 3, flags: [] }); @@ -266,9 +249,9 @@ describe('narrowsReducer', () => { }, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, message.id], - }; + }); const newState = narrowsReducer(initialState, action); @@ -277,7 +260,7 @@ describe('narrowsReducer', () => { }); test('appends private message to multiple cached narrows', () => { - const initialState = deepFreeze({ + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2], [ALL_PRIVATE_NARROW_STR]: [1, 2], [streamNarrowStr]: [2, 3], @@ -296,19 +279,17 @@ describe('narrowsReducer', () => { const action = deepFreeze({ ...eg.eventNewMessageActionBase, message, - caughtUp: Object.fromEntries( - Object.keys(initialState).map(key => [key, { older: false, newer: true }]), - ), + caughtUp: initialState.map(_ => ({ older: false, newer: true })).toObject(), }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, message.id], [ALL_PRIVATE_NARROW_STR]: [1, 2, message.id], [streamNarrowStr]: [2, 3], [topicNarrowStr]: [2, 3], [privateNarrowStr]: [2, 4, message.id], [groupNarrowStr]: [2, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -318,10 +299,7 @@ describe('narrowsReducer', () => { describe('EVENT_MESSAGE_DELETE', () => { test('if a message does not exist no changes are made', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - [privateNarrowStr]: [], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2], [privateNarrowStr]: [] }); const action = deepFreeze({ type: EVENT_MESSAGE_DELETE, @@ -334,18 +312,12 @@ describe('narrowsReducer', () => { }); test('if a message exists in one or more narrows delete it from there', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2, 3], - [privateNarrowStr]: [2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3], [privateNarrowStr]: [2] }); const action = deepFreeze({ type: EVENT_MESSAGE_DELETE, messageIds: [2], }); - const expectedState = deepFreeze({ - [HOME_NARROW_STR]: [1, 3], - [privateNarrowStr]: [], - }); + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 3], [privateNarrowStr]: [] }); const newState = narrowsReducer(initialState, action); @@ -353,18 +325,12 @@ describe('narrowsReducer', () => { }); test('if multiple messages indicated, delete the ones that exist', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2, 3], - [privateNarrowStr]: [2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3], [privateNarrowStr]: [2] }); const action = deepFreeze({ type: EVENT_MESSAGE_DELETE, messageIds: [2, 3, 4], }); - const expectedState = deepFreeze({ - [HOME_NARROW_STR]: [1], - [privateNarrowStr]: [], - }); + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1], [privateNarrowStr]: [] }); const newState = narrowsReducer(initialState, action); @@ -374,9 +340,7 @@ describe('narrowsReducer', () => { describe('MESSAGE_FETCH_START', () => { test('if fetching for a search narrow, ignore', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); const action = deepFreeze({ ...eg.action.message_fetch_start, @@ -395,7 +359,7 @@ describe('narrowsReducer', () => { // MESSAGE_FETCH_START applies the identity function to the // state (i.e., it doesn't do anything to it). Reversing that // effect is also done with the identity function. - const initialState = deepFreeze({ + const initialState = Immutable.Map({ [HOME_NARROW_STR]: { older: true, newer: true, @@ -423,9 +387,7 @@ describe('narrowsReducer', () => { describe('MESSAGE_FETCH_COMPLETE', () => { test('if no messages returned still create the key in state', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2, 3], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3] }); const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, @@ -438,10 +400,10 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3], [JSON.stringify(privateNarrow('mark@example.com'))]: [], - }; + }); const newState = narrowsReducer(initialState, action); @@ -449,9 +411,7 @@ describe('narrowsReducer', () => { }); test('no duplicate messages', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2, 3], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3] }); const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, @@ -468,9 +428,9 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -479,9 +439,7 @@ describe('narrowsReducer', () => { }); test('added messages are sorted by id', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, @@ -497,9 +455,9 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -508,9 +466,7 @@ describe('narrowsReducer', () => { }); test('when anchor is FIRST_UNREAD_ANCHOR previous messages are replaced', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); const action = deepFreeze({ anchor: FIRST_UNREAD_ANCHOR, @@ -526,9 +482,9 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [3, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -536,9 +492,7 @@ describe('narrowsReducer', () => { }); test('when anchor is LAST_MESSAGE_ANCHOR previous messages are replaced', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); const action = deepFreeze({ anchor: LAST_MESSAGE_ANCHOR, @@ -554,9 +508,9 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [3, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -564,9 +518,7 @@ describe('narrowsReducer', () => { }); test('if fetched messages are from a search narrow, ignore them', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); const action = deepFreeze({ ...eg.action.message_fetch_complete, @@ -589,9 +541,7 @@ describe('narrowsReducer', () => { }; test('Do nothing if the is:starred narrow has not been fetched', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: [1, 2], - }); + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }); const action = deepFreeze({ type: EVENT_UPDATE_MESSAGE_FLAGS, @@ -609,9 +559,7 @@ describe('narrowsReducer', () => { }); test("Do nothing if action.flag is not 'starred'", () => { - const initialState = deepFreeze({ - [STARRED_NARROW_STR]: [1, 2], - }); + const initialState = Immutable.Map({ [STARRED_NARROW_STR]: [1, 2] }); const action = deepFreeze({ type: EVENT_UPDATE_MESSAGE_FLAGS, @@ -632,9 +580,7 @@ describe('narrowsReducer', () => { 'Adds, while maintaining chronological order, ' + 'newly starred messages to the is:starred narrow', () => { - const initialState = deepFreeze({ - [STARRED_NARROW_STR]: [1, 3, 5], - }); + const initialState = Immutable.Map({ [STARRED_NARROW_STR]: [1, 3, 5] }); const action = deepFreeze({ type: EVENT_UPDATE_MESSAGE_FLAGS, @@ -646,9 +592,9 @@ describe('narrowsReducer', () => { messages: [4, 2], }); - const expectedState = { + const expectedState = Immutable.Map({ [STARRED_NARROW_STR]: [1, 2, 3, 4, 5], - }; + }); const newState = narrowsReducer(initialState, action); @@ -660,9 +606,7 @@ describe('narrowsReducer', () => { 'Removes, while maintaining chronological order, ' + 'newly unstarred messages from the is:starred narrow', () => { - const initialState = deepFreeze({ - [STARRED_NARROW_STR]: [1, 2, 3, 4, 5], - }); + const initialState = Immutable.Map({ [STARRED_NARROW_STR]: [1, 2, 3, 4, 5] }); const action = deepFreeze({ type: EVENT_UPDATE_MESSAGE_FLAGS, @@ -674,9 +618,9 @@ describe('narrowsReducer', () => { messages: [4, 2], }); - const expectedState = { + const expectedState = Immutable.Map({ [STARRED_NARROW_STR]: [1, 3, 5], - }; + }); const newState = narrowsReducer(initialState, action); diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 12205a382d4..d6a6517fe83 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -1,4 +1,6 @@ /* @flow strict-local */ +import Immutable from 'immutable'; + import { getFirstMessageId, getLastMessageId, @@ -29,9 +31,9 @@ describe('getMessagesForNarrow', () => { test('if no outbox messages returns messages with no change', () => { const state = eg.reduxState({ - narrows: { + narrows: Immutable.Map({ '[]': [123], - }, + }), messages, outbox: [], realm: eg.realmState({ email: eg.selfUser.email }), @@ -44,9 +46,9 @@ describe('getMessagesForNarrow', () => { test('combine messages and outbox in same narrow', () => { const state = eg.reduxState({ - narrows: { + narrows: Immutable.Map({ '[]': [123], - }, + }), messages, outbox: [outboxMessage], caughtUp: { @@ -62,9 +64,9 @@ describe('getMessagesForNarrow', () => { test('do not combine messages and outbox if not caught up', () => { const state = eg.reduxState({ - narrows: { + narrows: Immutable.Map({ [HOME_NARROW_STR]: [123], - }, + }), messages, outbox: [outboxMessage], realm: eg.realmState({ email: eg.selfUser.email }), @@ -77,9 +79,9 @@ describe('getMessagesForNarrow', () => { test('do not combine messages and outbox in different narrow', () => { const state = eg.reduxState({ - narrows: { + narrows: Immutable.Map({ [JSON.stringify(privateNarrow('john@example.com'))]: [123], - }, + }), messages, outbox: [outboxMessage], realm: eg.realmState({ email: eg.selfUser.email }), @@ -94,9 +96,9 @@ describe('getMessagesForNarrow', () => { describe('getFirstMessageId', () => { test('return undefined when there are no messages', () => { const state = eg.reduxState({ - narrows: { + narrows: Immutable.Map({ '[]': [], - }, + }), outbox: [], }); @@ -107,9 +109,9 @@ describe('getFirstMessageId', () => { test('returns first message id', () => { const state = eg.reduxState({ - narrows: { + narrows: Immutable.Map({ '[]': [1, 2, 3], - }, + }), messages: { [1]: eg.streamMessage({ id: 1 }) /* eslint-disable-line no-useless-computed-key */, [2]: eg.streamMessage({ id: 2 }) /* eslint-disable-line no-useless-computed-key */, @@ -127,9 +129,9 @@ describe('getFirstMessageId', () => { describe('getLastMessageId', () => { test('return undefined when there are no messages', () => { const state = eg.reduxState({ - narrows: { + narrows: Immutable.Map({ '[]': [], - }, + }), messages: {}, outbox: [], }); @@ -141,9 +143,9 @@ describe('getLastMessageId', () => { test('returns last message id', () => { const state = eg.reduxState({ - narrows: { + narrows: Immutable.Map({ '[]': [1, 2, 3], - }, + }), messages: { [1]: eg.streamMessage({ id: 1 }) /* eslint-disable-line no-useless-computed-key */, [2]: eg.streamMessage({ id: 2 }) /* eslint-disable-line no-useless-computed-key */, diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index c360d1d86a6..d88b0ad6f72 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -1,5 +1,6 @@ /* @flow strict-local */ import union from 'lodash.union'; +import Immutable from 'immutable'; import type { NarrowsState, Action } from '../types'; import { ensureUnreachable } from '../types'; @@ -22,9 +23,8 @@ import { STARRED_NARROW_STR, isSearchNarrow, } from '../utils/narrow'; -import { NULL_OBJECT } from '../nullObjects'; -const initialState: NarrowsState = NULL_OBJECT; +const initialState: NarrowsState = Immutable.Map(); const messageFetchComplete = (state, action) => { // We don't want to accumulate old searches that we'll never need again. @@ -35,62 +35,58 @@ const messageFetchComplete = (state, action) => { const fetchedMessageIds = action.messages.map(message => message.id); const replaceExisting = action.anchor === FIRST_UNREAD_ANCHOR || action.anchor === LAST_MESSAGE_ANCHOR; - return { - ...state, - [key]: replaceExisting + return state.set( + key, + replaceExisting ? fetchedMessageIds - : union(state[key], fetchedMessageIds).sort((a, b) => a - b), - }; + : union(state.get(key), fetchedMessageIds).sort((a, b) => a - b), + ); }; const eventNewMessage = (state, action) => { let stateChange = false; - const newState: NarrowsState = {}; - Object.keys(state).forEach(key => { + const newState = state.map((value, key) => { const { flags } = action.message; if (!flags) { throw new Error('EVENT_NEW_MESSAGE message missing flags'); } const isInNarrow = isMessageInNarrow(action.message, flags, JSON.parse(key), action.ownEmail); const isCaughtUp = action.caughtUp[key] && action.caughtUp[key].newer; - const messageDoesNotExist = state[key].find(id => action.message.id === id) === undefined; + const messageDoesNotExist = value.find(id => action.message.id === id) === undefined; if (isInNarrow && isCaughtUp && messageDoesNotExist) { stateChange = true; - newState[key] = [...state[key], action.message.id]; + return [...value, action.message.id]; } else { - newState[key] = state[key]; + return value; } }); + return stateChange ? newState : state; }; const eventMessageDelete = (state, action) => { let stateChange = false; - const newState: NarrowsState = {}; - Object.keys(state).forEach(key => { - newState[key] = state[key].filter(id => !action.messageIds.includes(id)); - stateChange = stateChange || newState[key].length < state[key].length; + const newState = state.map((value, key) => { + const result = value.filter(id => !action.messageIds.includes(id)); + stateChange = stateChange || result.length < value.length; + return result; }); return stateChange ? newState : state; }; const updateFlagNarrow = (state, narrowStr, operation, messageIds): NarrowsState => { - if (!state[narrowStr]) { + const value = state.get(narrowStr); + if (!value) { return state; } switch (operation) { - case 'add': - return { - ...state, - [narrowStr]: [...state[narrowStr], ...messageIds].sort((a, b) => a - b), - }; + case 'add': { + return state.set(narrowStr, [...value, ...messageIds].sort((a, b) => a - b)); + } case 'remove': { const messageIdSet = new Set(messageIds); - return { - ...state, - [narrowStr]: state[narrowStr].filter(id => !messageIdSet.has(id)), - }; + return state.set(narrowStr, value.filter(id => !messageIdSet.has(id))); } default: ensureUnreachable(operation); diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index 52ebb6fdcbd..a8b6c1419f5 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -56,7 +56,7 @@ export const outboxMessagesForNarrow: Selector = createSelecto ); export const getFetchedMessageIdsForNarrow = (state: GlobalState, narrow: Narrow) => - getAllNarrows(state)[JSON.stringify(narrow)] || NULL_ARRAY; + getAllNarrows(state).get(JSON.stringify(narrow)) || NULL_ARRAY; const getFetchedMessagesForNarrow: Selector = createSelector( getFetchedMessageIdsForNarrow, diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 071986d7a86..e19616626fb 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -2,6 +2,7 @@ import configureStore from 'redux-mock-store'; import thunk from 'redux-thunk'; import omit from 'lodash.omit'; +import Immutable from 'immutable'; import type { GlobalState } from '../../reduxTypes'; import type { Action } from '../../actionTypes'; @@ -40,9 +41,9 @@ describe('fetchActions', () => { older: true, }, }, - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [], - }, + }), streams: [eg.makeStream({ name: 'some stream' })], }); @@ -62,9 +63,9 @@ describe('fetchActions', () => { older: false, }, }, - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [1], - }, + }), messages: { [message1.id]: message1, [message2.id]: message2, @@ -114,9 +115,9 @@ describe('fetchActions', () => { const baseState = eg.reduxState({ accounts: [eg.makeAccount()], - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [message1.id], - }, + }), }); describe('success', () => { @@ -292,9 +293,9 @@ describe('fetchActions', () => { const store = mockStore( eg.reduxState({ accounts: [eg.selfAccount], - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [1], - }, + }), }), ); @@ -318,9 +319,9 @@ describe('fetchActions', () => { const store = mockStore( eg.reduxState({ accounts: [eg.selfAccount], - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [1], - }, + }), }), ); @@ -343,9 +344,9 @@ describe('fetchActions', () => { const store = mockStore( eg.reduxState({ accounts: [eg.selfAccount], - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [1], - }, + }), }), ); @@ -371,11 +372,12 @@ describe('fetchActions', () => { const baseState = eg.reduxState({ accounts: [eg.selfAccount], - narrows: { - ...eg.baseReduxState.narrows, - [streamNarrowStr]: [message2.id], - [HOME_NARROW_STR]: [message1.id, message2.id], - }, + narrows: eg.baseReduxState.narrows.merge( + Immutable.Map({ + [streamNarrowStr]: [message2.id], + [HOME_NARROW_STR]: [message1.id, message2.id], + }), + ), messages: { ...eg.baseReduxState.messages, [message1.id]: message1, @@ -464,11 +466,12 @@ describe('fetchActions', () => { const baseState = eg.reduxState({ accounts: [eg.selfAccount], - narrows: { - ...eg.baseReduxState.narrows, - [streamNarrowStr]: [message2.id], - [HOME_NARROW_STR]: [message1.id, message2.id], - }, + narrows: eg.baseReduxState.narrows.merge( + Immutable.Map({ + [streamNarrowStr]: [message2.id], + [HOME_NARROW_STR]: [message1.id, message2.id], + }), + ), messages: { ...eg.baseReduxState.messages, [message1.id]: message1, diff --git a/src/message/messageSelectors.js b/src/message/messageSelectors.js index af874bc3fc2..1dcefa2ac8c 100644 --- a/src/message/messageSelectors.js +++ b/src/message/messageSelectors.js @@ -39,7 +39,7 @@ export const getPrivateMessages: Selector = createSelector( const privateMessages: Message[] = []; const unknownIds: number[] = []; - const pmIds = narrows[ALL_PRIVATE_NARROW_STR] || NULL_ARRAY; + const pmIds = narrows.get(ALL_PRIVATE_NARROW_STR) || NULL_ARRAY; pmIds.forEach(id => { const msg = messages[id]; if (msg !== undefined) { diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index e9ea2d9ef21..7759bfa8e52 100644 --- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js +++ b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js @@ -1,4 +1,6 @@ /* @flow strict-local */ +import Immutable from 'immutable'; + import { getRecentConversations } from '../pmConversationsSelectors'; import { ALL_PRIVATE_NARROW_STR } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; @@ -11,9 +13,9 @@ describe('getRecentConversations', () => { const state = eg.reduxState({ realm: eg.realmState({ email: eg.selfUser.email }), users: [eg.selfUser], - narrows: { + narrows: Immutable.Map({ [ALL_PRIVATE_NARROW_STR]: [], - }, + }), unread: { ...eg.baseReduxState.unread, pms: [], @@ -36,7 +38,7 @@ describe('getRecentConversations', () => { const state = eg.reduxState({ realm: eg.realmState({ email: eg.selfUser.email }), users: [eg.selfUser, userJohn, userMark], - narrows: { + narrows: Immutable.Map({ [ALL_PRIVATE_NARROW_STR]: [ meJohnAndMarkPm.id, meAndJohnPm1.id, @@ -44,7 +46,7 @@ describe('getRecentConversations', () => { meAndJohnPm2.id, meOnlyPm.id, ], - }, + }), messages: { [meAndJohnPm1.id]: meAndJohnPm1, [meAndMarkPm.id]: meAndMarkPm, @@ -128,7 +130,7 @@ describe('getRecentConversations', () => { const state = eg.reduxState({ realm: eg.realmState({ email: eg.selfUser.email }), users: [eg.selfUser, userJohn, userMark], - narrows: { + narrows: Immutable.Map({ [ALL_PRIVATE_NARROW_STR]: [ meAndMarkPm1.id, meAndJohnPm1.id, @@ -137,7 +139,7 @@ describe('getRecentConversations', () => { meJohnAndMarkPm.id, meOnlyPm.id, ], - }, + }), messages: { [meAndJohnPm1.id]: meAndJohnPm1, [meAndMarkPm1.id]: meAndMarkPm1, diff --git a/src/reduxTypes.js b/src/reduxTypes.js index cf9a4f94b15..08d459d4e26 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -7,7 +7,7 @@ * * @flow strict-local */ - +import type Immutable from 'immutable'; import type { InputSelector } from 'reselect'; import type { Account, Outbox } from './types'; @@ -185,9 +185,7 @@ export type MuteState = MuteTuple[]; * * `FetchingState` for information about which narrows we're actively * fetching more messages from. */ -export type NarrowsState = { - [narrow: string]: number[], -}; +export type NarrowsState = Immutable.Map; export type NavigationRouteState = { key: string, diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index 65e3de63ad2..50729663700 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -1,4 +1,6 @@ /* @flow strict-local */ +import Immutable from 'immutable'; + import { getTopicsForNarrow, getLastMessageTopic, getTopicsForStream } from '../topicSelectors'; import { HOME_NARROW, streamNarrow } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; @@ -30,7 +32,7 @@ describe('getTopicsForNarrow', () => { describe('getLastMessageTopic', () => { test('when no messages in narrow return an empty string', () => { const state = eg.reduxState({ - narrows: {}, + narrows: Immutable.Map({}), realm: eg.realmState({ email: eg.selfUser.email }), }); @@ -44,9 +46,9 @@ describe('getLastMessageTopic', () => { const message1 = eg.streamMessage({ id: 1 }); const message2 = eg.streamMessage({ id: 2, subject: 'some topic' }); const state = eg.reduxState({ - narrows: { + narrows: Immutable.Map({ [JSON.stringify(narrow)]: [1, 2], - }, + }), messages: { [message1.id]: message1, [message2.id]: message2,