diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 427ac98926a..7782ac1fa29 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -453,15 +453,75 @@ export const makeOutboxMessage = (data: $Shape<$Diff>): const privateReduxStore = createStore(rootReducer); -/** The global Redux state, at its initial value. */ +/** + * The global Redux state, at its initial value. + * + * See `plusReduxState` for a version of the state that incorporates + * `selfUser` and other standard example data. + */ export const baseReduxState: GlobalState = deepFreeze(privateReduxStore.getState()); +/** + * A global Redux state, with `baseReduxState` plus the given data. + * + * See `reduxStatePlus` for a version that automatically includes `selfUser` + * and other standard example data. + */ export const reduxState = (extra?: $Rest): GlobalState => deepFreeze({ ...baseReduxState, ...extra, }); +/** + * The global Redux state, reflecting standard example data like `selfUser`. + * + * This approximates what the state might look like at a time when the app + * is showing its main UI: so when the user has logged into some account and + * we have our usual server data for it. + * + * In particular: + * * The self-user is `selfUser`. + * * Users `otherUser` and `thirdUser` also exist. + * + * More generally, each object in the Zulip app model -- a user, a stream, + * etc. -- that this module exports as a constant value (rather than only as + * a function to make a value) will typically appear here. + * + * That set will grow over time, so a test should never rely on + * `plusReduxState` containing only the things it currently contains. For + * example, it should not assume there are only three users, even if that + * happens to be true now. If the test needs a user (or stream, etc.) that + * isn't in this state, it should create the user privately for itself, with + * a helper like `makeUser`. + * + * On the other hand, a test *can* rely on an item being here if it + * currently is here. So for example a test which uses `plusReduxState` can + * assume it contains `otherUser`; it need not, and should not bother to, + * add `otherUser` to the state. + * + * See `baseReduxState` for a minimal version of the state. + */ +export const plusReduxState: GlobalState = reduxState({ + // TODO add .accounts, reflecting selfAuth, zulipVersion, zulipFeatureLevel + realm: { ...baseReduxState.realm, user_id: selfUser.user_id, email: selfUser.email }, + // TODO add crossRealmBot + users: [selfUser, otherUser, thirdUser], + // TODO add stream and subscription +}); + +/** + * A global Redux state, adding the given data to `plusReduxState`. + * + * This automatically includes standard example data like `selfUser` and + * `otherUser`, so that there's no need to add those explicitly. See + * `plusReduxState` for details on what's included. + * + * See `reduxState` for a version starting from a minimal state. + */ +export const reduxStatePlus = (extra?: $Rest): GlobalState => + deepFreeze({ ...plusReduxState, ...extra }); + export const realmState = (extra?: $Rest): RealmState => deepFreeze({ ...baseReduxState.realm, diff --git a/src/boot/__tests__/reducers-test.js b/src/boot/__tests__/reducers-test.js index eb0b05f0475..e99bf69b46e 100644 --- a/src/boot/__tests__/reducers-test.js +++ b/src/boot/__tests__/reducers-test.js @@ -1,6 +1,7 @@ /* @flow strict-local */ -import reducers, { ALL_KEYS } from '../reducers'; +import reducers from '../reducers'; import { discardKeys, storeKeys, cacheKeys } from '../store'; +import * as eg from '../../__tests__/lib/exampleData'; describe('reducers', () => { test('reducers return the default states on unknown action', () => { @@ -10,7 +11,7 @@ describe('reducers', () => { test('every reducer is listed in config as "discard", "store" or "cache"', () => { const configKeys = [...discardKeys, ...storeKeys, ...cacheKeys]; - expect(configKeys).toHaveLength(ALL_KEYS.length); - expect(configKeys).toSatisfyAll(key => ALL_KEYS.includes(key)); + const reducerKeys = Object.keys(eg.baseReduxState); + expect(configKeys.sort()).toEqual(reducerKeys.sort()); }); }); diff --git a/src/boot/reducers.js b/src/boot/reducers.js index ed9d4fd650a..9cc12dff91c 100644 --- a/src/boot/reducers.js +++ b/src/boot/reducers.js @@ -1,10 +1,7 @@ /* @flow strict-local */ -import { combineReducers } from 'redux'; -import type { CombinedReducer } from 'redux'; import { enableBatching } from 'redux-batched-actions'; import config from '../config'; -import { logSlowReducers } from '../utils/redux'; import { NULL_OBJECT } from '../nullObjects'; import type { Action, GlobalState, MigrationsState } from '../types'; @@ -31,38 +28,90 @@ import { reducer as unread } from '../unread/unreadModel'; import userGroups from '../user-groups/userGroupsReducer'; import userStatus from '../user-status/userStatusReducer'; import users from '../users/usersReducer'; +import timing from '../utils/timing'; -const reducers = { - migrations: (state: MigrationsState = NULL_OBJECT) => state, - accounts, - alertWords, - caughtUp, - drafts, - fetching, - flags, - messages, - narrows, - mute, - outbox, - pmConversations, - presence, - realm, - session, - settings, - streams, - subscriptions, - topics, - typing, - unread, - userGroups, - userStatus, - users, -}; +const migrations = (state: MigrationsState = NULL_OBJECT): MigrationsState => state; + +const { enableReduxSlowReducerWarnings, slowReducersThreshold } = config; + +function maybeLogSlowReducer(action, key, startMs, endMs) { + if (endMs - startMs >= slowReducersThreshold) { + timing.add({ text: `${action.type} @ ${key}`, startMs, endMs }); + } +} + +function applyReducer, State>( + key: Key, + reducer: (void | State, Action, GlobalState) => State, + state: void | State, + action: Action, + globalState: void | GlobalState, +): State { + let startMs = undefined; + if (enableReduxSlowReducerWarnings) { + startMs = Date.now(); + } + + /* $FlowFixMe - We make a small lie about the type, pretending that + globalState is not void. + + This is OK because it's only ever void at the initialization action, + and no reducer should do anything there other than return its initial + state, so in particular no reducer should even look at globalState. -export const ALL_KEYS: string[] = Object.keys(reducers); + Then on the other hand it's helpful because we want each reducer that + ever does use the globalState parameter to require it -- so that Flow + can help us be sure to pass it at the reducer's many other call sites, + in tests. That means it has to be `globalState: GlobalState`, not + `globalState : void | GlobalState`. + */ + const castGlobalState: GlobalState = globalState; -const combinedReducer: CombinedReducer = combineReducers( - config.enableReduxSlowReducerWarnings ? logSlowReducers(reducers) : reducers, -); + const nextState = reducer(state, action, castGlobalState); + + if (startMs !== undefined) { + const endMs = Date.now(); + maybeLogSlowReducer(action, key, startMs, endMs); + } + + return nextState; +} + +// Based on Redux upstream's combineReducers. +const combinedReducer = (state: void | GlobalState, action: Action): GlobalState => { + // prettier-ignore + const nextState = { + migrations: applyReducer('migrations', migrations, state?.migrations, action, state), + accounts: applyReducer('accounts', accounts, state?.accounts, action, state), + alertWords: applyReducer('alertWords', alertWords, state?.alertWords, action, state), + caughtUp: applyReducer('caughtUp', caughtUp, state?.caughtUp, action, state), + drafts: applyReducer('drafts', drafts, state?.drafts, action, state), + fetching: applyReducer('fetching', fetching, state?.fetching, action, state), + flags: applyReducer('flags', flags, state?.flags, action, state), + messages: applyReducer('messages', messages, state?.messages, action, state), + narrows: applyReducer('narrows', narrows, state?.narrows, action, state), + mute: applyReducer('mute', mute, state?.mute, action, state), + outbox: applyReducer('outbox', outbox, state?.outbox, action, state), + pmConversations: applyReducer('pmConversations', pmConversations, state?.pmConversations, action, state), + presence: applyReducer('presence', presence, state?.presence, action, state), + realm: applyReducer('realm', realm, state?.realm, action, state), + session: applyReducer('session', session, state?.session, action, state), + settings: applyReducer('settings', settings, state?.settings, action, state), + streams: applyReducer('streams', streams, state?.streams, action, state), + subscriptions: applyReducer('subscriptions', subscriptions, state?.subscriptions, action, state), + topics: applyReducer('topics', topics, state?.topics, action, state), + typing: applyReducer('typing', typing, state?.typing, action, state), + unread: applyReducer('unread', unread, state?.unread, action, state), + userGroups: applyReducer('userGroups', userGroups, state?.userGroups, action, state), + userStatus: applyReducer('userStatus', userStatus, state?.userStatus, action, state), + users: applyReducer('users', users, state?.users, action, state), + }; + + if (state && Object.keys(nextState).every(key => nextState[key] === state[key])) { + return state; + } + + return nextState; +}; export default enableBatching(combinedReducer); diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index 374bbeca944..b24e1f71a8f 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -94,7 +94,7 @@ describe('getTopicsForStream', () => { test('Return list of topic object with isMuted, unreadCount, topic name and max id in it.', () => { const stream = { ...eg.makeStream({ name: 'stream 1' }), stream_id: 1 }; - const state = eg.reduxState({ + const state = eg.reduxStatePlus({ streams: [stream], topics: { [stream.stream_id]: [ @@ -113,8 +113,8 @@ describe('getTopicsForStream', () => { eg.streamMessage({ stream_id: 1, subject: 'topic 4', id: 7 }), eg.streamMessage({ stream_id: 1, subject: 'topic 4', id: 8 }), ].reduce( - (st, message) => unreadReducer(st, mkMessageAction(message)), - eg.baseReduxState.unread, + (st, message) => unreadReducer(st, mkMessageAction(message), eg.plusReduxState), + eg.plusReduxState.unread, ), }); const expected = [ diff --git a/src/unread/__tests__/unread-testlib.js b/src/unread/__tests__/unread-testlib.js index b33c656d12f..3af78693e82 100644 --- a/src/unread/__tests__/unread-testlib.js +++ b/src/unread/__tests__/unread-testlib.js @@ -4,7 +4,11 @@ import type { Message } from '../../types'; import { reducer } from '../unreadModel'; import * as eg from '../../__tests__/lib/exampleData'; -export const initialState = reducer(undefined, ({ type: eg.randString() }: $FlowFixMe)); +export const initialState = reducer( + undefined, + ({ type: eg.randString() }: $FlowFixMe), + eg.baseReduxState, +); export const mkMessageAction = (message: Message) => ({ ...eg.eventNewMessageActionBase, @@ -19,6 +23,16 @@ const [user0, user1, user2, user3, user4, user5] = [0, 1, 2, 3, 4, 5].map(user_i ); export const selectorBaseState = (() => { + // We take user1 to be self. + // It might be convenient to convert this to the standard eg.selfUser, + // and use eg.reduxStatePlus. Until then, this just minimizes how much + // we've had to do in order to adapt our pre-existing tests. + const globalState = eg.reduxState({ + realm: { ...eg.baseReduxState.realm, user_id: user1.user_id }, + users: [user0, user1, user2, user3, user4, user5], + streams: [stream0, stream2], + }); + let state = initialState; for (const message of [ eg.streamMessage({ stream_id: 0, subject: 'a topic', id: 1, flags: ['mentioned'] }), @@ -28,7 +42,6 @@ export const selectorBaseState = (() => { eg.streamMessage({ stream_id: 0, subject: 'another topic', id: 5 }), eg.streamMessage({ stream_id: 2, subject: 'some other topic', id: 6 }), eg.streamMessage({ stream_id: 2, subject: 'some other topic', id: 7 }), - // We take user1 to be self. eg.pmMessageFromTo(user0, [user1], { id: 11 }), eg.pmMessageFromTo(user0, [user1], { id: 12 }), eg.pmMessageFromTo(user2, [user1], { id: 13 }), @@ -40,7 +53,7 @@ export const selectorBaseState = (() => { eg.pmMessageFromTo(user4, [user1, user5], { id: 24 }), eg.pmMessageFromTo(user4, [user1, user5], { id: 25 }), ]) { - state = reducer(state, mkMessageAction(message)); + state = reducer(state, mkMessageAction(message), globalState); } return state; })(); diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js index 80802bcae1b..b69abde1ffb 100644 --- a/src/unread/__tests__/unreadModel-test.js +++ b/src/unread/__tests__/unreadModel-test.js @@ -28,11 +28,11 @@ describe('stream substate', () => { describe('ACCOUNT_SWITCH', () => { test('resets state to initial state', () => { - const state = reducer(initialState, mkMessageAction(eg.streamMessage())); + const state = reducer(initialState, mkMessageAction(eg.streamMessage()), eg.plusReduxState); expect(state).not.toEqual(initialState); const action = { type: ACCOUNT_SWITCH, index: 1 }; - expect(reducer(state, action)).toEqual(initialState); + expect(reducer(state, action, eg.plusReduxState)).toEqual(initialState); }); }); @@ -61,7 +61,7 @@ describe('stream substate', () => { }; // prettier-ignore - expect(summary(reducer(initialState, action))).toEqual(Immutable.Map([ + expect(summary(reducer(initialState, action, eg.plusReduxState))).toEqual(Immutable.Map([ [message1.stream_id, Immutable.Map([[message1.subject, [1, 2]]])], ])); }); @@ -72,7 +72,11 @@ describe('stream substate', () => { const baseState = (() => { let state = initialState; - state = reducer(state, action(eg.streamMessage({ id: 1, subject: 'some topic' }))); + state = reducer( + state, + action(eg.streamMessage({ id: 1, subject: 'some topic' })), + eg.plusReduxState, + ); return state; })(); @@ -84,22 +88,34 @@ describe('stream substate', () => { }); test('if message id already exists, do not mutate state', () => { - const state = reducer(baseState, action(eg.streamMessage({ id: 1, subject: 'some topic' }))); + const state = reducer( + baseState, + action(eg.streamMessage({ id: 1, subject: 'some topic' })), + eg.plusReduxState, + ); expect(state).toBe(baseState); }); test('if message is not stream, return original state', () => { - const state = reducer(baseState, action(eg.pmMessage({ id: 4 }))); + const state = reducer(baseState, action(eg.pmMessage({ id: 4 })), eg.plusReduxState); expect(state.streams).toBe(baseState.streams); }); test('if message is sent by self, do not mutate state', () => { - const state = reducer(baseState, action(eg.streamMessage({ sender: eg.selfUser }))); + const state = reducer( + baseState, + action(eg.streamMessage({ sender: eg.selfUser })), + eg.plusReduxState, + ); expect(state).toBe(baseState); }); test('if message id does not exist, append to state', () => { - const state = reducer(baseState, action(eg.streamMessage({ id: 4, subject: 'some topic' }))); + const state = reducer( + baseState, + action(eg.streamMessage({ id: 4, subject: 'some topic' })), + eg.plusReduxState, + ); // prettier-ignore expect(summary(state)).toEqual(Immutable.Map([ [eg.stream.stream_id, Immutable.Map([['some topic', [1, 4]]])], @@ -108,7 +124,7 @@ describe('stream substate', () => { test('known stream, new topic', () => { const message = eg.streamMessage({ id: 4, subject: 'another topic' }); - const state = reducer(baseState, action(message)); + const state = reducer(baseState, action(message), eg.plusReduxState); // prettier-ignore expect(summary(state)).toEqual(Immutable.Map([ [eg.stream.stream_id, Immutable.Map([ @@ -120,7 +136,7 @@ describe('stream substate', () => { test('if stream with topic does not exist, append to state', () => { const message = eg.streamMessage({ id: 4, stream_id: 2, subject: 'another topic' }); - const state = reducer(baseState, action(message)); + const state = reducer(baseState, action(message), eg.plusReduxState); // prettier-ignore expect(summary(state)).toEqual(Immutable.Map([ [eg.stream.stream_id, Immutable.Map([['some topic', [1]]])], @@ -145,12 +161,13 @@ describe('stream substate', () => { const baseState = (() => { const streamAction = args => mkMessageAction(eg.streamMessage(args)); + const r = (state, action) => reducer(state, action, eg.plusReduxState); let state = initialState; - state = reducer(state, streamAction({ stream_id: 123, subject: 'foo', id: 1 })); - state = reducer(state, streamAction({ stream_id: 123, subject: 'foo', id: 2 })); - state = reducer(state, streamAction({ stream_id: 123, subject: 'foo', id: 3 })); - state = reducer(state, streamAction({ stream_id: 234, subject: 'bar', id: 4 })); - state = reducer(state, streamAction({ stream_id: 234, subject: 'bar', id: 5 })); + state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 1 })); + state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 2 })); + state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 3 })); + state = r(state, streamAction({ stream_id: 234, subject: 'bar', id: 4 })); + state = r(state, streamAction({ stream_id: 234, subject: 'bar', id: 5 })); return state; })(); @@ -164,30 +181,30 @@ describe('stream substate', () => { test('when operation is "add" but flag is not "read" do not mutate state', () => { const action = mkAction({ messages: [1, 2, 3], flag: 'star' }); - expect(reducer(initialState, action)).toBe(initialState); + expect(reducer(initialState, action, eg.plusReduxState)).toBe(initialState); }); test('if id does not exist do not mutate state', () => { const action = mkAction({ messages: [6, 7] }); - expect(reducer(baseState, action)).toBe(baseState); + expect(reducer(baseState, action, eg.plusReduxState)).toBe(baseState); }); test('if ids are in state remove them', () => { const action = mkAction({ messages: [3, 4, 5, 6] }); // prettier-ignore - expect(summary(reducer(baseState, action))).toEqual(Immutable.Map([ + expect(summary(reducer(baseState, action, eg.plusReduxState))).toEqual(Immutable.Map([ [123, Immutable.Map([['foo', [1, 2]]])], ])); }); test('when operation is "remove" do nothing', () => { const action = mkAction({ messages: [1, 2], operation: 'remove' }); - expect(reducer(baseState, action)).toBe(baseState); + expect(reducer(baseState, action, eg.plusReduxState)).toBe(baseState); }); test('when "all" is true reset state', () => { const action = mkAction({ messages: [], all: true }); - expect(reducer(baseState, action).streams).toBe(initialState.streams); + expect(reducer(baseState, action, eg.plusReduxState).streams).toBe(initialState.streams); }); }); }); diff --git a/src/unread/__tests__/unreadSelectors-test.js b/src/unread/__tests__/unreadSelectors-test.js index 35590e29e26..b703b27806c 100644 --- a/src/unread/__tests__/unreadSelectors-test.js +++ b/src/unread/__tests__/unreadSelectors-test.js @@ -434,7 +434,7 @@ describe('getUnreadStreamsAndTopics', () => { }); test('streams are sorted alphabetically, case-insensitive, topics by last activity, pinned stream on top', () => { - const state = deepFreeze({ + const state = eg.reduxStatePlus({ subscriptions: [ { stream_id: 2, @@ -473,7 +473,10 @@ describe('getUnreadStreamsAndTopics', () => { eg.streamMessage({ stream_id: 2, subject: 'c topic', id: 8 }), eg.streamMessage({ stream_id: 1, subject: 'e topic', id: 10 }), eg.streamMessage({ stream_id: 1, subject: 'd topic', id: 9 }), - ].reduce((st, message) => reducer(st, mkMessageAction(message)), initialState), + ].reduce( + (st, message) => reducer(st, mkMessageAction(message), eg.plusReduxState), + eg.plusReduxState.unread, + ), mute: [['def stream', 'c topic']], }); diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js index c7f5a19b7a5..9c8878b1c1d 100644 --- a/src/unread/unreadModel.js +++ b/src/unread/unreadModel.js @@ -1,5 +1,4 @@ /* @flow strict-local */ -import { combineReducers, type Reducer } from 'redux'; import type { Action } from '../actionTypes'; import type { @@ -23,9 +22,21 @@ export const getUnreadHuddles = (state: GlobalState): UnreadHuddlesState => stat export const getUnreadMentions = (state: GlobalState): UnreadMentionsState => state.unread.mentions; -export const reducer: Reducer = combineReducers({ - streams: unreadStreamsReducer, - pms: unreadPmsReducer, - huddles: unreadHuddlesReducer, - mentions: unreadMentionsReducer, -}); +export const reducer = ( + state: void | UnreadState, + action: Action, + globalState: GlobalState, +): UnreadState => { + const nextState = { + streams: unreadStreamsReducer(state?.streams, action, globalState), + pms: unreadPmsReducer(state?.pms, action), + huddles: unreadHuddlesReducer(state?.huddles, action), + mentions: unreadMentionsReducer(state?.mentions, action), + }; + + if (state && Object.keys(nextState).every(key => nextState[key] === state[key])) { + return state; + } + + return nextState; +}; diff --git a/src/unread/unreadStreamsReducer.js b/src/unread/unreadStreamsReducer.js index 58053100b57..e36d1bf0884 100644 --- a/src/unread/unreadStreamsReducer.js +++ b/src/unread/unreadStreamsReducer.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import type { Action } from '../types'; +import type { Action, GlobalState } from '../types'; import type { UnreadStreamsState } from './unreadModelTypes'; import { REALM_INIT, @@ -11,15 +11,16 @@ import { } from '../actionConstants'; import { addItemsToStreamArray, removeItemsDeeply } from './unreadHelpers'; import { NULL_ARRAY } from '../nullObjects'; +import { getOwnUserId } from '../users/userSelectors'; const initialState: UnreadStreamsState = NULL_ARRAY; -const eventNewMessage = (state, action) => { +const eventNewMessage = (state, action, globalState) => { if (action.message.type !== 'stream') { return state; } - if (action.ownUserId === action.message.sender_id) { + if (getOwnUserId(globalState) === action.message.sender_id) { return state; } @@ -49,7 +50,11 @@ const eventUpdateMessageFlags = (state, action) => { return state; }; -export default (state: UnreadStreamsState = initialState, action: Action): UnreadStreamsState => { +export default ( + state: UnreadStreamsState = initialState, + action: Action, + globalState: GlobalState, +): UnreadStreamsState => { switch (action.type) { case LOGOUT: case ACCOUNT_SWITCH: @@ -59,7 +64,7 @@ export default (state: UnreadStreamsState = initialState, action: Action): Unrea return (action.data.unread_msgs && action.data.unread_msgs.streams) || initialState; case EVENT_NEW_MESSAGE: - return eventNewMessage(state, action); + return eventNewMessage(state, action, globalState); case EVENT_MESSAGE_DELETE: return removeItemsDeeply(state, action.messageIds); diff --git a/src/utils/redux.js b/src/utils/redux.js deleted file mode 100644 index 384b97281e8..00000000000 --- a/src/utils/redux.js +++ /dev/null @@ -1,23 +0,0 @@ -/* @flow strict-local */ -import config from '../config'; -import timing from './timing'; - -export function logSlowReducers(reducers: O): O { - Object.keys(reducers).forEach((name: string) => { - const originalReducer = reducers[name]; - reducers[name] = (state, action) => { - const startMs = Date.now(); - const result = originalReducer(state, action); - const endMs = Date.now(); - if (endMs - startMs >= config.slowReducersThreshold) { - timing.add({ - text: `${action.type} @ ${name}`, - startMs, - endMs, - }); - } - return result; - }; - }); - return reducers; -}