Merge pull request #1309 from hypothesis/store-namespace-refactor

Add namespace capability to store modules

src/sidebar/services/service-url.js

@@ -42,7 +42,7 @@ function serviceUrl(store, apiRoutes) {
     });
 
   return function(linkName, params) {
-    const links = store.getState().links;
+    const links = store.getRootState().links;
 
     if (links === null) {
       return '';

src/sidebar/services/test/service-url-test.js

@@ -12,6 +12,9 @@ function fakeStore() {
     getState: function() {
       return { links: links };
     },
+    getRootState: function() {
+      return { links: links };
+    },
   };
 }
 

src/sidebar/store/create-store.js

@@ -26,14 +26,63 @@ const { createReducer, bindSelectors } = require('./util');
  * @param [any[]] middleware - List of additional Redux middlewares to use.
  */
 function createStore(modules, initArgs = [], middleware = []) {
-  // Create the initial state and state update function.
-  const initialState = Object.assign(
-    {},
-    ...modules.map(m => m.init(...initArgs))
-  );
-  const reducer = createReducer(...modules.map(m => m.update));
+  // Create the initial state and state update function. The "base"
+  // namespace is reserved for non-namespaced modules which will eventually
+  // be converted over.
+
+  // Namespaced objects for initial states.
+  const initialState = {
+    base: null,
+  };
+  // Namespaced reducers from each module.
+  const allReducers = {
+    base: null,
+  };
+  // Namespaced selectors from each module.
+  const allSelectors = {
+    base: {
+      selectors: {},
+      // Tells the bindSelector method to use local state for these selectors
+      // rather than the top level root state.
+      useLocalState: true,
+    },
+  };
+
+  // Temporary list of non-namespaced modules used for createReducer.
+  const baseModules = [];
+
+  // Iterate over each module and prep each module's:
+  //    1. state
+  //    2. reducers
+  //    3. selectors
+  //
+  // Modules that have no namespace get dumped into the "base" namespace.
+  //
+  modules.forEach(module => {
+    if (module.namespace) {
+      initialState[module.namespace] = module.init(...initArgs);
+      allReducers[module.namespace] = createReducer(module.update);
+      allSelectors[module.namespace] = {
+        selectors: module.selectors,
+      };
+    } else {
+      // No namespace
+      allSelectors.base.selectors = {
+        // Aggregate the selectors into a single "base" map
+        ...allSelectors.base.selectors,
+        ...module.selectors,
+      };
+      initialState.base = {
+        ...initialState.base,
+        ...module.init(...initArgs),
+      };
+      baseModules.push(module);
+    }
+  });
+
+  // Create the base reducer for modules that are not opting in for namespacing
+  allReducers.base = createReducer(...baseModules.map(m => m.update));
 
-  // Create the store.
   const defaultMiddleware = [
     // The `thunk` middleware handles actions which are functions.
     // This is used to implement actions which have side effects or are
@@ -41,13 +90,30 @@ function createStore(modules, initArgs = [], middleware = []) {
     thunk,
   ];
   const enhancer = redux.applyMiddleware(...defaultMiddleware, ...middleware);
-  const store = redux.createStore(reducer, initialState, enhancer);
+
+  // Create the store.
+  const store = redux.createStore(
+    redux.combineReducers(allReducers),
+    initialState,
+    enhancer
+  );
+
+  // Temporary wrapper while we use the "base" namespace. This allows getState
+  // to work as it did before. Under the covers the state is actually
+  // nested inside "base" namespace.
+  const getState = store.getState;
+  store.getState = () => getState().base;
+
+  // Because getState is overridden, we still need a fallback for the root state
+  // for the namespaced modules. They will temporarily use getRootState
+  // until all modules are namespaced and then this will be deprecated.
+  store.getRootState = () => getState();
 
   // Add actions and selectors as methods to the store.
   const actions = Object.assign({}, ...modules.map(m => m.actions));
   const boundActions = redux.bindActionCreators(actions, store.dispatch);
-  const selectors = Object.assign({}, ...modules.map(m => m.selectors));
-  const boundSelectors = bindSelectors(selectors, store.getState);
+  const boundSelectors = bindSelectors(allSelectors, store.getRootState);
+
   Object.assign(store, boundActions, boundSelectors);
 
   return store;

src/sidebar/store/modules/annotations.js

@@ -254,7 +254,7 @@ function addAnnotations(annotations, now) {
 
   return function(dispatch, getState) {
     const added = annotations.filter(function(annot) {
-      return !findByID(getState().annotations, annot.id);
+      return !findByID(getState().base.annotations, annot.id);
     });
 
     dispatch({
@@ -262,7 +262,7 @@ function addAnnotations(annotations, now) {
       annotations: annotations,
     });
 
-    if (!getState().isSidebar) {
+    if (!getState().base.isSidebar) {
       return;
     }
 
@@ -277,7 +277,7 @@ function addAnnotations(annotations, now) {
     if (anchoringIDs.length > 0) {
       setTimeout(() => {
         // Find annotations which haven't yet been anchored in the document.
-        const anns = getState().annotations;
+        const anns = getState().base.annotations;
         const annsStillAnchoring = anchoringIDs
           .map(id => findByID(anns, id))
           .filter(ann => ann && metadata.isWaitingToAnchor(ann));

src/sidebar/store/modules/drafts.js

@@ -106,10 +106,10 @@ function createDraft(annotation, changes) {
 function deleteNewAndEmptyDrafts() {
   const annotations = require('./annotations');
   return (dispatch, getState) => {
-    const newDrafts = getState().drafts.filter(draft => {
+    const newDrafts = getState().base.drafts.filter(draft => {
       return (
         metadata.isNew(draft.annotation) &&
-        !getDraftIfNotEmpty(getState(), draft.annotation)
+        !getDraftIfNotEmpty(getState().base, draft.annotation)
       );
     });
     const removedAnnotations = newDrafts.map(draft => {

src/sidebar/store/modules/links.js

@@ -11,12 +11,12 @@
 
 /** Return the initial links. */
 function init() {
-  return { links: null };
+  return null;
 }
 
 /** Return updated links based on the given current state and action object. */
 function updateLinks(state, action) {
-  return { links: action.newLinks };
+  return { ...action.newLinks };
 }
 
 /** Return an action object for updating the links to the given newLinks. */
@@ -26,6 +26,7 @@ function updateLinksAction(newLinks) {
 
 module.exports = {
   init: init,
+  namespace: 'links',
   update: { UPDATE_LINKS: updateLinks },
   actions: { updateLinks: updateLinksAction },
   selectors: {},

src/sidebar/store/modules/selection.js

@@ -236,7 +236,7 @@ function selectAnnotations(ids) {
 /** Toggle whether annotations are selected or not. */
 function toggleSelectedAnnotations(ids) {
   return function(dispatch, getState) {
-    const selection = Object.assign({}, getState().selectedAnnotationMap);
+    const selection = Object.assign({}, getState().base.selectedAnnotationMap);
     for (let i = 0; i < ids.length; i++) {
       const id = ids[i];
       if (selection[id]) {
@@ -260,7 +260,7 @@ function setForceVisible(id, visible) {
   // FIXME: This should be converted to a plain action and accessing the state
   // should happen in the update() function
   return function(dispatch, getState) {
-    const forceVisible = Object.assign({}, getState().forceVisible);
+    const forceVisible = Object.assign({}, getState().base.forceVisible);
     forceVisible[id] = visible;
     dispatch({
       type: actions.SET_FORCE_VISIBLE,
@@ -285,7 +285,7 @@ function setCollapsed(id, collapsed) {
   // FIXME: This should be converted to a plain action and accessing the state
   // should happen in the update() function
   return function(dispatch, getState) {
-    const expanded = Object.assign({}, getState().expanded);
+    const expanded = Object.assign({}, getState().base.expanded);
     expanded[id] = !collapsed;
     dispatch({
       type: actions.SET_EXPANDED,

src/sidebar/store/modules/test/links-test.js

@@ -9,23 +9,24 @@ const action = links.actions.updateLinks;
 describe('sidebar.reducers.links', function() {
   describe('#init()', function() {
     it('returns a null links object', function() {
-      assert.deepEqual(init(), { links: null });
+      assert.deepEqual(init(), null);
     });
   });
 
   describe('#update.UPDATE_LINKS()', function() {
     it('returns the given newLinks as the links object', function() {
-      assert.deepEqual(update('CURRENT_STATE', { newLinks: 'NEW_LINKS' }), {
-        links: 'NEW_LINKS',
-      });
+      assert.deepEqual(
+        update('CURRENT_STATE', { newLinks: { NEW_LINK: 'http://new_link' } }),
+        { NEW_LINK: 'http://new_link' }
+      );
     });
   });
 
   describe('#actions.updateLinks()', function() {
     it('returns an UPDATE_LINKS action object for the given newLinks', function() {
-      assert.deepEqual(action('NEW_LINKS'), {
+      assert.deepEqual(action({ NEW_LINK: 'http://new_link' }), {
         type: 'UPDATE_LINKS',
-        newLinks: 'NEW_LINKS',
+        newLinks: { NEW_LINK: 'http://new_link' },
       });
     });
   });

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

@@ -2,38 +2,68 @@
 
 const createStore = require('../create-store');
 
-const counterModule = {
-  init(value = 0) {
-    return { count: value };
-  },
+const BASE = 0;
 
-  update: {
-    ['INCREMENT_COUNTER'](state, action) {
-      return { count: state.count + action.amount };
+const modules = [
+  {
+    // base module
+    init(value = 0) {
+      return { count: value };
+    },
+
+    update: {
+      INCREMENT_COUNTER: (state, action) => {
+        return { count: state.count + action.amount };
+      },
+    },
+
+    actions: {
+      increment(amount) {
+        return { type: 'INCREMENT_COUNTER', amount };
+      },
     },
-  },
 
-  actions: {
-    increment(amount) {
-      return { type: 'INCREMENT_COUNTER', amount };
+    selectors: {
+      getCount(state) {
+        return state.count;
+      },
     },
   },
+  {
+    // namespaced module
+    init(value = 0) {
+      return { count: value };
+    },
+    namespace: 'foo',
+
+    update: {
+      ['INCREMENT_COUNTER_2'](state, action) {
+        return { count: state.count + action.amount };
+      },
+    },
 
-  selectors: {
-    getCount(state) {
-      return state.count;
+    actions: {
+      increment2(amount) {
+        return { type: 'INCREMENT_COUNTER_2', amount };
+      },
+    },
+
+    selectors: {
+      getCount2(state) {
+        return state.foo.count;
+      },
     },
   },
-};
+];
 
 function counterStore(initArgs = [], middleware = []) {
-  return createStore([counterModule], initArgs, middleware);
+  return createStore(modules, initArgs, middleware);
 }
 
 describe('sidebar.store.create-store', () => {
   it('returns a working Redux store', () => {
     const store = counterStore();
-    store.dispatch(counterModule.actions.increment(5));
+    store.dispatch(modules[BASE].actions.increment(5));
     assert.equal(store.getState().count, 5);
   });
 
@@ -60,14 +90,14 @@ describe('sidebar.store.create-store', () => {
 
   it('adds selectors as methods to the store', () => {
     const store = counterStore();
-    store.dispatch(counterModule.actions.increment(5));
+    store.dispatch(modules[BASE].actions.increment(5));
     assert.equal(store.getCount(), 5);
   });
 
   it('applies `thunk` middleware by default', () => {
     const store = counterStore();
     const doubleAction = (dispatch, getState) => {
-      dispatch(counterModule.actions.increment(getState().count));
+      dispatch(modules[BASE].actions.increment(getState().base.count));
     };
 
     store.increment(5);
@@ -92,4 +122,25 @@ describe('sidebar.store.create-store', () => {
 
     assert.deepEqual(actions, [{ type: 'INCREMENT_COUNTER', amount: 5 }]);
   });
+
+  it('namespaced actions and selectors operate on their respective state', () => {
+    const store = counterStore();
+    store.increment2(6);
+    store.increment(5);
+    assert.equal(store.getCount2(), 6);
+  });
+
+  it('getState returns the base state', () => {
+    const store = counterStore();
+    store.increment(5);
+    assert.equal(store.getState().count, 5);
+  });
+
+  it('getRootState returns the top level root state', () => {
+    const store = counterStore();
+    store.increment(5);
+    store.increment2(6);
+    assert.equal(store.getRootState().base.count, 5);
+    assert.equal(store.getRootState().foo.count, 6);
+  });
 });

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

@@ -14,8 +14,23 @@ const fixtures = {
       return { tab: action.tab };
     },
   },
-  countAnnotations: function(state) {
-    return state.annotations.length;
+  selectors: {
+    namespace1: {
+      selectors: {
+        countAnnotations1: function(state) {
+          return state.namespace1.annotations.length;
+        },
+      },
+    },
+    namespace2: {
+      useLocalState: true,
+      selectors: {
+        countAnnotations2: function(state) {
+          // useLocalState does not need namespaced path.
+          return state.annotations.length;
+        },
+      },
+    },
   },
 };
 
@@ -36,6 +51,15 @@ describe('reducer utils', function() {
   });
 
   describe('#createReducer', function() {
+    it('returns an object if input state is undefined', function() {
+      // 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', function() {
       const reducer = util.createReducer(fixtures.update);
       const newState = reducer(
@@ -105,23 +129,39 @@ describe('reducer utils', function() {
 
       assert.deepEqual(newState, { firstCounter: 6, secondCounter: 11 });
     });
+
+    it('supports reducer functions that return an array', function() {
+      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', function() {
     it('bound functions call original functions with current value of getState()', function() {
-      const annotations = [{ id: 1 }];
-      const getState = sinon.stub().returns({ annotations: annotations });
-      const bound = util.bindSelectors(
-        {
-          countAnnotations: fixtures.countAnnotations,
+      const getState = sinon.stub().returns({
+        namespace1: {
+          annotations: [{ id: 1 }],
         },
-        getState
-      );
-
-      assert.equal(bound.countAnnotations(), 1);
-
-      getState.returns({ annotations: annotations.concat([{ id: 2 }]) });
-      assert.equal(bound.countAnnotations(), 2);
+        namespace2: {
+          annotations: [{ id: 1 }],
+        },
+      });
+      const bound = util.bindSelectors(fixtures.selectors, getState);
+      // Test non-namespaced selector (useLocalState=true)
+      assert.equal(bound.countAnnotations1(), 1);
+      // Test the namespaced selector
+      assert.equal(bound.countAnnotations2(), 1);
     });
   });
 });

src/sidebar/store/util.js

@@ -20,6 +20,10 @@ function actionTypes(updateFns) {
 function createReducer(...actionToUpdateFn) {
   // Combine the (action name => update function) maps together into a single
   // (action name => update functions) map.
+  //
+  // After namespace migration, remove the requirement for actionToUpdateFns
+  // to use arrays. Why? createReducer will be called once for each module rather
+  // than once for all modules.
   const actionToUpdateFns = {};
   actionToUpdateFn.forEach(map => {
     Object.keys(map).forEach(k => {
@@ -27,35 +31,51 @@ function createReducer(...actionToUpdateFn) {
     });
   });
 
-  return (state, action) => {
+  return (state = {}, action) => {
     const fns = actionToUpdateFns[action.type];
     if (!fns) {
       return state;
     }
+    // Some modules return an array rather than an object. They need to be
+    // handled differently so we don't cast them to an object.
+    if (Array.isArray(state)) {
+      return [...fns[0](state, action)];
+    }
     return Object.assign({}, state, ...fns.map(f => f(state, action)));
   };
 }
 
 /**
- * Takes an object mapping keys to selector functions and the `getState()`
- * function from the store and returns an object with the same keys but where
- * the values are functions that call the original functions with the `state`
- * argument set to the current value of `getState()`
+ * 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()`
+ * for namespaced modules or `getState().base` for non-namespaced modules.
  */
-function bindSelectors(selectors, getState) {
-  return Object.keys(selectors).reduce(function(bound, key) {
-    const selector = selectors[key];
-    bound[key] = function() {
-      const args = [].slice.apply(arguments);
-      args.unshift(getState());
-      return selector.apply(null, args);
-    };
-    return bound;
-  }, {});
+function bindSelectors(namespaces, getState) {
+  const totalSelectors = {};
+  Object.keys(namespaces).forEach(namespace => {
+    const selectors = namespaces[namespace].selectors;
+    const useLocalState = namespaces[namespace].useLocalState;
+    Object.keys(selectors).forEach(selector => {
+      totalSelectors[selector] = function() {
+        const args = [].slice.apply(arguments);
+        if (useLocalState) {
+          // Temporary scaffold until all selectors use namespaces.
+          args.unshift(getState()[namespace]);
+        } else {
+          // Namespace modules get root scope
+          args.unshift(getState());
+        }
+        return selectors[selector].apply(null, args);
+      };
+    });
+  });
+  return totalSelectors;
 }
 
 module.exports = {
-  actionTypes: actionTypes,
-  bindSelectors: bindSelectors,
-  createReducer: createReducer,
+  actionTypes,
+  bindSelectors,
+  createReducer,
 };