Merge pull request #1146 from hypothesis/use-store-hook

Add utility to re-render components when store changes

package.json

@@ -100,6 +100,7 @@
     "retry": "^0.12.0",
     "scroll-into-view": "^1.8.2",
     "seamless-immutable": "^7.1.4",
+    "shallowequal": "^1.1.0",
     "showdown": "^1.6.4",
     "sinon": "^7.2.3",
     "stringify": "^5.1.0",

src/sidebar/components/group-list-item.js

@@ -4,18 +4,26 @@ const classnames = require('classnames');
 const propTypes = require('prop-types');
 const { createElement } = require('preact');
 
+const useStore = require('../store/use-store');
 const { orgName } = require('../util/group-list-item-common');
 const { withServices } = require('../util/service-context');
 
-function GroupListItem({ analytics, group, store }) {
+function GroupListItem({ analytics, group }) {
+  const actions = useStore(store => ({
+    clearDirectLinkedGroupFetchFailed: store.clearDirectLinkedGroupFetchFailed,
+    clearDirectLinkedIds: store.clearDirectLinkedIds,
+    focusGroup: store.focusGroup,
+  }));
+
   const focusGroup = () => {
     analytics.track(analytics.events.GROUP_SWITCH);
-    store.clearDirectLinkedGroupFetchFailed();
-    store.clearDirectLinkedIds();
-    store.focusGroup(group.id);
+    actions.clearDirectLinkedGroupFetchFailed();
+    actions.clearDirectLinkedIds();
+    actions.focusGroup(group.id);
   };
 
-  const isSelected = group.id === store.focusedGroupId();
+  const focusedGroupId = useStore(store => store.focusedGroupId());
+  const isSelected = group.id === focusedGroupId;
   const groupOrgName = orgName(group);
 
   return (
@@ -59,9 +67,8 @@ GroupListItem.propTypes = {
   group: propTypes.object.isRequired,
 
   analytics: propTypes.object.isRequired,
-  store: propTypes.object.isRequired,
 };
 
-GroupListItem.injectedProps = ['analytics', 'store'];
+GroupListItem.injectedProps = ['analytics'];
 
 module.exports = withServices(GroupListItem);

src/sidebar/components/test/group-list-item-test.js

@@ -31,6 +31,7 @@ describe('GroupListItem', () => {
 
     GroupListItem.$imports.$mock({
       '../util/group-list-item-common': fakeGroupListItemCommon,
+      '../store/use-store': callback => callback(fakeStore),
     });
   });
 

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

+'use strict';
+
+const { mount } = require('enzyme');
+const { createStore } = require('redux');
+const { createElement } = require('preact');
+const { act } = require('preact/test-utils');
+
+const useStore = require('../use-store');
+const { $imports } = useStore;
+
+const initialState = { value: 10, otherValue: 20 };
+const reducer = (state = initialState, action) => {
+  if (action.type === 'INCREMENT') {
+    return { ...state, value: state.value + 1 };
+  } else if (action.type === 'INCREMENT_OTHER') {
+    return { ...state, otherValue: state.otherValue + 1 };
+  } else {
+    return state;
+  }
+};
+
+describe('useStore', () => {
+  let renderCount;
+  let testStore;
+  let TestComponent;
+
+  beforeEach(() => {
+    renderCount = 0;
+
+    // eslint-disable-next-line react/display-name
+    TestComponent = () => {
+      renderCount += 1;
+      const aValue = useStore(store => store.getState().value);
+      return <div>{aValue}</div>;
+    };
+
+    testStore = createStore(reducer);
+    $imports.$mock({
+      '../util/service-context': {
+        useService: name => (name === 'store' ? testStore : null),
+      },
+    });
+  });
+
+  afterEach(() => {
+    $imports.$restore();
+  });
+
+  it('returns result of `callback(store)`', () => {
+    const wrapper = mount(<TestComponent />);
+    assert.equal(wrapper.text(), '10');
+  });
+
+  it('re-renders when the store changes and result of `callback(store)` also changes', () => {
+    // An update which changes result of `callback(store)` should cause a re-render.
+    const wrapper = mount(<TestComponent />);
+    act(() => {
+      testStore.dispatch({ type: 'INCREMENT' });
+    });
+    wrapper.update();
+    assert.equal(wrapper.text(), '11');
+
+    // The new result from `callback(store)` should be remembered so that another
+    // update which doesn't change the result doesn't cause a re-render.
+    const prevRenderCount = renderCount;
+    act(() => {
+      testStore.dispatch({ type: 'INCREMENT_OTHER' });
+    });
+    wrapper.update();
+    assert.equal(renderCount, prevRenderCount);
+  });
+
+  it('does not re-render if the result of `callback(store)` did not change', () => {
+    mount(<TestComponent />);
+    const originalRenderCount = renderCount;
+    act(() => {
+      testStore.dispatch({ type: 'INCREMENT_OTHER' });
+    });
+    assert.equal(renderCount, originalRenderCount);
+  });
+
+  it('warns if the callback always returns a different value', () => {
+    const warnOnce = sinon.stub();
+    $imports.$mock({
+      '../util/warn-once': warnOnce,
+    });
+    const BuggyComponent = () => {
+      // The result of the callback is an object with an `aValue` property
+      // which is a new array every time. This causes unnecessary re-renders.
+      useStore(() => ({ aValue: [] }));
+      return null;
+    };
+    mount(<BuggyComponent />);
+    assert.called(warnOnce);
+    assert.match(warnOnce.firstCall.args[0], /changes every time/);
+  });
+
+  it('unsubscribes when the component is unmounted', () => {
+    const unsubscribe = sinon.stub();
+    testStore.subscribe = sinon.stub().returns(unsubscribe);
+
+    const wrapper = mount(<TestComponent />);
+
+    assert.calledOnce(testStore.subscribe);
+    wrapper.unmount();
+    assert.calledOnce(unsubscribe);
+  });
+});

src/sidebar/store/use-store.js

+'use strict';
+
+/* global process */
+
+const shallowEqual = require('shallowequal');
+const { useEffect, useRef, useReducer } = require('preact/hooks');
+
+const { useService } = require('../util/service-context');
+const warnOnce = require('../util/warn-once');
+
+/**
+ * Hook for accessing state or actions from the store inside a component.
+ *
+ * This hook fetches the store using `useService` and returns the result of
+ * passing it to the provided callback. The callback will be re-run whenever
+ * the store updates and the component will be re-rendered if the result of
+ * `callback(store)` changed.
+ *
+ * This ensures that the component updates when relevant store state changes.
+ *
+ * @example
+ *   function MyWidget({ widgetId }) {
+ *     const widget = useStore(store => store.getWidget(widgetId));
+ *     const hideWidget = useStore(store => store.hideWidget);
+ *
+ *     return (
+ *       <div>
+ *         {widget.name}
+ *         <button onClick={() => hideWidget(widgetId)}>Hide</button>
+ *       </div>
+ *     )
+ *   }
+ *
+ * @template T
+ * @param {Function} callback -
+ *   Callback that receives the store as an argument and returns some state
+ *   and/or actions extracted from the store.
+ * @return {T} - The result of `callback(store)`
+ */
+function useStore(callback) {
+  const store = useService('store');
+
+  // Store the last-used callback in a ref so we can access it in the effect
+  // below without having to re-subscribe to the store when it changes.
+  const lastCallback = useRef(null);
+  lastCallback.current = callback;
+
+  const lastResult = useRef(null);
+  lastResult.current = callback(store);
+
+  // Check for a performance issue caused by `callback` returning a different
+  // result on every call, even if the store has not changed.
+  if (process.env.NODE_ENV !== 'production') {
+    if (!shallowEqual(lastResult.current, callback(store))) {
+      warnOnce(
+        'The output of a callback passed to `useStore` changes every time. ' +
+          'This will lead to a component updating more often than necessary.'
+      );
+    }
+  }
+
+  // Abuse `useReducer` to force updates when the store state changes.
+  const [, forceUpdate] = useReducer(x => x + 1, 0);
+
+  // Connect to the store, call `callback(store)` whenever the store changes
+  // and re-render the component if the result changed.
+  useEffect(() => {
+    function checkForUpdate() {
+      const result = lastCallback.current(store);
+      if (shallowEqual(result, lastResult.current)) {
+        return;
+      }
+      lastResult.current = result;
+      forceUpdate();
+    }
+
+    // Check for any changes since the component was rendered.
+    checkForUpdate();
+
+    // Check for updates when the store changes in future.
+    const unsubscribe = store.subscribe(checkForUpdate);
+
+    // Remove the subscription when the component is unmounted.
+    return unsubscribe;
+  }, [forceUpdate, store]);
+
+  return lastResult.current;
+}
+
+module.exports = useStore;

src/sidebar/util/service-context.js

 'use strict';
 
+/* global process */
+
 /**
  * This module provides dependency injection of services into React
  * components via React's "context" API [1].
@@ -70,6 +72,15 @@ function withServices(Component) {
     // the parent component.
     const services = {};
     for (let service of Component.injectedProps) {
+      // Debugging check to make sure the store is used correctly.
+      if (process.env.NODE_ENV !== 'production') {
+        if (service === 'store') {
+          throw new Error(
+            'Do not use `withServices` to inject the `store` service. Use the `useStore` hook instead'
+          );
+        }
+      }
+
       if (!(service in props)) {
         services[service] = $injector.get(service);
       }
@@ -91,7 +102,21 @@ function withServices(Component) {
   return Wrapper;
 }
 
+/**
+ * Hook for looking up a service within a component or a custom hook.
+ *
+ * This is an alternative to `withServices` that is mainly useful in the
+ * context of custom hooks.
+ *
+ * @param {string} service - Name of the service to look up
+ */
+function useService(service) {
+  const injector = useContext(ServiceContext);
+  return injector.get(service);
+}
+
 module.exports = {
   ServiceContext,
   withServices,
+  useService,
 };

src/sidebar/util/test/service-context-test.js

 'use strict';
 
+const { mount } = require('enzyme');
 const propTypes = require('prop-types');
 const { createElement, render } = require('preact');
 
-const { ServiceContext, withServices } = require('../service-context');
+const {
+  ServiceContext,
+  withServices,
+  useService,
+} = require('../service-context');
 
 describe('service-context', () => {
   describe('withServices', () => {
@@ -75,4 +80,25 @@ describe('service-context', () => {
       }, /Missing ServiceContext/);
     });
   });
+
+  describe('useService', () => {
+    it('returns the named service', () => {
+      const injector = {
+        get: sinon
+          .stub()
+          .withArgs('aService')
+          .returns('aValue'),
+      };
+      function TestComponent() {
+        const value = useService('aService');
+        return <div>{value}</div>;
+      }
+      const wrapper = mount(
+        <ServiceContext.Provider value={injector}>
+          <TestComponent />
+        </ServiceContext.Provider>
+      );
+      assert.equal(wrapper.text(), 'aValue');
+    });
+  });
 });

src/sidebar/util/test/warn-once-test.js

+'use strict';
+
+const warnOnce = require('../warn-once');
+
+describe('warnOnce', () => {
+  beforeEach(() => {
+    sinon.stub(console, 'warn');
+    warnOnce.reset();
+  });
+
+  afterEach(() => {
+    console.warn.restore();
+  });
+
+  it('outputs a warning only the first time a given string is passed', () => {
+    warnOnce('something is fishy');
+    assert.calledWith(console.warn, 'something is fishy');
+
+    console.warn.reset();
+    warnOnce('something is fishy');
+    assert.notCalled(console.warn);
+
+    warnOnce('something else is wrong');
+    assert.calledWith(console.warn, 'something else is wrong');
+  });
+});

src/sidebar/util/warn-once.js

+'use strict';
+
+let shownWarnings = {};
+
+/**
+ * Log a warning if it has not already been reported.
+ *
+ * This is useful to avoid spamming the console if a warning is emitted in a
+ * context that may be called frequently.
+ *
+ * @param {string} warning
+ */
+function warnOnce(warning) {
+  if (warning in shownWarnings) {
+    return;
+  }
+  console.warn(warning);
+  shownWarnings[warning] = true;
+}
+
+warnOnce.reset = () => {
+  shownWarnings = {};
+};
+
+module.exports = warnOnce;

src/styles/sidebar/components/group-list-item.scss

@@ -26,10 +26,7 @@
   }
 
   &.is-selected {
-    .name-link {
-      font-size: $body2-font-size;
-      font-weight: 600;
-    }
+    background: $gray-lightest;
   }
 }
 

yarn.lock

@@ -7795,6 +7795,11 @@ sha.js@^2.4.0, sha.js@^2.4.8, sha.js@~2.4.4:
     inherits "^2.0.1"
     safe-buffer "^5.0.1"
 
+shallowequal@^1.1.0:
+  version "1.1.0"
+  resolved "https://registry.yarnpkg.com/shallowequal/-/shallowequal-1.1.0.tgz#188d521de95b9087404fd4dcb68b13df0ae4e7f8"
+  integrity sha512-y0m1JoUZSlPAjXVtPPW70aZWfIL/dSP7AFkRnniLCrK/8MDKog3TySTBmckD+RObVxH0v4Tox67+F14PdED2oQ==
+
 shasum@^1.0.0:
   version "1.0.2"
   resolved "https://registry.yarnpkg.com/shasum/-/shasum-1.0.2.tgz#e7012310d8f417f4deb5712150e5678b87ae565f"