From 0aa7705f2d3f406b8239a3fa74c56c50494b9d0e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 10 Dec 2020 15:04:14 -0500 Subject: [PATCH 01/10] unreadStreamsReducer tests: Start type-checking and use example data. And tweak a few tests to be a bit more straightforward. --- .../__tests__/unreadStreamsReducer-test.js | 129 +++++++++--------- 1 file changed, 64 insertions(+), 65 deletions(-) diff --git a/src/unread/__tests__/unreadStreamsReducer-test.js b/src/unread/__tests__/unreadStreamsReducer-test.js index c755ec07c74..b85b492c02d 100644 --- a/src/unread/__tests__/unreadStreamsReducer-test.js +++ b/src/unread/__tests__/unreadStreamsReducer-test.js @@ -1,13 +1,10 @@ +/* @flow strict-local */ import deepFreeze from 'deep-freeze'; import unreadStreamsReducer from '../unreadStreamsReducer'; -import { - REALM_INIT, - ACCOUNT_SWITCH, - EVENT_NEW_MESSAGE, - EVENT_UPDATE_MESSAGE_FLAGS, -} from '../../actionConstants'; +import { ACCOUNT_SWITCH, EVENT_UPDATE_MESSAGE_FLAGS } from '../../actionConstants'; import { NULL_ARRAY } from '../../nullObjects'; +import * as eg from '../../__tests__/lib/exampleData'; describe('unreadStreamsReducer', () => { describe('ACCOUNT_SWITCH', () => { @@ -22,6 +19,7 @@ describe('unreadStreamsReducer', () => { const action = deepFreeze({ type: ACCOUNT_SWITCH, + index: 1, }); const expectedState = []; @@ -36,29 +34,33 @@ describe('unreadStreamsReducer', () => { test('received data from "unread_msgs.streams" key replaces the current state ', () => { const initialState = deepFreeze([]); + const message1 = eg.streamMessage({ id: 1 }); + const action = deepFreeze({ - type: REALM_INIT, + ...eg.action.realm_init, data: { + ...eg.action.realm_init.data, unread_msgs: { + ...eg.action.realm_init.data.unread_msgs, streams: [ { - stream_id: 1, - topic: 'some topic', - unread_message_ids: [1, 2], + stream_id: message1.stream_id, + topic: message1.subject, + unread_message_ids: [message1.id, 2], }, ], - huddles: [{}, {}, {}], - pms: [{}, {}, {}], - mentions: [1, 2, 3], + huddles: [], + pms: [], + mentions: [message1.id, 2, 3], }, }, }); const expectedState = [ { - stream_id: 1, - topic: 'some topic', - unread_message_ids: [1, 2], + stream_id: message1.stream_id, + topic: message1.subject, + unread_message_ids: [message1.id, 2], }, ]; @@ -70,22 +72,18 @@ describe('unreadStreamsReducer', () => { describe('EVENT_NEW_MESSAGE', () => { test('if message id already exists, do not mutate state', () => { + const message1 = eg.streamMessage({ id: 1 }); const initialState = deepFreeze([ { - stream_id: 1, - topic: 'some topic', - unread_message_ids: [1, 2], + stream_id: message1.stream_id, + topic: message1.subject, + unread_message_ids: [message1.id], }, ]); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 1, - type: 'stream', - stream_id: 1, - subject: 'some topic', - }, + ...eg.eventNewMessageActionBase, + message: message1, }); const actualState = unreadStreamsReducer(initialState, action); @@ -94,20 +92,18 @@ describe('unreadStreamsReducer', () => { }); test('if message is not stream, return original state', () => { + const message4 = eg.pmMessage({ id: 4 }); const initialState = deepFreeze([ { - sender_id: 1, + stream_id: message4.stream_id, + topic: message4.subject, unread_message_ids: [1, 2, 3], }, ]); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 4, - type: 'private', - sender_id: 1, - }, + ...eg.eventNewMessageActionBase, + message: message4, }); const actualState = unreadStreamsReducer(initialState, action); @@ -117,17 +113,12 @@ describe('unreadStreamsReducer', () => { test('if message is sent by self, do not mutate state', () => { const initialState = deepFreeze([]); + const message1 = eg.streamMessage({ sender: eg.selfUser }); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 1, - type: 'stream', - stream_id: 2, - subject: 'another topic', - sender_email: 'me@example.com', - }, - ownEmail: 'me@example.com', + ...eg.eventNewMessageActionBase, + message: message1, + ownEmail: eg.selfUser.email, }); const actualState = unreadStreamsReducer(initialState, action); @@ -136,29 +127,26 @@ describe('unreadStreamsReducer', () => { }); test('if message id does not exist, append to state', () => { + const message4 = eg.streamMessage({ id: 4 }); + const initialState = deepFreeze([ { - stream_id: 1, - topic: 'some topic', + stream_id: message4.stream_id, + topic: message4.subject, unread_message_ids: [1, 2, 3], }, ]); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 4, - type: 'stream', - stream_id: 1, - subject: 'some topic', - }, + ...eg.eventNewMessageActionBase, + message: message4, }); const expectedState = [ { - stream_id: 1, - topic: 'some topic', - unread_message_ids: [1, 2, 3, 4], + stream_id: message4.stream_id, + topic: message4.subject, + unread_message_ids: [1, 2, 3, message4.id], }, ]; @@ -168,6 +156,8 @@ describe('unreadStreamsReducer', () => { }); test('if stream with topic does not exist, append to state', () => { + const message4 = eg.streamMessage({ id: 4, stream_id: 2, subject: 'another topic' }); + const initialState = deepFreeze([ { stream_id: 1, @@ -177,13 +167,8 @@ describe('unreadStreamsReducer', () => { ]); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 4, - type: 'stream', - stream_id: 2, - subject: 'another topic', - }, + ...eg.eventNewMessageActionBase, + message: message4, }); const expectedState = [ @@ -193,9 +178,9 @@ describe('unreadStreamsReducer', () => { unread_message_ids: [1, 2, 3], }, { - stream_id: 2, - topic: 'another topic', - unread_message_ids: [4], + stream_id: message4.stream_id, + topic: message4.subject, + unread_message_ids: [message4.id], }, ]; @@ -210,7 +195,10 @@ describe('unreadStreamsReducer', () => { const initialState = deepFreeze([]); const action = { + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [1, 2, 3], flag: 'star', operation: 'add', @@ -236,7 +224,10 @@ describe('unreadStreamsReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [6, 7], flag: 'read', operation: 'add', @@ -262,7 +253,10 @@ describe('unreadStreamsReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [3, 4, 5, 6], flag: 'read', operation: 'add', @@ -291,7 +285,10 @@ describe('unreadStreamsReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [1, 2], flag: 'read', operation: 'remove', @@ -312,11 +309,13 @@ describe('unreadStreamsReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: true, + allMessages: {}, messages: [], flag: 'read', operation: 'add', - all: true, }); const actualState = unreadStreamsReducer(initialState, action); From 44de5d65f6dcb2fe0f1a334c12b944679ba5ad18 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 10 Dec 2020 15:06:14 -0500 Subject: [PATCH 02/10] unreadPmsReducer tests: Start type-checking and use example data. And tweak a few tests to be a bit more straightforward. --- src/unread/__tests__/unreadPmsReducer-test.js | 122 ++++++++++-------- 1 file changed, 66 insertions(+), 56 deletions(-) diff --git a/src/unread/__tests__/unreadPmsReducer-test.js b/src/unread/__tests__/unreadPmsReducer-test.js index 5e3c5e4b48d..0c806eca633 100644 --- a/src/unread/__tests__/unreadPmsReducer-test.js +++ b/src/unread/__tests__/unreadPmsReducer-test.js @@ -1,13 +1,10 @@ +/* @flow strict-local */ import deepFreeze from 'deep-freeze'; import unreadPmsReducer from '../unreadPmsReducer'; -import { - REALM_INIT, - ACCOUNT_SWITCH, - EVENT_NEW_MESSAGE, - EVENT_UPDATE_MESSAGE_FLAGS, -} from '../../actionConstants'; +import { ACCOUNT_SWITCH, EVENT_UPDATE_MESSAGE_FLAGS } from '../../actionConstants'; import { NULL_ARRAY } from '../../nullObjects'; +import * as eg from '../../__tests__/lib/exampleData'; describe('unreadPmsReducer', () => { describe('ACCOUNT_SWITCH', () => { @@ -21,6 +18,7 @@ describe('unreadPmsReducer', () => { const action = deepFreeze({ type: ACCOUNT_SWITCH, + index: 1, }); const expectedState = []; @@ -35,27 +33,35 @@ describe('unreadPmsReducer', () => { test('received data from "unread_msgs.pms" key replaces the current state ', () => { const initialState = deepFreeze([]); + const message1 = eg.pmMessage({ id: 1, sender_id: 1 }); + const message2 = eg.pmMessage({ id: 2, sender_id: 1 }); + const message3 = eg.pmMessage({ id: 3, sender_id: 1 }); + const message4 = eg.pmMessage({ id: 4, sender_id: 1 }); + const message5 = eg.pmMessage({ id: 5, sender_id: 1 }); + const action = deepFreeze({ - type: REALM_INIT, + ...eg.action.realm_init, data: { + ...eg.action.realm_init.data, unread_msgs: { - streams: [{}, {}], - huddles: [{}, {}, {}], + ...eg.action.realm_init.data.unread_msgs, + streams: [], + huddles: [], pms: [ { - sender_id: 1, - unread_message_ids: [1, 2, 4, 5], + sender_id: message1.sender_id, + unread_message_ids: [message1.id, message2.id, message4.id, message5.id], }, ], - mentions: [1, 2, 3], + mentions: [message1.id, message2.id, message3.id], }, }, }); const expectedState = [ { - sender_id: 1, - unread_message_ids: [1, 2, 4, 5], + sender_id: message1.sender_id, + unread_message_ids: [message1.id, message2.id, message4.id, message5.id], }, ]; @@ -67,18 +73,17 @@ describe('unreadPmsReducer', () => { describe('EVENT_NEW_MESSAGE', () => { test('if message id already exists, do not mutate state', () => { + const message1 = eg.pmMessage({ sender_id: 1 }); const initialState = deepFreeze([ { - sender_id: 1, - unread_message_ids: [1, 2, 3], + sender_id: message1.sender_id, + unread_message_ids: [message1.id], }, ]); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 2, - }, + ...eg.eventNewMessageActionBase, + message: message1, }); const actualState = unreadPmsReducer(initialState, action); @@ -88,17 +93,15 @@ describe('unreadPmsReducer', () => { test('if message is sent by self, do not mutate state', () => { const initialState = deepFreeze([]); + const message1 = eg.pmMessage({ + sender: eg.selfUser, + recipients: [eg.otherUser, eg.selfUser], + }); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 2, - type: 'private', - sender_id: 1, - sender_email: 'me@example.com', - display_recipient: [{ email: 'john@example.com' }, { email: 'me@example.com' }], - }, - ownEmail: 'me@example.com', + ...eg.eventNewMessageActionBase, + message: message1, + ownEmail: eg.selfUser.email, }); const actualState = unreadPmsReducer(initialState, action); @@ -107,6 +110,7 @@ describe('unreadPmsReducer', () => { }); test('if message is not private, return original state', () => { + const message4 = eg.streamMessage({ id: 4 }); const initialState = deepFreeze([ { sender_id: 1, @@ -115,12 +119,8 @@ describe('unreadPmsReducer', () => { ]); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 4, - type: 'stream', - sender_id: 1, - }, + ...eg.eventNewMessageActionBase, + message: message4, }); const actualState = unreadPmsReducer(initialState, action); @@ -129,27 +129,27 @@ describe('unreadPmsReducer', () => { }); test('if message id does not exist, append to state', () => { + const message1 = eg.pmMessage({ id: 1, sender_id: 1 }); + const message2 = eg.pmMessage({ id: 2, sender_id: 1 }); + const message3 = eg.pmMessage({ id: 3, sender_id: 1 }); + const message4 = eg.pmMessage({ id: 4, sender_id: 1 }); + const initialState = deepFreeze([ { - sender_id: 1, - unread_message_ids: [1, 2, 3], + sender_id: message4.sender_id, + unread_message_ids: [message1.id, message2.id, message3.id], }, ]); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 4, - type: 'private', - sender_id: 1, - display_recipient: [{ email: 'john@example.com' }, { email: 'me@example.com' }], - }, + ...eg.eventNewMessageActionBase, + message: message4, }); const expectedState = [ { - sender_id: 1, - unread_message_ids: [1, 2, 3, 4], + sender_id: message4.sender_id, + unread_message_ids: [message1.id, message2.id, message3.id, message4.id], }, ]; @@ -159,6 +159,7 @@ describe('unreadPmsReducer', () => { }); test('if sender id does not exist, append to state as new sender', () => { + const message4 = eg.pmMessage({ id: 4, sender_id: 2 }); const initialState = deepFreeze([ { sender_id: 1, @@ -167,13 +168,8 @@ describe('unreadPmsReducer', () => { ]); const action = deepFreeze({ - type: EVENT_NEW_MESSAGE, - message: { - id: 4, - type: 'private', - sender_id: 2, - display_recipient: [{ email: 'john@example.com' }, { email: 'me@example.com' }], - }, + ...eg.eventNewMessageActionBase, + message: message4, }); const expectedState = [ @@ -182,8 +178,8 @@ describe('unreadPmsReducer', () => { unread_message_ids: [1, 2, 3], }, { - sender_id: 2, - unread_message_ids: [4], + sender_id: message4.sender_id, + unread_message_ids: [message4.id], }, ]; @@ -198,7 +194,10 @@ describe('unreadPmsReducer', () => { const initialState = deepFreeze([]); const action = { + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [1, 2, 3], flag: 'star', operation: 'add', @@ -222,7 +221,10 @@ describe('unreadPmsReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [6, 7], flag: 'read', operation: 'add', @@ -246,7 +248,10 @@ describe('unreadPmsReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [3, 4, 5, 6], flag: 'read', operation: 'add', @@ -273,7 +278,10 @@ describe('unreadPmsReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [1, 2], flag: 'read', operation: 'remove', @@ -293,11 +301,13 @@ describe('unreadPmsReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: true, + allMessages: {}, messages: [], flag: 'read', operation: 'add', - all: true, }); const actualState = unreadPmsReducer(initialState, action); From d1974490e897f248f190844d13f7b016228bc30f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 10 Dec 2020 15:06:52 -0500 Subject: [PATCH 03/10] unreadHuddlesReducer tests: Start type-checking and use example data. And tweak a few tests to be a bit more straightforward. --- .../__tests__/unreadHuddlesReducer-test.js | 123 ++++++++++-------- 1 file changed, 69 insertions(+), 54 deletions(-) diff --git a/src/unread/__tests__/unreadHuddlesReducer-test.js b/src/unread/__tests__/unreadHuddlesReducer-test.js index fbd73f180f6..3b12596f474 100644 --- a/src/unread/__tests__/unreadHuddlesReducer-test.js +++ b/src/unread/__tests__/unreadHuddlesReducer-test.js @@ -1,13 +1,14 @@ +/* @flow strict-local */ import deepFreeze from 'deep-freeze'; import unreadHuddlesReducer from '../unreadHuddlesReducer'; import { - REALM_INIT, ACCOUNT_SWITCH, EVENT_NEW_MESSAGE, EVENT_UPDATE_MESSAGE_FLAGS, } from '../../actionConstants'; import { NULL_ARRAY } from '../../nullObjects'; +import * as eg from '../../__tests__/lib/exampleData'; describe('unreadHuddlesReducer', () => { describe('ACCOUNT_SWITCH', () => { @@ -21,6 +22,7 @@ describe('unreadHuddlesReducer', () => { const action = deepFreeze({ type: ACCOUNT_SWITCH, + index: 1, }); const expectedState = []; @@ -36,17 +38,19 @@ describe('unreadHuddlesReducer', () => { const initialState = deepFreeze([]); const action = deepFreeze({ - type: REALM_INIT, + ...eg.action.realm_init, data: { + ...eg.action.realm_init.data, unread_msgs: { - streams: [{}, {}], + ...eg.action.realm_init.data.unread_msgs, + streams: [], huddles: [ { user_ids_string: '0,1,2', unread_message_ids: [1, 2, 4, 5], }, ], - pms: [{}, {}], + pms: [], mentions: [1, 2, 3], }, }, @@ -67,18 +71,25 @@ describe('unreadHuddlesReducer', () => { describe('EVENT_NEW_MESSAGE', () => { test('if message id already exists, do not mutate state', () => { + const user1 = { ...eg.makeUser(), user_id: 1 }; + const user2 = { ...eg.makeUser(), user_id: 2 }; + const user3 = { ...eg.makeUser(), user_id: 3 }; + + const message1 = eg.pmMessage({ id: 1, recipients: [user1, user2, user3] }); + const message2 = eg.pmMessage({ id: 2, recipients: [user1, user2, user3] }); + const message3 = eg.pmMessage({ id: 3, recipients: [user1, user2, user3] }); + const initialState = deepFreeze([ { - user_ids_string: '1,2,3', - unread_message_ids: [1, 2, 3], + user_ids_string: `${user1.user_id},${user2.user_id},${user3.user_id}`, + unread_message_ids: [message1.id, message2.id, message3.id], }, ]); const action = deepFreeze({ type: EVENT_NEW_MESSAGE, - message: { - id: 2, - }, + ...eg.eventNewMessageActionBase, + message: message2, }); const actualState = unreadHuddlesReducer(initialState, action); @@ -87,6 +98,7 @@ describe('unreadHuddlesReducer', () => { }); test('if message is not group, return original state', () => { + const streamMessage = eg.streamMessage(); const initialState = deepFreeze([ { user_ids_string: '1,2,3', @@ -96,15 +108,8 @@ describe('unreadHuddlesReducer', () => { const action = deepFreeze({ type: EVENT_NEW_MESSAGE, - message: { - id: 4, - type: 'stream', - sender_id: 1, - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - ], - }, + ...eg.eventNewMessageActionBase, + message: streamMessage, }); const actualState = unreadHuddlesReducer(initialState, action); @@ -113,22 +118,22 @@ describe('unreadHuddlesReducer', () => { }); test('if message is sent by self, do not mutate state', () => { + const selfUser = { ...eg.selfUser, user_id: 1 }; + const user2 = { ...eg.otherUser, user_id: 2 }; + const user3 = { ...eg.thirdUser, user_id: 3 }; + const initialState = deepFreeze([]); + const message2 = eg.pmMessage({ + sender: selfUser, + recipients: [selfUser, user2, user3], + }); + const action = deepFreeze({ type: EVENT_NEW_MESSAGE, - message: { - id: 2, - type: 'private', - sender_id: 1, - sender_email: 'me@example.com', - display_recipient: [ - { email: 'john@example.com' }, - { email: 'mark@example.com' }, - { email: 'me@example.com' }, - ], - }, - ownEmail: 'me@example.com', + ...eg.eventNewMessageActionBase, + message: message2, + ownEmail: selfUser.email, }); const actualState = unreadHuddlesReducer(initialState, action); @@ -137,29 +142,28 @@ describe('unreadHuddlesReducer', () => { }); test('if message id does not exist, append to state', () => { + const selfUser = { ...eg.selfUser, user_id: 1 }; + const user2 = { ...eg.otherUser, user_id: 2 }; + const user3 = { ...eg.thirdUser, user_id: 3 }; + + const message4 = eg.pmMessage({ id: 4, recipients: [selfUser, user2, user3] }); + const initialState = deepFreeze([ { - user_ids_string: '0,1,2', + user_ids_string: `${selfUser.user_id},${user2.user_id},${user3.user_id}`, unread_message_ids: [1, 2, 3], }, ]); const action = deepFreeze({ type: EVENT_NEW_MESSAGE, - message: { - id: 4, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, + ...eg.eventNewMessageActionBase, + message: message4, }); const expectedState = [ { - user_ids_string: '0,1,2', + user_ids_string: `${selfUser.user_id},${user2.user_id},${user3.user_id}`, unread_message_ids: [1, 2, 3, 4], }, ]; @@ -169,7 +173,12 @@ describe('unreadHuddlesReducer', () => { expect(actualState).toEqual(expectedState); }); - test('if sender id does not exist, append to state as new sender', () => { + test('if sender-ids string does not exist, append to state as new', () => { + const user1 = { ...eg.selfUser, user_id: 1 }; + const user2 = { ...eg.otherUser, user_id: 2 }; + const user3 = { ...eg.thirdUser, user_id: 3 }; + + const message4 = eg.pmMessage({ id: 4, recipients: [user1, user2, user3] }); const initialState = deepFreeze([ { user_ids_string: '0,3,4', @@ -179,16 +188,8 @@ describe('unreadHuddlesReducer', () => { const action = deepFreeze({ type: EVENT_NEW_MESSAGE, - message: { - id: 4, - type: 'private', - sender_id: 2, - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, + ...eg.eventNewMessageActionBase, + message: message4, }); const expectedState = [ @@ -197,7 +198,7 @@ describe('unreadHuddlesReducer', () => { unread_message_ids: [1, 2, 3], }, { - user_ids_string: '0,1,2', + user_ids_string: `${user1.user_id},${user2.user_id},${user3.user_id}`, unread_message_ids: [4], }, ]; @@ -213,7 +214,10 @@ describe('unreadHuddlesReducer', () => { const initialState = deepFreeze([]); const action = { + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [1, 2, 3], flag: 'star', operation: 'add', @@ -237,7 +241,10 @@ describe('unreadHuddlesReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [6, 7], flag: 'read', operation: 'add', @@ -261,7 +268,10 @@ describe('unreadHuddlesReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [3, 4, 5, 6], flag: 'read', operation: 'add', @@ -288,7 +298,10 @@ describe('unreadHuddlesReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: false, + allMessages: {}, messages: [1, 2], flag: 'read', operation: 'remove', @@ -308,11 +321,13 @@ describe('unreadHuddlesReducer', () => { ]); const action = deepFreeze({ + id: 1, type: EVENT_UPDATE_MESSAGE_FLAGS, + all: true, + allMessages: {}, messages: [], flag: 'read', operation: 'add', - all: true, }); const actualState = unreadHuddlesReducer(initialState, action); From db801940a2a0be3c55eacf954d04f3f4cf036f06 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 10 Dec 2020 15:46:18 -0500 Subject: [PATCH 04/10] unreadPmsReducer tests [nfc]: Reorder two tests. Unsurprisingly, these tests are pretty similar to the tests for `unreadStreamsReducer` and `unreadHuddlesReducer`. But these two don't match the order in the other two. Might as well fix this small inconsistency now that we've spotted it. --- src/unread/__tests__/unreadPmsReducer-test.js | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/unread/__tests__/unreadPmsReducer-test.js b/src/unread/__tests__/unreadPmsReducer-test.js index 0c806eca633..285b81b9bb1 100644 --- a/src/unread/__tests__/unreadPmsReducer-test.js +++ b/src/unread/__tests__/unreadPmsReducer-test.js @@ -91,17 +91,18 @@ describe('unreadPmsReducer', () => { expect(actualState).toBe(initialState); }); - test('if message is sent by self, do not mutate state', () => { - const initialState = deepFreeze([]); - const message1 = eg.pmMessage({ - sender: eg.selfUser, - recipients: [eg.otherUser, eg.selfUser], - }); + test('if message is not private, return original state', () => { + const message4 = eg.streamMessage({ id: 4 }); + const initialState = deepFreeze([ + { + sender_id: 1, + unread_message_ids: [1, 2, 3], + }, + ]); const action = deepFreeze({ ...eg.eventNewMessageActionBase, - message: message1, - ownEmail: eg.selfUser.email, + message: message4, }); const actualState = unreadPmsReducer(initialState, action); @@ -109,18 +110,17 @@ describe('unreadPmsReducer', () => { expect(actualState).toBe(initialState); }); - test('if message is not private, return original state', () => { - const message4 = eg.streamMessage({ id: 4 }); - const initialState = deepFreeze([ - { - sender_id: 1, - unread_message_ids: [1, 2, 3], - }, - ]); + test('if message is sent by self, do not mutate state', () => { + const initialState = deepFreeze([]); + const message1 = eg.pmMessage({ + sender: eg.selfUser, + recipients: [eg.otherUser, eg.selfUser], + }); const action = deepFreeze({ ...eg.eventNewMessageActionBase, - message: message4, + message: message1, + ownEmail: eg.selfUser.email, }); const actualState = unreadPmsReducer(initialState, action); From 17bd75231c204ab40cd1f850d80de484f38b955e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 20 Apr 2020 16:06:17 -0700 Subject: [PATCH 05/10] 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 | 171 ++++++------------ 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(+), 191 deletions(-) diff --git a/src/boot/store.js b/src/boot/store.js index 2d26c347899..47c85a9e804 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -199,6 +199,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 9eec8565ff1..337af9263f7 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 { @@ -32,9 +33,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, flags: [] }); @@ -45,17 +44,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: [] }); @@ -70,9 +65,9 @@ describe('narrowsReducer', () => { }, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3], - }; + }); const newState = narrowsReducer(initialState, action); @@ -81,9 +76,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, flags: [], stream: eg.makeStream() }); @@ -94,17 +87,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, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser], @@ -117,9 +108,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); @@ -127,10 +118,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, flags: [] }); @@ -148,10 +136,10 @@ describe('narrowsReducer', () => { }, }, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2], [topicNarrowStr]: [2, message.id], - }; + }); const newState = narrowsReducer(initialState, action); @@ -160,7 +148,7 @@ describe('narrowsReducer', () => { test('message sent to self is stored correctly', () => { const narrowWithSelfStr = JSON.stringify(pmNarrowFromEmail(eg.selfUser.email)); - const initialState = deepFreeze({ + const initialState = Immutable.Map({ [HOME_NARROW_STR]: [], [narrowWithSelfStr]: [], }); @@ -181,10 +169,10 @@ describe('narrowsReducer', () => { }, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [message.id], [narrowWithSelfStr]: [message.id], - }; + }); const newState = narrowsReducer(initialState, action); @@ -193,7 +181,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], @@ -214,14 +202,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); @@ -230,9 +218,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: [] }); @@ -244,9 +230,9 @@ describe('narrowsReducer', () => { }, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, message.id], - }; + }); const newState = narrowsReducer(initialState, action); @@ -255,7 +241,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], @@ -274,19 +260,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); @@ -296,10 +280,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, @@ -312,18 +293,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); @@ -331,18 +306,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); @@ -352,9 +321,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, @@ -373,7 +340,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, @@ -401,9 +368,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, @@ -416,10 +381,10 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3], [JSON.stringify(pmNarrowFromEmail(eg.otherUser.email))]: [], - }; + }); const newState = narrowsReducer(initialState, action); @@ -427,9 +392,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, @@ -446,9 +409,9 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -457,9 +420,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, @@ -475,9 +436,9 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -486,9 +447,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, @@ -504,9 +463,9 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [3, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -514,9 +473,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, @@ -532,9 +489,9 @@ describe('narrowsReducer', () => { foundOldest: false, }); - const expectedState = { + const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [3, 4], - }; + }); const newState = narrowsReducer(initialState, action); @@ -542,9 +499,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, @@ -567,9 +522,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, @@ -587,9 +540,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, @@ -610,9 +561,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, @@ -624,9 +573,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); @@ -638,9 +587,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, @@ -652,9 +599,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 420edcca029..3ffc90b4bea 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(pmNarrowFromEmail('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 7ffb100b8f8..192c28c2e4d 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -57,7 +57,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 52893aa1486..04251957e77 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -3,6 +3,7 @@ import invariant from 'invariant'; 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'; @@ -41,9 +42,9 @@ describe('fetchActions', () => { older: true, }, }, - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [], - }, + }), streams: [eg.makeStream({ name: 'some stream' })], }); @@ -63,9 +64,9 @@ describe('fetchActions', () => { older: false, }, }, - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [1], - }, + }), messages: { [message1.id]: message1, [message2.id]: message2, @@ -115,9 +116,9 @@ describe('fetchActions', () => { const baseState = eg.reduxState({ accounts: [eg.makeAccount()], - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [message1.id], - }, + }), }); describe('success', () => { @@ -293,9 +294,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], - }, + }), }), ); @@ -342,9 +343,9 @@ describe('fetchActions', () => { const store = mockStore( eg.reduxState({ accounts: [eg.selfAccount], - narrows: { + narrows: Immutable.Map({ [streamNarrowStr]: [1], - }, + }), }), ); @@ -369,11 +370,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, @@ -461,11 +463,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, From f7f34d00da36e5714d3254638b0edbd7408777d2 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 18 Nov 2020 11:05:09 -0800 Subject: [PATCH 06/10] deps: Add `immutable-devtools` and document its simple setup. In a recent commit, we converted `state.narrows` from a plain object-as-map into an `Immutable.Map`. This will make it convenient to inspect its data in Chrome. --- docs/howto/debugging.md | 28 +++++++++++++++++ flow-typed/npm/immutable-devtools_vx.x.x.js | 35 +++++++++++++++++++++ package.json | 1 + src/boot/store.js | 12 +++++++ yarn.lock | 5 +++ 5 files changed, 81 insertions(+) create mode 100644 flow-typed/npm/immutable-devtools_vx.x.x.js diff --git a/docs/howto/debugging.md b/docs/howto/debugging.md index 5887243db54..50a14055700 100644 --- a/docs/howto/debugging.md +++ b/docs/howto/debugging.md @@ -14,6 +14,7 @@ A variety of tools are available to help us do that. * ... [with React DevTools](#react-devtools) * ... [with Reactotron](#reactotron) * ... [with redux-logger](#redux-logger) + * ... [with immutable-devtools](#immutable-devtools) * [Debugging the message list](#webview) in its WebView * ... on iOS, [with Safari developer tools](#webview-safari) * ... on Android, [with the Chrome Developer Tools](#webview-chrome) @@ -158,6 +159,33 @@ call to `createLogger` in `src/boot/middleware.js`. doc](https://github.com/evgenyrodionov/redux-logger#options). +
+ +## immutable-devtools: inspect Immutable.js objects in Chrome + +Some of the data in the Redux state is stored in Immutable.js data +structures. It's normally awkward to inspect these data structures; an +`Immutable.Map` object, for example, is full of properties like +`size`, `__altered`, `__hash`, and `__ownerID`, which you have to dig +around to get at a clear representation of the items contained. + +[`immutable-devtools`](https://github.com/andrewdavey/immutable-devtools) +fixes this problem in Google Chrome v47+ by installing a "custom +formatter". + +(Alternatively, we might have recommended a [Chrome +extension](https://github.com/mattzeunert/immutable-object-formatter-extension), +which `immutable-devtools` mentions in its setup doc. But using the +NPM package provides the formatter for `zulip-mobile` just as +effectively without making widespread changes in behavior when you +browse the Web.) + +To enable it, open Chrome Dev Tools and press F1 to open the settings. +In the "Console" section, tick "Enable custom formatters". If it isn't +working, please consult the project's issue tracker before opening an +issue in `zulip-mobile`. + +
# Tools: Debugging the message list (in WebView) diff --git a/flow-typed/npm/immutable-devtools_vx.x.x.js b/flow-typed/npm/immutable-devtools_vx.x.x.js new file mode 100644 index 00000000000..e26a0ebc482 --- /dev/null +++ b/flow-typed/npm/immutable-devtools_vx.x.x.js @@ -0,0 +1,35 @@ +// flow-typed signature: 14523166b9c09a9ed88bca4845a653b1 +// flow-typed version: <>/immutable-devtools_v0.1.5/flow_v0.113.0 + +/** + * This is an autogenerated libdef stub for: + * + * 'immutable-devtools' + * + * Fill this stub out by replacing all the `any` types. + * + * Once filled out, we encourage you to share your work with the + * community by sending a pull request to: + * https://github.com/flowtype/flow-typed + */ + +declare module 'immutable-devtools' { + declare module.exports: any; +} + +/** + * We include stubs for each file inside this npm package in case you need to + * require those files directly. Feel free to delete any files that aren't + * needed. + */ +declare module 'immutable-devtools/dist' { + declare module.exports: any; +} + +// Filename aliases +declare module 'immutable-devtools/dist/index' { + declare module.exports: $Exports<'immutable-devtools/dist'>; +} +declare module 'immutable-devtools/dist/index.js' { + declare module.exports: $Exports<'immutable-devtools/dist'>; +} diff --git a/package.json b/package.json index da0bf7ccb0e..97a00a5a1a3 100644 --- a/package.json +++ b/package.json @@ -120,6 +120,7 @@ "flow-bin": "^0.113.0", "flow-coverage-report": "^0.6.0", "flow-typed": "^2.4.0", + "immutable-devtools": "^0.1.5", "jest": "^26.4.1", "jest-cli": "^26.4.1", "jest-environment-jsdom": "^26.3.0", diff --git a/src/boot/store.js b/src/boot/store.js index 47c85a9e804..e978ff1da1c 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -19,6 +19,18 @@ import createMigration from '../redux-persist-migrate/index'; import { provideLoggingContext } from './loggingContext'; import { tryGetActiveAccount } from '../account/accountsSelectors'; +if (process.env.NODE_ENV === 'development') { + // Chrome dev tools for Immutable. + // + // To enable, press F1 from the Chrome dev tools to open the + // settings. In the "Console" section, check "Enable custom + // formatters". + // + // eslint-disable-next-line import/no-extraneous-dependencies, global-require + const installDevTools = require('immutable-devtools'); + installDevTools(Immutable); +} + // AsyncStorage.clear(); // use to reset storage during development /** diff --git a/yarn.lock b/yarn.lock index 1811a4d59cd..6355be3d68d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6154,6 +6154,11 @@ image-size@^0.6.0: resolved "https://registry.yarnpkg.com/image-size/-/image-size-0.6.3.tgz#e7e5c65bb534bd7cdcedd6cb5166272a85f75fb2" integrity sha512-47xSUiQioGaB96nqtp5/q55m0aBQSQdyIloMOc/x+QVTDZLNmXE892IIDrJ0hM1A5vcNUDD5tDffkSP5lCaIIA== +immutable-devtools@^0.1.5: + version "0.1.5" + resolved "https://registry.yarnpkg.com/immutable-devtools/-/immutable-devtools-0.1.5.tgz#32a653c8ba8258bfed37680a860a3b5fa2675693" + integrity sha512-bgQP4q+RiD1Oolw8c0sfNrCpShQIEdqJIGmPPrcG6efyJrX00hNzzM1noe3FsQdDwj2eQqL8JEtukCrwFQbt/w== + immutable@^4.0.0-rc.12: version "4.0.0-rc.12" resolved "https://registry.yarnpkg.com/immutable/-/immutable-4.0.0-rc.12.tgz#ca59a7e4c19ae8d9bf74a97bdf0f6e2f2a5d0217" From 46969516a9d5ac8c4e526ea03ef0dd83af501249 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 10 Dec 2020 15:07:22 -0500 Subject: [PATCH 07/10] EVENT_NEW_MESSAGE: Take the whole `ownUser` instead of `ownEmail`. Not for storing anywhere, just for giving us what we need to handle EVENT_NEW_MESSAGE a bit differently in `narrowsReducer` in an upcoming commit. We'll be calling the recently added `getNarrowsForMessage`, which takes `ownUser`. --- src/__tests__/lib/exampleData.js | 2 +- src/actionTypes.js | 2 +- src/chat/narrowsReducer.js | 7 ++++++- src/events/eventToAction.js | 4 ++-- src/unread/__tests__/unreadHuddlesReducer-test.js | 2 +- src/unread/__tests__/unreadPmsReducer-test.js | 2 +- src/unread/__tests__/unreadStreamsReducer-test.js | 2 +- src/unread/unreadHuddlesReducer.js | 2 +- src/unread/unreadPmsReducer.js | 2 +- src/unread/unreadStreamsReducer.js | 2 +- 10 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 3720c191363..8f7a2f3d48d 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -576,5 +576,5 @@ export const eventNewMessageActionBase /* \: $Diff, type: typeof EVENT_NEW_MESSAGE, caughtUp: CaughtUpState, - ownEmail: string, + ownUser: User, |}; type EventSubmessageAction = {| diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index d88b0ad6f72..15b20bdbc15 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -50,7 +50,12 @@ const eventNewMessage = (state, action) => { if (!flags) { throw new Error('EVENT_NEW_MESSAGE message missing flags'); } - const isInNarrow = isMessageInNarrow(action.message, flags, JSON.parse(key), action.ownEmail); + const isInNarrow = isMessageInNarrow( + action.message, + flags, + JSON.parse(key), + action.ownUser.email, + ); const isCaughtUp = action.caughtUp[key] && action.caughtUp[key].newer; const messageDoesNotExist = value.find(id => action.message.id === id) === undefined; diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index ac5410bbfad..ddc74f49e72 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -31,7 +31,7 @@ import { EVENT_SUBSCRIPTION, EVENT, } from '../actionConstants'; -import { getOwnEmail, getOwnUserId } from '../users/userSelectors'; +import { getOwnUser, getOwnUserId } from '../users/userSelectors'; const opToActionUserGroup = { add: EVENT_USER_GROUP_ADD, @@ -86,7 +86,7 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { }, local_message_id: event.local_message_id, caughtUp: state.caughtUp, - ownEmail: getOwnEmail(state), + ownUser: getOwnUser(state), }; // Before server feature level 13, or if we don't specify the diff --git a/src/unread/__tests__/unreadHuddlesReducer-test.js b/src/unread/__tests__/unreadHuddlesReducer-test.js index 3b12596f474..51724dee6b8 100644 --- a/src/unread/__tests__/unreadHuddlesReducer-test.js +++ b/src/unread/__tests__/unreadHuddlesReducer-test.js @@ -133,7 +133,7 @@ describe('unreadHuddlesReducer', () => { type: EVENT_NEW_MESSAGE, ...eg.eventNewMessageActionBase, message: message2, - ownEmail: selfUser.email, + ownUser: selfUser, }); const actualState = unreadHuddlesReducer(initialState, action); diff --git a/src/unread/__tests__/unreadPmsReducer-test.js b/src/unread/__tests__/unreadPmsReducer-test.js index 285b81b9bb1..d8708a589da 100644 --- a/src/unread/__tests__/unreadPmsReducer-test.js +++ b/src/unread/__tests__/unreadPmsReducer-test.js @@ -120,7 +120,7 @@ describe('unreadPmsReducer', () => { const action = deepFreeze({ ...eg.eventNewMessageActionBase, message: message1, - ownEmail: eg.selfUser.email, + ownUser: eg.selfUser, }); const actualState = unreadPmsReducer(initialState, action); diff --git a/src/unread/__tests__/unreadStreamsReducer-test.js b/src/unread/__tests__/unreadStreamsReducer-test.js index b85b492c02d..548694bbca5 100644 --- a/src/unread/__tests__/unreadStreamsReducer-test.js +++ b/src/unread/__tests__/unreadStreamsReducer-test.js @@ -118,7 +118,7 @@ describe('unreadStreamsReducer', () => { const action = deepFreeze({ ...eg.eventNewMessageActionBase, message: message1, - ownEmail: eg.selfUser.email, + ownUser: eg.selfUser, }); const actualState = unreadStreamsReducer(initialState, action); diff --git a/src/unread/unreadHuddlesReducer.js b/src/unread/unreadHuddlesReducer.js index d74e62bee92..a8fe4dbae4f 100644 --- a/src/unread/unreadHuddlesReducer.js +++ b/src/unread/unreadHuddlesReducer.js @@ -23,7 +23,7 @@ const eventNewMessage = (state, action) => { return state; } - if (action.ownEmail && action.ownEmail === action.message.sender_email) { + if (action.ownUser.email && action.ownUser.email === action.message.sender_email) { return state; } diff --git a/src/unread/unreadPmsReducer.js b/src/unread/unreadPmsReducer.js index 7b14784a342..ab3426cc1bb 100644 --- a/src/unread/unreadPmsReducer.js +++ b/src/unread/unreadPmsReducer.js @@ -23,7 +23,7 @@ const eventNewMessage = (state, action) => { return state; } - if (action.ownEmail && action.ownEmail === action.message.sender_email) { + if (action.ownUser.email && action.ownUser.email === action.message.sender_email) { return state; } diff --git a/src/unread/unreadStreamsReducer.js b/src/unread/unreadStreamsReducer.js index b338ba274a8..1a34ace84d8 100644 --- a/src/unread/unreadStreamsReducer.js +++ b/src/unread/unreadStreamsReducer.js @@ -18,7 +18,7 @@ const eventNewMessage = (state, action) => { return state; } - if (action.ownEmail && action.ownEmail === action.message.sender_email) { + if (action.ownUser.email && action.ownUser.email === action.message.sender_email) { return state; } From cefc9578339d1384212e11e2cdb5a1da616ca3ca Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 10 Dec 2020 15:34:19 -0500 Subject: [PATCH 08/10] narrowsReducer: Fix a perf regression in EVENT_NEW_MESSAGE handling. Thanks to Greg for the analysis, which I'm summarizing here, and the suggested solution. For more background, see discussion [1]. When we started using `Immutable.Map` for `state.narrows` in a recent commit, there was a performance regression on handling EVENT_NEW_MESSAGE when `state.narrows` has very many keys (we tested with 1000). We left the regression in, until now, so that commit could remain a direct, easy-to-follow translation from plain object-as-map to `Immutable.Map`. Although using `Immutable.Map` the way we did had the advantage (over an object-as-map, or over an ES6 `Map`) of not having to allocate a whole new copy of `state.narrows` just to change one thing, it still meant *iterating* through all the narrows to check whether we needed to update each one. So the work being done was still linear with the number of narrows, even supposing (which is very reasonable) that the constant factor for read-only iterations was much smaller than for iterations that had to allocate. Greg then tried the approach in this commit, which isn't linear in the number of narrows, and he found that it was significantly faster for 1000 narrows. The key fact is that you can look at a message and tell what narrows it belongs in, and that's always a very low number (like five) [2]. It also makes an improvement that's orthogonal to `Immutable.Map` vs. objects-as-map vs. `Map`: on an iteration, we should skip the `messageDoesNotExist` computation (which goes and checks all items in the array) if `isCaughtUp` has already indicated we're not interested in that narrow [3]. See his observations for 1000 narrows [4] and for (the much more usual) 30 narrows [5], where the approach in this commit still shows up as faster than the object-as-map approach before this series. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60Immutable.2EMap.60.20performance.20for.20.60state.2Enarrows.60/near/1068889 [2] As long as you exclude search narrows, which there's a pretty much unbounded number of -- but we've never bothered live-updating them with message events anyway. [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60Immutable.2EMap.60.20performance.20for.20.60state.2Enarrows.60/near/1067739 [4] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60Immutable.2EMap.60.20performance.20for.20.60state.2Enarrows.60/near/1068895 [5] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60Immutable.2EMap.60.20performance.20for.20.60state.2Enarrows.60/near/1068917 --- src/chat/narrowsReducer.js | 57 ++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index 15b20bdbc15..ca80cadeee9 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -18,7 +18,7 @@ import { } from '../actionConstants'; import { LAST_MESSAGE_ANCHOR, FIRST_UNREAD_ANCHOR } from '../anchor'; import { - isMessageInNarrow, + getNarrowsForMessage, MENTIONED_NARROW_STR, STARRED_NARROW_STR, isSearchNarrow, @@ -44,30 +44,39 @@ const messageFetchComplete = (state, action) => { }; const eventNewMessage = (state, action) => { - let stateChange = false; - 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.ownUser.email, - ); - const isCaughtUp = action.caughtUp[key] && action.caughtUp[key].newer; - const messageDoesNotExist = value.find(id => action.message.id === id) === undefined; - - if (isInNarrow && isCaughtUp && messageDoesNotExist) { - stateChange = true; - return [...value, action.message.id]; - } else { - return value; - } - }); + const { message } = action; + const { flags } = message; - return stateChange ? newState : state; + if (!flags) { + throw new Error('EVENT_NEW_MESSAGE message missing flags'); + } + + return state.withMutations(stateMutable => { + const narrowsForMessage = getNarrowsForMessage(message, action.ownUser, flags); + + narrowsForMessage.forEach(narrow => { + const key = JSON.stringify(narrow); + const value = stateMutable.get(key); + + if (!value) { + // We haven't loaded this narrow. The time to add a new key + // isn't now; we do that in MESSAGE_FETCH_COMPLETE, when we + // might have a reasonably long, contiguous list of messages + // to show. + return; // i.e., continue + } + + if (!(action.caughtUp[key] && action.caughtUp[key].newer)) { + return; // i.e., continue + } + + if (!(value.find(id => action.message.id === id) === undefined)) { + return; // i.e., continue + } + + stateMutable.set(key, [...value, message.id]); + }); + }); }; const eventMessageDelete = (state, action) => { From 47d4f97b9f85832a50498b82497b13cdea190f11 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 10 Dec 2020 11:59:49 -0500 Subject: [PATCH 09/10] narrowsReducer [nfc]: Add a few comments. These comments have been true for a while; I just noticed that they were missing while working on the adjacent code. --- src/chat/narrowsReducer.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index ca80cadeee9..7c1a6a7b499 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -67,10 +67,18 @@ const eventNewMessage = (state, action) => { } if (!(action.caughtUp[key] && action.caughtUp[key].newer)) { + // Don't add a message to the end of the list unless we know + // it's the most recent message, i.e., unless we know we're + // currently looking at (caught up with) the newest messages in + // the narrow. return; // i.e., continue } if (!(value.find(id => action.message.id === id) === undefined)) { + // Don't add a message that's already been added. It's probably + // very rare for a message to have already been added when we + // get an EVENT_NEW_MESSAGE, and perhaps impossible. (TODO: + // investigate?) return; // i.e., continue } From 07b163ea9bbd406b3976bc9fec2e7e0fa2323dee Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 11 Dec 2020 11:04:13 -0500 Subject: [PATCH 10/10] narrowsReducer [nfc]: Simplify some conditions. Having stared at these conditions for a bit (particularly the latter), I think they're harder to read than they need to be. So, simplify. --- src/chat/narrowsReducer.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index 7c1a6a7b499..7e37a275ed0 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -66,7 +66,9 @@ const eventNewMessage = (state, action) => { return; // i.e., continue } - if (!(action.caughtUp[key] && action.caughtUp[key].newer)) { + // (No guarantee that `key` is in `action.caughtUp`) + // flowlint-next-line unnecessary-optional-chain:off + if (!action.caughtUp[key]?.newer) { // Don't add a message to the end of the list unless we know // it's the most recent message, i.e., unless we know we're // currently looking at (caught up with) the newest messages in @@ -74,7 +76,7 @@ const eventNewMessage = (state, action) => { return; // i.e., continue } - if (!(value.find(id => action.message.id === id) === undefined)) { + if (value.some(id => action.message.id === id)) { // Don't add a message that's already been added. It's probably // very rare for a message to have already been added when we // get an EVENT_NEW_MESSAGE, and perhaps impossible. (TODO: