Modernize and simplify `features` service

 - Convert `features` service to a class

 - Remove the unused `flagEnabled` method. Calls to this method have
   gradually been replaced by `store.isFeatureEnabled`.

 - Move the initialization of the background sidebar => annotator
   synchronization of feature flag information into an `init` method
   which is then called from the sidebar's entry point. This pattern is
   consistent with several other services. It is also necessary for services
   that are not depended upon by anything else because services are
   lazily instantiated.

src/sidebar/index.js

@@ -57,24 +57,20 @@ function setupRoute(groups, session, router) {
 }
 
 /**
- * Fetch any persisted client-side defaults, and persist any app-state changes to
- * those defaults
+ * Initialize background processes provided by various services.
  *
- * @param {import('./services/persisted-defaults').PersistedDefaultsService} persistedDefaults
- * @inject
- */
-function persistDefaults(persistedDefaults) {
-  persistedDefaults.init();
-}
-
-/**
- * Set up autosave-new-highlights service
+ * These processes include persisting or synchronizing data from one place
+ * to another.
  *
  * @param {import('./services/autosave').AutosaveService} autosaveService
+ * @param {import('./services/features').FeaturesService} features
+ * @param {import('./services/persisted-defaults').PersistedDefaultsService} persistedDefaults
  * @inject
  */
-function autosave(autosaveService) {
+function initServices(autosaveService, features, persistedDefaults) {
   autosaveService.init();
+  features.init();
+  persistedDefaults.init();
 }
 
 // @inject
@@ -104,7 +100,7 @@ import apiService from './services/api';
 import { APIRoutesService } from './services/api-routes';
 import authService from './services/oauth-auth';
 import { AutosaveService } from './services/autosave';
-import featuresService from './services/features';
+import { FeaturesService } from './services/features';
 import frameSyncService from './services/frame-sync';
 import groupsService from './services/groups';
 import loadAnnotationsService from './services/load-annotations';
@@ -142,7 +138,7 @@ function startApp(config, appEl) {
     .register('auth', authService)
     .register('autosaveService', AutosaveService)
     .register('bridge', bridgeService)
-    .register('features', featuresService)
+    .register('features', FeaturesService)
     .register('frameSync', frameSyncService)
     .register('groups', groupsService)
     .register('loadAnnotationsService', loadAnnotationsService)
@@ -167,8 +163,7 @@ function startApp(config, appEl) {
     .register('settings', { value: config });
 
   // Initialize services.
-  container.run(persistDefaults);
-  container.run(autosave);
+  container.run(initServices);
   container.run(setupApi);
   container.run(setupRoute);
   container.run(startRPCServer);

src/sidebar/services/features.js

+import bridgeEvents from '../../shared/bridge-events';
+import { watch } from '../util/watch';
+
 /**
- * Provides access to feature flag states for the current
- * Hypothesis user.
+ * Service that provides operations related to feature flags.
  *
- * This service is a thin wrapper around the feature flag data in
- * the session state.
+ * Feature flags information is part of the user's profile and in the sidebar
+ * is accessed via the store. This service synchronizes the state of feature
+ * flags to the `annotator` side of the application.
  *
- * Users of this service should assume that the value of any given flag can
- * change at any time and should write code accordingly. Feature flags should
- * not be cached, and should not be interrogated only at setup time.
+ * Note that the state of feature flags can change whenever the active profile
+ * information changes.
+ *
+ * @inject
  */
+export class FeaturesService {
+  /**
+   * @param {import('../../shared/bridge').default} bridge
+   * @param {import('../store').SidebarStore} store
+   */
+  constructor(bridge, store) {
+    this._bridge = bridge;
+    this._store = store;
+  }
 
-import bridgeEvents from '../../shared/bridge-events';
-import warnOnce from '../../shared/warn-once';
-import { watch } from '../util/watch';
-
-// @inject
-export default function features(bridge, session, store) {
-  const currentFlags = () => store.profile().features;
+  init() {
+    const currentFlags = () => this._store.profile().features;
     const sendFeatureFlags = () => {
-    bridge.call(bridgeEvents.FEATURE_FLAGS_UPDATED, currentFlags() || {});
+      this._bridge.call(
+        bridgeEvents.FEATURE_FLAGS_UPDATED,
+        currentFlags() || {}
+      );
     };
 
     // Re-send feature flags to connected frames when flags change or a new
     // frame connects.
-  watch(store.subscribe, [currentFlags, () => store.frames()], () =>
-    sendFeatureFlags()
+    watch(
+      this._store.subscribe,
+      [currentFlags, () => this._store.frames()],
+      () => sendFeatureFlags()
     );
-
-  /**
-   * Returns true if the flag with the given name is enabled for the current
-   * user.
-   *
-   * Returns false if session data has not been fetched for the current
-   * user yet or if the feature flag name is unknown.
-   */
-  function flagEnabled(flag) {
-    // trigger a refresh of session data, if it has not been
-    // refetched within a cache timeout managed by the session service
-    // (see CACHE_TTL in session.js)
-    session.load();
-
-    const flags = currentFlags();
-
-    if (!flags) {
-      // features data has not yet been fetched
-      return false;
   }
-
-    if (!(flag in flags)) {
-      warnOnce('looked up unknown feature', flag);
-      return false;
-    }
-    return flags[flag];
-  }
-
-  return {
-    flagEnabled: flagEnabled,
-  };
 }

src/sidebar/services/test/features-test.js

 import bridgeEvents from '../../../shared/bridge-events';
-import features from '../features';
-import { $imports } from '../features';
+import { FeaturesService } from '../features';
 
-describe('sidebar/services/features', function () {
+describe('FeaturesService', () => {
   let fakeBridge;
-  let fakeWarnOnce;
-  let fakeSession;
   let fakeStore;
 
-  beforeEach(function () {
+  beforeEach(() => {
     fakeBridge = {
       call: sinon.stub(),
     };
 
-    fakeWarnOnce = sinon.stub();
-
-    fakeSession = {
-      load: sinon.stub(),
-    };
-
     fakeStore = {
       subscribe: sinon.stub(),
       frames: sinon.stub().returns([]),
@@ -29,46 +20,14 @@ describe('sidebar/services/features', function () {
         },
       }),
     };
-
-    $imports.$mock({
-      '../../shared/warn-once': fakeWarnOnce,
-    });
-  });
-
-  afterEach(function () {
-    $imports.$restore();
   });
 
   function createService() {
-    return features(fakeBridge, fakeSession, fakeStore);
+    const service = new FeaturesService(fakeBridge, fakeStore);
+    service.init();
+    return service;
   }
 
-  describe('flagEnabled', function () {
-    it('should retrieve features data', function () {
-      const features_ = createService();
-      assert.equal(features_.flagEnabled('feature_on'), true);
-      assert.equal(features_.flagEnabled('feature_off'), false);
-    });
-
-    it('should return false if features have not been loaded', function () {
-      const features_ = createService();
-      // Simulate feature data not having been loaded yet
-      fakeStore.profile.returns({});
-      assert.equal(features_.flagEnabled('feature_on'), false);
-    });
-
-    it('should trigger a refresh of session data', function () {
-      const features_ = createService();
-      features_.flagEnabled('feature_on');
-      assert.calledOnce(fakeSession.load);
-    });
-
-    it('should return false for unknown flags', function () {
-      const features_ = createService();
-      assert.isFalse(features_.flagEnabled('unknown_feature'));
-    });
-  });
-
   function notifyStoreSubscribers() {
     const subscribers = fakeStore.subscribe.args.map(args => args[0]);
     subscribers.forEach(s => s());

src/sidebar/test/integration/threading-test.js

@@ -45,14 +45,9 @@ describe('integration: annotation threading', () => {
   let forceUpdate;
 
   beforeEach(function () {
-    const fakeFeatures = {
-      flagEnabled: sinon.stub().returns(true),
-    };
-
     const container = new Injector()
       .register('store', storeFactory)
       .register('annotationsService', () => {})
-      .register('features', { value: fakeFeatures })
       .register('settings', { value: {} });
 
     // Mount a dummy component to be able to use the `useRootThread` hook