Refactor `createStore` helpers

Move helper functions used only by `createStore` into `create-store.js` and
test the functionality as part of the `createStore` tests rather than having
direct tests for the helpers.

This refactoring decouples the tests for `createStore` from implementation
details. Having a single module for `createStore` and its tests should also
make it easier for a new developer to grok it.

 - Move `createReducer` and `bindSelectors` from util.js into create-store.js and
   refactor `bindSelectors`

 - Remove dedicated tests for these helpers and instead test the functionality
   via tests for `createStore`.

 - Remove check for `namespace` property of modules, as the types for the
   `createStoreModule` function now enforce the presence of this property.

 - Remove checks for conflicting selectors. These will be re-added in a
   subsequent commit.

src/sidebar/store/create-store.js

@@ -5,8 +5,6 @@ import thunk from 'redux-thunk';
 
 import { immutable } from '../util/immutable';
 
-import { createReducer, bindSelectors } from './util';
-
 /**
  * Helper that strips the first argument from a function type.
  *
@@ -83,6 +81,56 @@ import { createReducer, bindSelectors } from './util';
  *   SelectorMethods<RootSelectors>} Store
  */
 
+/**
+ * Create a Redux reducer from a store module's reducer map.
+ *
+ * @template State
+ * @param {Record<string, (s: State, a: redux.Action) => Partial<State>>} reducers -
+ *   Map of reducers from a store module.
+ */
+function createReducer(reducers) {
+  /** @param {redux.Action} action */
+  return (state = /** @type {State} */ ({}), action) => {
+    const reducer = reducers[action.type];
+    if (!reducer) {
+      return state;
+    }
+    const stateChanges = reducer(state, action);
+
+    // Some modules return an array rather than an object. They need to be
+    // handled differently so we don't convert them to an object.
+    if (Array.isArray(stateChanges)) {
+      return stateChanges;
+    }
+
+    return {
+      ...state,
+      ...stateChanges,
+    };
+  };
+}
+
+/**
+ * Convert a map of selector functions, which take a state value as their
+ * first argument, to a map of selector methods, which pre-fill the first
+ * argument by calling `getState()`.
+ *
+ * @template State
+ * @template {SelectorMap<State>} Selectors
+ * @param {Selectors} selectors
+ * @param {() => State} getState
+ * @return {SelectorMethods<Selectors>}
+ */
+function bindSelectors(selectors, getState) {
+  /** @type {Record<string, Function>} */
+  const boundSelectors = {};
+  for (let [name, selector] of Object.entries(selectors)) {
+    boundSelectors[name] = /** @param {any[]} args */ (...args) =>
+      selector(getState(), ...args);
+  }
+  return /** @type {SelectorMethods<Selectors>} */ (boundSelectors);
+}
+
 /**
  * Create a Redux store from a set of _modules_.
  *
@@ -113,37 +161,17 @@ import { createReducer, bindSelectors } from './util';
  * @return Store<any,any,any>
  */
 export function createStore(modules, initArgs = [], middleware = []) {
-  // Create the initial state and state update function.
-
-  // Namespaced objects for initial states.
+  /** @type {Record<string, unknown>} */
   const initialState = {};
+  for (let module of modules) {
+    initialState[module.namespace] = module.initialState(...initArgs);
+  }
 
-  /**
-   * Namespaced reducers from each module.
-   * @type {import("redux").ReducersMapObject} allReducers
-   */
+  /** @type {redux.ReducersMapObject} */
   const allReducers = {};
-  // Namespaced selectors from each module.
-  const allSelectors = {};
-
-  // Iterate over each module and prep each module's:
-  //    1. state
-  //    2. reducers
-  //    3. selectors
-  //
-  modules.forEach(module => {
-    if (module.namespace) {
-      initialState[module.namespace] = module.initialState(...initArgs);
-
-      allReducers[module.namespace] = createReducer(module.reducers);
-      allSelectors[module.namespace] = {
-        selectors: module.selectors,
-        rootSelectors: module.rootSelectors || {},
-      };
-    } else {
-      console.warn('Store module does not specify a namespace', module);
-    }
-  });
+  for (let module of modules) {
+    allReducers[module.namespace] = createReducer(module.reducers);
+  }
 
   const defaultMiddleware = [
     // The `thunk` middleware handles actions which are functions.
@@ -154,7 +182,7 @@ export function createStore(modules, initArgs = [], middleware = []) {
 
   const enhancer = redux.applyMiddleware(...defaultMiddleware, ...middleware);
 
-  // Create the combined reducer from the reducers for each module.
+  // Combine the reducers for all modules
   let reducer = redux.combineReducers(allReducers);
 
   // In debug builds, freeze the new state after each action to catch any attempts
@@ -164,15 +192,35 @@ export function createStore(modules, initArgs = [], middleware = []) {
     reducer = (state, action) => immutable(originalReducer(state, action));
   }
 
-  // Create the store.
   const store = redux.createStore(reducer, initialState, enhancer);
 
-  // Add actions and selectors as methods to the store.
-  const actions = Object.assign({}, ...modules.map(m => m.actionCreators));
-  const boundActions = redux.bindActionCreators(actions, store.dispatch);
-  const boundSelectors = bindSelectors(allSelectors, store.getState);
+  // Add action creators as methods to the store.
+  const actionCreators = Object.assign(
+    {},
+    ...modules.map(m => m.actionCreators)
+  );
+  const actionMethods = redux.bindActionCreators(
+    actionCreators,
+    store.dispatch
+  );
+  Object.assign(store, actionMethods);
 
-  Object.assign(store, boundActions, boundSelectors);
+  // Add selectors as methods to the store.
+  const selectorMethods = {};
+  for (let module of modules) {
+    const { namespace, selectors, rootSelectors } = module;
+    const boundSelectors = bindSelectors(
+      selectors,
+      () => store.getState()[namespace]
+    );
+    Object.assign(selectorMethods, boundSelectors);
+
+    if (rootSelectors) {
+      const boundRootSelectors = bindSelectors(rootSelectors, store.getState);
+      Object.assign(selectorMethods, boundRootSelectors);
+    }
+  }
+  Object.assign(store, selectorMethods);
 
   return store;
 }
@@ -193,9 +241,6 @@ export function makeAction(reducers, type, payload) {
   return { type, ...payload };
 }
 
-// The properties of the `config` argument to `createStoreModule` below are
-// declared inline due to https://github.com/microsoft/TypeScript/issues/43403.
-
 /**
  * Create a store module that can be passed to `createStore`.
  *

src/sidebar/store/test/create-store-test.js

@@ -8,8 +8,8 @@ function initialState(value = 0) {
   return { count: value };
 }
 
-const modules = [
-  // namespaced module A
+// Store modules that have the same state but under difference namespaces.
+const counterModules = [
   createStoreModule(initialState, {
     namespace: 'a',
 
@@ -41,7 +41,6 @@ const modules = [
     },
   }),
 
-  // namespaced module B
   createStoreModule(initialState, {
     namespace: 'b',
 
@@ -68,8 +67,71 @@ const modules = [
   }),
 ];
 
+// Store module whose state is an array.
+const tagsModule = createStoreModule([], {
+  namespace: 'tags',
+
+  reducers: {
+    ADD_TAG(state, action) {
+      return [...state, action.tag];
+    },
+  },
+
+  actionCreators: {
+    addTag(tag) {
+      return { type: 'ADD_TAG', tag };
+    },
+  },
+
+  selectors: {
+    getTags(state) {
+      return state;
+    },
+  },
+});
+
+// Store module with reducers that update only a subset of the state.
+const groupsModule = createStoreModule(
+  {
+    currentGroup: null,
+    groups: [],
+  },
+  {
+    namespace: 'groups',
+
+    reducers: {
+      ADD_GROUP(state, action) {
+        return { groups: [...state.groups, action.group] };
+      },
+
+      SELECT_GROUP(state, action) {
+        return { currentGroup: action.id };
+      },
+    },
+
+    actionCreators: {
+      addGroup(group) {
+        return { type: 'ADD_GROUP', group };
+      },
+      selectGroup(id) {
+        return { type: 'SELECT_GROUP', id };
+      },
+    },
+
+    selectors: {
+      allGroups(state) {
+        return state.groups;
+      },
+
+      currentGroup(state) {
+        return state.groups.find(g => g.id === state.currentGroup);
+      },
+    },
+  }
+);
+
 function counterStore(initArgs = [], middleware = []) {
-  return createStore(modules, initArgs, middleware);
+  return createStore(counterModules, initArgs, middleware);
 }
 
 describe('createStore', () => {
@@ -107,20 +169,20 @@ describe('createStore', () => {
 
   it('adds selectors as methods to the store', () => {
     const store = counterStore();
-    store.dispatch(modules[A].actionCreators.incrementA(5));
+    store.dispatch(counterModules[A].actionCreators.incrementA(5));
     assert.equal(store.getCountA(), 5);
   });
 
   it('adds root selectors as methods to the store', () => {
     const store = counterStore();
-    store.dispatch(modules[A].actionCreators.incrementA(5));
+    store.dispatch(counterModules[A].actionCreators.incrementA(5));
     assert.equal(store.getCountAFromRoot(), 5);
   });
 
   it('applies `thunk` middleware by default', () => {
     const store = counterStore();
     const doubleAction = (dispatch, getState) => {
-      dispatch(modules[A].actionCreators.incrementA(getState().a.count));
+      dispatch(counterModules[A].actionCreators.incrementA(getState().a.count));
     };
 
     store.incrementA(5);
@@ -185,6 +247,32 @@ describe('createStore', () => {
     assert.equal(store.getState().test.value, 42);
   });
 
+  it('supports modules whose state is an array', () => {
+    const store = createStore([tagsModule]);
+    assert.deepEqual(store.getTags(), []);
+    store.addTag('tag-1');
+    store.addTag('tag-2');
+    assert.deepEqual(store.getTags(), ['tag-1', 'tag-2']);
+  });
+
+  it('combines state updates from reducers with initial module state', () => {
+    const store = createStore([groupsModule]);
+
+    const group1 = { id: 'g1', name: 'Test group 1' };
+    const group2 = { id: 'g2', name: 'Test group 2' };
+
+    // Trigger reducers which update different module state. For this to work
+    // the result of each reducer must be combined with existing state.
+    store.addGroup(group1);
+    store.addGroup(group2);
+
+    store.selectGroup('g1');
+    assert.deepEqual(store.currentGroup(), group1);
+
+    store.selectGroup('g2');
+    assert.deepEqual(store.currentGroup(), group2);
+  });
+
   if (process.env.NODE_ENV !== 'production') {
     it('freezes store state in development builds', () => {
       const store = counterStore();

src/sidebar/store/test/util-test.js

 import { fakeReduxStore } from '../../test/fake-redux-store';
 
-import * as util from '../util';
-
-const fixtures = {
-  update: {
-    ADD_ANNOTATIONS: function (state, action) {
-      if (!state.annotations) {
-        return { annotations: action.annotations };
-      }
-      return { annotations: state.annotations.concat(action.annotations) };
-    },
-    SELECT_TAB: function (state, action) {
-      return { tab: action.tab };
-    },
-  },
-  selectors: {
-    namespace1: {
-      selectors: {
-        countAnnotations1: localState => localState.annotations.length,
-      },
-
-      rootSelectors: {
-        rootCountAnnotations1: rootState =>
-          rootState.namespace1.annotations.length,
-      },
-    },
-    namespace2: {
-      selectors: {
-        countAnnotations2: localState => localState.annotations.length,
-      },
-
-      rootSelectors: {
-        rootCountAnnotations2: rootState =>
-          rootState.namespace2.annotations.length,
-      },
-    },
-  },
-};
+import { actionTypes, awaitStateChange } from '../util';
 
 describe('sidebar/store/util', () => {
   describe('actionTypes', () => {
     it('returns an object with values equal to keys', () => {
       assert.deepEqual(
-        util.actionTypes({
+        actionTypes({
           SOME_ACTION: sinon.stub(),
           ANOTHER_ACTION: sinon.stub(),
         }),
@@ -54,143 +18,7 @@ describe('sidebar/store/util', () => {
     });
   });
 
-  describe('createReducer', () => {
-    it('returns an object if input state is undefined', () => {
-      // See redux.js:assertReducerShape in the "redux" package.
-      const reducer = util.createReducer(fixtures.update);
-      const initialState = reducer(undefined, {
-        type: 'DUMMY_ACTION',
-      });
-      assert.isOk(initialState);
-    });
-
-    it('returns a reducer that combines each update function from the input object', () => {
-      const reducer = util.createReducer(fixtures.update);
-      const newState = reducer(
-        {},
-        {
-          type: 'ADD_ANNOTATIONS',
-          annotations: [{ id: 1 }],
-        }
-      );
-      assert.deepEqual(newState, {
-        annotations: [{ id: 1 }],
-      });
-    });
-
-    it('returns a new object if the action was handled', () => {
-      const reducer = util.createReducer(fixtures.update);
-      const originalState = { someFlag: false };
-      assert.notEqual(
-        reducer(originalState, { type: 'SELECT_TAB', tab: 'notes' }),
-        originalState
-      );
-    });
-
-    it('returns the original object if the action was not handled', () => {
-      const reducer = util.createReducer(fixtures.update);
-      const originalState = { someFlag: false };
-      assert.equal(
-        reducer(originalState, { type: 'UNKNOWN_ACTION' }),
-        originalState
-      );
-    });
-
-    it('preserves state not modified by the update function', () => {
-      const reducer = util.createReducer(fixtures.update);
-      const newState = reducer(
-        { otherFlag: false },
-        {
-          type: 'ADD_ANNOTATIONS',
-          annotations: [{ id: 1 }],
-        }
-      );
-      assert.deepEqual(newState, {
-        otherFlag: false,
-        annotations: [{ id: 1 }],
-      });
-    });
-
-    it('supports reducer functions that return an array', () => {
-      const action = {
-        type: 'FIRST_ITEM',
-        item: 'bar',
-      };
-      const addItem = {
-        FIRST_ITEM(state, action) {
-          // Concatenate the array with a new item.
-          return [...state, action.item];
-        },
-      };
-      const reducer = util.createReducer(addItem);
-      const newState = reducer(['foo'], action);
-      assert.equal(newState.length, 2);
-    });
-  });
-
-  describe('bindSelectors', () => {
-    it('binds selectors to current value of module state', () => {
-      const getState = sinon.stub().returns({
-        namespace1: {
-          annotations: [{ id: 1 }],
-        },
-        namespace2: {
-          annotations: [{ id: 2 }],
-        },
-      });
-      const bound = util.bindSelectors(fixtures.selectors, getState);
-      assert.equal(bound.countAnnotations1(), 1);
-      assert.equal(bound.countAnnotations2(), 1);
-    });
-
-    it('binds root selectors to current value of root state', () => {
-      const getState = sinon.stub().returns({
-        namespace1: {
-          annotations: [{ id: 1 }],
-        },
-        namespace2: {
-          annotations: [{ id: 2 }],
-        },
-      });
-      const bound = util.bindSelectors(fixtures.selectors, getState);
-      assert.equal(bound.rootCountAnnotations1(), 1);
-      assert.equal(bound.rootCountAnnotations2(), 1);
-    });
-
-    it('throws an error if selector names in different modules conflict', () => {
-      const getState = () => ({});
-      assert.throws(() => {
-        util.bindSelectors(
-          {
-            moduleA: {
-              selectors: { someSelector: () => {} },
-            },
-            moduleB: {
-              selectors: { someSelector: () => {} },
-            },
-          },
-          getState
-        );
-      }, 'Duplicate selector "someSelector"');
-    });
-
-    it('throws an error if selector names in different modules conflict', () => {
-      const getState = () => ({});
-      assert.throws(() => {
-        util.bindSelectors(
-          {
-            moduleA: {
-              selectors: { someSelector: () => {} },
-              rootSelectors: { someSelector: () => {} },
-            },
-          },
-          getState
-        );
-      });
-    });
-  });
-
-  describe('awaitStateChange()', () => {
+  describe('awaitStateChange', () => {
     let store;
 
     beforeEach(() => {
@@ -209,11 +37,9 @@ describe('sidebar/store/util', () => {
     it('should return promise that resolves to a non-null value', () => {
       const expected = 5;
       store.setState({ val: 5 });
-      return util
-        .awaitStateChange(store, getValWhenGreaterThanTwo)
-        .then(actual => {
-          assert.equal(actual, expected);
-        });
+      return awaitStateChange(store, getValWhenGreaterThanTwo).then(actual => {
+        assert.equal(actual, expected);
+      });
     });
 
     it('should wait for awaitStateChange to return a non-null value', () => {
@@ -221,7 +47,7 @@ describe('sidebar/store/util', () => {
       const expected = 5;
 
       store.setState({ val: 2 });
-      valPromise = util.awaitStateChange(store, getValWhenGreaterThanTwo);
+      valPromise = awaitStateChange(store, getValWhenGreaterThanTwo);
       store.setState({ val: 5 });
 
       return valPromise.then(actual => {

src/sidebar/store/util.js

@@ -14,66 +14,6 @@ export function actionTypes(reducers) {
   }, /** @type {any} */ ({}));
 }
 
-/**
- * Create a standard Redux reducer function from a map of action types to reducers.
- *
- * @template {object} State
- * @param {Record<string, (s: State, action: any) => Partial<State>>} reducers -
- *   Object mapping action types to reducer functions. The result of the
- *   reducer is merged with the existing state.
- */
-export function createReducer(reducers) {
-  return (state = /** @type {State} */ ({}), action) => {
-    const reducer = reducers[action.type];
-    if (!reducer) {
-      return state;
-    }
-    const stateChanges = reducer(state, action);
-
-    // Some modules return an array rather than an object. They need to be
-    // handled differently so we don't convert them to an object.
-    if (Array.isArray(stateChanges)) {
-      return stateChanges;
-    }
-
-    return {
-      // @ts-expect-error - TS isn't certain `state` is spreadable here. Trust us!
-      ...state,
-      ...stateChanges,
-    };
-  };
-}
-
-/**
- * Takes a mapping of namespaced modules and the store's `getState()` function
- * and returns an aggregated flat object with all the selectors at the root
- * level. The keys to this object are functions that call the original
- * selectors with the `state` argument set to the current value of `getState()`.
- */
-export function bindSelectors(namespaces, getState) {
-  const boundSelectors = {};
-  Object.keys(namespaces).forEach(namespace => {
-    const { selectors, rootSelectors = {} } = namespaces[namespace];
-
-    Object.keys(selectors).forEach(selector => {
-      if (boundSelectors[selector]) {
-        throw new Error(`Duplicate selector "${selector}"`);
-      }
-      boundSelectors[selector] = (...args) =>
-        selectors[selector](getState()[namespace], ...args);
-    });
-
-    Object.keys(rootSelectors).forEach(selector => {
-      if (boundSelectors[selector]) {
-        throw new Error(`Duplicate selector "${selector}"`);
-      }
-      boundSelectors[selector] = (...args) =>
-        rootSelectors[selector](getState(), ...args);
-    });
-  });
-  return boundSelectors;
-}
-
 /**
  * Return a value from app state when it meets certain criteria.
  *

src/tsconfig.no-any.json

@@ -29,6 +29,7 @@
     "shared/**/*.js",
     "sidebar/config/*.js",
     "sidebar/helpers/*.js",
+    "sidebar/store/create-store.js",
     "sidebar/util/*.js",
     "types/*.d.ts"
   ],