Commit 5250a6f7 authored by Robert Knight's avatar Robert Knight

Convert store selectors to receive local rather than global state

Change store module selectors to receive the module's local state rather than
the global/root state as the first argument. The majority of selectors only need
access to the module's local state. Changing these selectors to receive the
local rather than root state has benefits:

 - It avoids the need for the selectors to know about the structure of
   the root state. This can make future refactorings and re-use easier.

 - It is consistent with how state is accessed in the reducer functions

 - It will simplify typing selectors because the type of the `state`
   argument will be the same as the type of `state` returned by `init`
   and passed to `update` functions

 - It potentially enables automatic memoization of selectors based on
   the local state. ie. If the local state for a module has not changed as a
   result of an action, then none of the local selectors will need to be
   recomputed

There are a small number of selectors that need access to state from
multple modules. In future we may want to move these to some separate
location but, as an incremental step, add support for "root selectors"
to modules under a `rootSelectors` key. These selectors will still receive the
root state as their first argument as before.

This commit also adds missing tests for the `annotationExists` selector and the
`route` module.

See also https://hypothes-is.slack.com/archives/C1M8NH76X/p1595237570226700.
parent 9edc2928
......@@ -52,6 +52,7 @@ export default function createStore(modules, initArgs = [], middleware = []) {
allReducers[module.namespace] = createReducer(module.update);
allSelectors[module.namespace] = {
selectors: module.selectors,
rootSelectors: module.rootSelectors || {},
};
} else {
console.warn('Store module does not specify a namespace', module);
......
......@@ -133,14 +133,14 @@ function apiRequestFinished() {
/** Selectors */
function hasFetchedAnnotations(state) {
return state.activity.hasFetchedAnnotations;
return state.hasFetchedAnnotations;
}
/**
* Return true when annotations are actively being fetched.
*/
function isFetchingAnnotations(state) {
return state.activity.activeAnnotationFetches > 0;
return state.activeAnnotationFetches > 0;
}
/**
......@@ -148,10 +148,7 @@ function isFetchingAnnotations(state) {
* before the UI is ready for interactivity with annotations.
*/
function isLoading(state) {
return (
state.activity.activeApiRequests > 0 ||
!state.activity.hasFetchedAnnotations
);
return state.activeApiRequests > 0 || !state.hasFetchedAnnotations;
}
/**
......@@ -167,7 +164,7 @@ function isSavingAnnotation(state, annotation) {
if (!annotation.$tag) {
return false;
}
return state.activity.activeAnnotationSaveRequests.includes(annotation.$tag);
return state.activeAnnotationSaveRequests.includes(annotation.$tag);
}
export default {
......
......@@ -256,7 +256,7 @@ function addAnnotations(annotations) {
// If we're not in the sidebar, we're done here.
// FIXME Split the annotation-adding from the anchoring code; possibly
// move into service
if (route.selectors.route(getState()) !== 'sidebar') {
if (route.selectors.route(getState().route) !== 'sidebar') {
return;
}
......@@ -410,7 +410,7 @@ function updateFlagStatus(id, isFlagged) {
* @return {number}
*/
const annotationCount = createSelector(
state => state.annotations.annotations,
state => state.annotations,
annotations => countIf(annotations, metadata.isAnnotation)
);
......@@ -421,14 +421,14 @@ const annotationCount = createSelector(
* @return {boolean}
*/
function annotationExists(state, id) {
return state.annotations.annotations.some(annot => annot.id === id);
return state.annotations.some(annot => annot.id === id);
}
/**
* Return the annotation with the given ID
*/
function findAnnotationByID(state, id) {
return findByID(state.annotations.annotations, id);
return findByID(state.annotations, id);
}
/**
......@@ -442,7 +442,7 @@ function findAnnotationByID(state, id) {
function findIDsForTags(state, tags) {
const ids = [];
tags.forEach(tag => {
const annot = findByTag(state.annotations.annotations, tag);
const annot = findByTag(state.annotations, tag);
if (annot && annot.id) {
ids.push(annot.id);
}
......@@ -456,7 +456,7 @@ function findIDsForTags(state, tags) {
* @return {string[]}
*/
const focusedAnnotations = createSelector(
state => state.annotations.focused,
state => state.focused,
focused => trueKeys(focused)
);
......@@ -466,7 +466,7 @@ const focusedAnnotations = createSelector(
* @return {string[]}
*/
const highlightedAnnotations = createSelector(
state => state.annotations.highlighted,
state => state.highlighted,
highlighted => trueKeys(highlighted)
);
......@@ -477,7 +477,7 @@ const highlightedAnnotations = createSelector(
* @return {boolean}
*/
function isAnnotationFocused(state, $tag) {
return state.annotations.focused[$tag] === true;
return state.focused[$tag] === true;
}
/**
......@@ -486,7 +486,7 @@ function isAnnotationFocused(state, $tag) {
* @return {boolean}
*/
const isWaitingToAnchorAnnotations = createSelector(
state => state.annotations.annotations,
state => state.annotations,
annotations => annotations.some(metadata.isWaitingToAnchor)
);
......@@ -497,7 +497,7 @@ const isWaitingToAnchorAnnotations = createSelector(
* @return {Annotation[]}
*/
const newAnnotations = createSelector(
state => state.annotations.annotations,
state => state.annotations,
annotations =>
annotations.filter(ann => metadata.isNew(ann) && !metadata.isHighlight(ann))
);
......@@ -509,7 +509,7 @@ const newAnnotations = createSelector(
* @return {Annotation[]}
*/
const newHighlights = createSelector(
state => state.annotations.annotations,
state => state.annotations,
annotations =>
annotations.filter(ann => metadata.isNew(ann) && metadata.isHighlight(ann))
);
......@@ -520,7 +520,7 @@ const newHighlights = createSelector(
* @return {number}
*/
const noteCount = createSelector(
state => state.annotations.annotations,
state => state.annotations,
annotations => countIf(annotations, metadata.isPageNote)
);
......@@ -530,7 +530,7 @@ const noteCount = createSelector(
* @type {(state: any) => number}
*/
const orphanCount = createSelector(
state => state.annotations.annotations,
state => state.annotations,
annotations => countIf(annotations, metadata.isOrphan)
);
......@@ -540,7 +540,7 @@ const orphanCount = createSelector(
* @return {Annotation[]}
*/
function savedAnnotations(state) {
return state.annotations.annotations.filter(function (ann) {
return state.annotations.filter(function (ann) {
return !metadata.isNew(ann);
});
}
......
......@@ -48,11 +48,11 @@ function setDefault(defaultKey, value) {
* present
*/
function getDefault(state, defaultKey) {
return state.defaults[defaultKey];
return state[defaultKey];
}
function getDefaults(state) {
return state.defaults;
return state;
}
export default {
......
......@@ -129,15 +129,15 @@ function clearDirectLinkedIds() {
* Selectors
*/
function directLinkedAnnotationId(state) {
return state.directLinked.directLinkedAnnotationId;
return state.directLinkedAnnotationId;
}
function directLinkedGroupId(state) {
return state.directLinked.directLinkedGroupId;
return state.directLinkedGroupId;
}
function directLinkedGroupFetchFailed(state) {
return state.directLinked.directLinkedGroupFetchFailed;
return state.directLinkedGroupFetchFailed;
}
export default {
......
......@@ -102,7 +102,7 @@ function deleteNewAndEmptyDrafts() {
const newDrafts = getState().drafts.filter(draft => {
return (
metadata.isNew(draft.annotation) &&
!getDraftIfNotEmpty(getState(), draft.annotation)
!getDraftIfNotEmpty(getState().drafts, draft.annotation)
);
});
const removedAnnotations = newDrafts.map(draft => {
......@@ -141,7 +141,7 @@ function removeDraft(annotation) {
* @return {number}
*/
function countDrafts(state) {
return state.drafts.length;
return state.length;
}
/**
......@@ -150,7 +150,7 @@ function countDrafts(state) {
* @return {Draft|null}
*/
function getDraft(state, annotation) {
const drafts = state.drafts;
const drafts = state;
for (let i = 0; i < drafts.length; i++) {
const draft = drafts[i];
if (draft.match(annotation)) {
......@@ -181,7 +181,7 @@ function getDraftIfNotEmpty(state, annotation) {
* @return {Object[]}
*/
const unsavedAnnotations = createSelector(
state => state.drafts,
state => state,
drafts => drafts.filter(d => !d.annotation.id).map(d => d.annotation)
);
......
......@@ -67,7 +67,7 @@ function updateFrameAnnotationFetchStatus(uri, status) {
* Return the list of frames currently connected to the sidebar app.
*/
function frames(state) {
return state.frames;
return state;
}
/**
......@@ -81,7 +81,7 @@ function frames(state) {
* This may be `null` during startup.
*/
const mainFrame = createSelector(
state => state.frames,
state => state,
// Sub-frames will all have a "frame identifier" set. The main frame is the
// one with a `null` id.
......@@ -117,8 +117,8 @@ const createShallowEqualSelector = createSelectorCreator(
// Memoized selector will return the same array (of URIs) reference unless the
// values of the array change (are not shallow-equal).
const searchUris = createShallowEqualSelector(
state => {
return state.frames.reduce(
frames => {
return frames.reduce(
(uris, frame) => uris.concat(searchUrisForFrame(frame)),
[]
);
......
......@@ -104,10 +104,10 @@ function loadGroups(groups) {
* @return {Group|undefined|null}
*/
function focusedGroup(state) {
if (!state.groups.focusedGroupId) {
if (!state.focusedGroupId) {
return null;
}
return getGroup(state, state.groups.focusedGroupId);
return getGroup(state, state.focusedGroupId);
}
/**
......@@ -116,7 +116,7 @@ function focusedGroup(state) {
* @return {string|null}
*/
function focusedGroupId(state) {
return state.groups.focusedGroupId;
return state.focusedGroupId;
}
/**
......@@ -125,7 +125,7 @@ function focusedGroupId(state) {
* @return {Group[]}
*/
function allGroups(state) {
return state.groups.groups;
return state.groups;
}
/**
......@@ -135,9 +135,33 @@ function allGroups(state) {
* @return {Group|undefined}
*/
function getGroup(state, id) {
return state.groups.groups.find(g => g.id === id);
return state.groups.find(g => g.id === id);
}
/**
* Return groups the user isn't a member of that are scoped to the URI.
*
* @return {Group[]}
*/
const getFeaturedGroups = createSelector(
state => state.groups,
groups => groups.filter(group => !group.isMember && group.isScopedToUri)
);
/**
* Return groups that are scoped to the uri. This is used to return the groups
* that show up in the old groups menu. This should be removed once the new groups
* menu is permanent.
*
* @return {Group[]}
*/
const getInScopeGroups = createSelector(
state => state.groups,
groups => groups.filter(g => g.isScopedToUri)
);
// Selectors that receive root state.
/**
* Return groups the logged in user is a member of.
*
......@@ -145,7 +169,7 @@ function getGroup(state, id) {
*/
const getMyGroups = createSelector(
state => state.groups.groups,
session.selectors.isLoggedIn,
state => session.selectors.isLoggedIn(state.session),
(groups, loggedIn) => {
// If logged out, the Public group still has isMember set to true so only
// return groups with membership in logged in state.
......@@ -156,25 +180,15 @@ const getMyGroups = createSelector(
}
);
/**
* Return groups the user isn't a member of that are scoped to the URI.
*
* @return {Group[]}
*/
const getFeaturedGroups = createSelector(
state => state.groups.groups,
groups => groups.filter(group => !group.isMember && group.isScopedToUri)
);
/**
* Return groups that don't show up in Featured and My Groups.
*
* @return {Group[]}
*/
const getCurrentlyViewingGroups = createSelector(
allGroups,
getMyGroups,
getFeaturedGroups,
state => allGroups(state.groups),
state => getMyGroups(state),
state => getFeaturedGroups(state.groups),
(allGroups, myGroups, featuredGroups) => {
return allGroups.filter(
g => !myGroups.includes(g) && !featuredGroups.includes(g)
......@@ -182,18 +196,6 @@ const getCurrentlyViewingGroups = createSelector(
}
);
/**
* Return groups that are scoped to the uri. This is used to return the groups
* that show up in the old groups menu. This should be removed once the new groups
* menu is permanent.
*
* @return {Group[]}
*/
const getInScopeGroups = createSelector(
state => state.groups.groups,
groups => groups.filter(g => g.isScopedToUri)
);
export default {
init,
namespace: 'groups',
......@@ -205,12 +207,14 @@ export default {
},
selectors: {
allGroups,
focusedGroup,
focusedGroupId,
getFeaturedGroups,
getGroup,
getInScopeGroups,
},
rootSelectors: {
getCurrentlyViewingGroups,
getFeaturedGroups,
getMyGroups,
getInScopeGroups,
focusedGroup,
focusedGroupId,
},
};
......@@ -143,7 +143,7 @@ function clearPendingUpdates() {
* @return {{[id: string]: Annotation}}
*/
function pendingUpdates(state) {
return state.realTimeUpdates.pendingUpdates;
return state.pendingUpdates;
}
/**
......@@ -153,17 +153,14 @@ function pendingUpdates(state) {
* @return {{[id: string]: boolean}}
*/
function pendingDeletions(state) {
return state.realTimeUpdates.pendingDeletions;
return state.pendingDeletions;
}
/**
* Return a total count of pending updates and deletions.
*/
const pendingUpdateCount = createSelector(
state => [
state.realTimeUpdates.pendingUpdates,
state.realTimeUpdates.pendingDeletions,
],
state => [state.pendingUpdates, state.pendingDeletions],
([pendingUpdates, pendingDeletions]) =>
Object.keys(pendingUpdates).length + Object.keys(pendingDeletions).length
);
......@@ -173,7 +170,7 @@ const pendingUpdateCount = createSelector(
* has not yet been applied.
*/
function hasPendingDeletion(state, id) {
return state.realTimeUpdates.pendingDeletions.hasOwnProperty(id);
return state.pendingDeletions.hasOwnProperty(id);
}
export default {
......
......@@ -45,7 +45,7 @@ function changeRoute(name, params = {}) {
* Return the name of the current route.
*/
function route(state) {
return state.route.name;
return state.name;
}
/**
......@@ -53,7 +53,7 @@ function route(state) {
* query string.
*/
function routeParams(state) {
return state.route.params;
return state.params;
}
export default {
......
......@@ -390,11 +390,11 @@ function toggleSelectedAnnotations(toggleIds) {
* @return {Object<string,boolean>}
*/
function expandedMap(state) {
return state.selection.expanded;
return state.expanded;
}
function filterQuery(state) {
return state.selection.filterQuery;
return state.filterQuery;
}
/**
......@@ -403,7 +403,7 @@ function filterQuery(state) {
* @return {boolean}
*/
function focusModeActive(state) {
return state.selection.focusMode.active;
return state.focusMode.active;
}
/**
......@@ -414,7 +414,7 @@ function focusModeActive(state) {
* @return {boolean}
*/
function focusModeConfigured(state) {
return state.selection.focusMode.configured;
return state.focusMode.configured;
}
/**
......@@ -426,7 +426,7 @@ function focusModeUserFilter(state) {
if (!focusModeActive(state)) {
return null;
}
return state.selection.focusMode.user.filter;
return state.focusMode.user.filter;
}
/**
......@@ -440,11 +440,11 @@ function focusModeUserPrettyName(state) {
if (!focusModeConfigured(state)) {
return '';
}
return state.selection.focusMode.user.displayName;
return state.focusMode.user.displayName;
}
const forcedVisibleAnnotations = createSelector(
state => state.selection.forcedVisible,
state => state.forcedVisible,
forcedVisible => trueKeys(forcedVisible)
);
......@@ -455,7 +455,7 @@ const forcedVisibleAnnotations = createSelector(
* @return {string|null}
*/
const getFirstSelectedAnnotationId = createSelector(
state => state.selection.selected,
state => state.selected,
selection => {
const selectedIds = trueKeys(selection);
return selectedIds.length ? selectedIds[0] : null;
......@@ -468,12 +468,12 @@ const getFirstSelectedAnnotationId = createSelector(
* @return {boolean}
*/
const hasSelectedAnnotations = createSelector(
state => state.selection.selected,
state => state.selected,
selection => trueKeys(selection).length > 0
);
const selectedAnnotations = createSelector(
state => state.selection.selected,
state => state.selected,
selection => trueKeys(selection)
);
......@@ -499,7 +499,7 @@ const hasAppliedFilter = createSelector(
*/
function sortKeys(state) {
const sortKeysForTab = ['Newest', 'Oldest'];
if (state.selection.selectedTab !== uiConstants.TAB_NOTES) {
if (state.selectedTab !== uiConstants.TAB_NOTES) {
// Location is inapplicable to Notes tab
sortKeysForTab.push('Location');
}
......
......@@ -50,7 +50,7 @@ function updateProfile(profile) {
* @param {object} state - The application state
*/
function isLoggedIn(state) {
return state.session.profile.userid !== null;
return state.profile.userid !== null;
}
/**
......@@ -61,7 +61,7 @@ function isLoggedIn(state) {
* name of the feature flag as declared in the Hypothesis service.
*/
function isFeatureEnabled(state, feature) {
return !!state.session.profile.features[feature];
return !!state.profile.features[feature];
}
/**
......@@ -70,7 +70,7 @@ function isFeatureEnabled(state, feature) {
* logged-out user profile returned by the server.
*/
function hasFetchedProfile(state) {
return state.session.profile !== initialProfile;
return state.profile !== initialProfile;
}
/**
......@@ -82,7 +82,7 @@ function hasFetchedProfile(state) {
* returned. This allows code to skip a null check.
*/
function profile(state) {
return state.session.profile;
return state.profile;
}
export default {
......
......@@ -108,7 +108,7 @@ function toggleSidebarPanel(panelName, panelState) {
* @return {Boolean} - `true` if `panelName` is the currently-active panel
*/
function isSidebarPanelOpen(state, panelName) {
return state.sidebarPanels.activePanelName === panelName;
return state.activePanelName === panelName;
}
export default {
......
......@@ -459,4 +459,18 @@ describe('sidebar/store/modules/annotations', function () {
});
});
});
describe('#annotationExists', () => {
it('returns false if annotation does not exist', () => {
const store = createTestStore();
assert.isFalse(store.annotationExists('foobar'));
});
it('returns true if annotation does exist', () => {
const store = createTestStore();
const annot = fixtures.defaultAnnotation();
store.addAnnotations([annot]);
assert.isTrue(store.annotationExists(annot.id));
});
});
});
import createStore from '../../create-store';
import route from '../route';
describe('store/modules/route', () => {
let store;
beforeEach(() => {
store = createStore([route]);
});
it('sets initial route to `null`', () => {
assert.equal(store.route(), null);
});
it('sets initial params to `{}`', () => {
assert.deepEqual(store.routeParams(), {});
});
describe('#changeRoute', () => {
it('sets the current route name and params', () => {
store.changeRoute('stream', { q: 'some-query' });
assert.equal(store.route(), 'stream');
assert.deepEqual(store.routeParams(), { q: 'some-query' });
});
});
});
......@@ -71,7 +71,7 @@ function updateMessage(message) {
* @return {Object|undefined}
*/
function getMessage(state, id) {
return state.toastMessages.messages.find(message => message.id === id);
return state.messages.find(message => message.id === id);
}
/**
......@@ -80,7 +80,7 @@ function getMessage(state, id) {
* @return {Object[]}
*/
function getMessages(state) {
return state.toastMessages.messages;
return state.messages;
}
/**
......@@ -93,7 +93,7 @@ function getMessages(state) {
* @return {boolean}
*/
function hasMessage(state, type, text) {
return state.toastMessages.messages.some(message => {
return state.messages.some(message => {
return message.type === type && message.message === text;
});
}
......
......@@ -50,7 +50,7 @@ function setSidebarOpened(opened) {
// Selectors
function hasSidebarOpened(state) {
return state.viewer.sidebarHasOpened;
return state.sidebarHasOpened;
}
export default {
......
......@@ -29,6 +29,12 @@ const modules = [
selectors: {
getCountA(state) {
return state.count;
},
},
rootSelectors: {
getCountAFromRoot(state) {
return state.a.count;
},
},
......@@ -57,7 +63,7 @@ const modules = [
selectors: {
getCountB(state) {
return state.b.count;
return state.count;
},
},
},
......@@ -106,6 +112,12 @@ describe('sidebar/store/create-store', () => {
assert.equal(store.getCountA(), 5);
});
it('adds root selectors as methods to the store', () => {
const store = counterStore();
store.dispatch(modules[A].actions.incrementA(5));
assert.equal(store.getCountAFromRoot(), 5);
});
it('applies `thunk` middleware by default', () => {
const store = counterStore();
const doubleAction = (dispatch, getState) => {
......
......@@ -15,23 +15,29 @@ const fixtures = {
selectors: {
namespace1: {
selectors: {
countAnnotations1: function (state) {
return state.namespace1.annotations.length;
},
countAnnotations1: localState => localState.annotations.length,
},
rootSelectors: {
rootCountAnnotations1: rootState =>
rootState.namespace1.annotations.length,
},
},
namespace2: {
selectors: {
countAnnotations2: function (state) {
return state.namespace2.annotations.length;
},
countAnnotations2: localState => localState.annotations.length,
},
rootSelectors: {
rootCountAnnotations2: rootState =>
rootState.namespace2.annotations.length,
},
},
},
};
describe('reducer utils', function () {
describe('#actionTypes', function () {
describe('sidebar/store/util', function () {
describe('actionTypes', function () {
it('returns an object with values equal to keys', function () {
assert.deepEqual(
util.actionTypes({
......@@ -46,7 +52,7 @@ describe('reducer utils', function () {
});
});
describe('#createReducer', 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);
......@@ -120,8 +126,8 @@ describe('reducer utils', function () {
});
});
describe('#bindSelectors', function () {
it('bound functions call original functions with current value of getState()', function () {
describe('bindSelectors', function () {
it('binds selectors to current value of module state', () => {
const getState = sinon.stub().returns({
namespace1: {
annotations: [{ id: 1 }],
......@@ -134,5 +140,19 @@ describe('reducer utils', function () {
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);
});
});
});
......@@ -44,16 +44,19 @@ export function createReducer(actionToUpdateFn) {
* selectors with the `state` argument set to the current value of `getState()`.
*/
export function bindSelectors(namespaces, getState) {
const totalSelectors = {};
const boundSelectors = {};
Object.keys(namespaces).forEach(namespace => {
const selectors = namespaces[namespace].selectors;
const { selectors, rootSelectors = {} } = namespaces[namespace];
Object.keys(selectors).forEach(selector => {
totalSelectors[selector] = function () {
const args = [].slice.apply(arguments);
args.unshift(getState());
return selectors[selector].apply(null, args);
};
boundSelectors[selector] = (...args) =>
selectors[selector](getState()[namespace], ...args);
});
Object.keys(rootSelectors).forEach(selector => {
boundSelectors[selector] = (...args) =>
rootSelectors[selector](getState(), ...args);
});
});
return totalSelectors;
return boundSelectors;
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment