Refactor `SessionService` to a more idiomatic ES class

 - Convert closures into methods

 - Remove the `profileFetchRetryOpts` field that was used only to shorten the
   delay between retries in tests, in favor of mocking
   `retryPromiseOperation` in tests to remove the delay between retries.

 - Change `SessionService` instance creation in tests to allow
   individual tests to run custom setup logic before the service is
   constructed. This was needed due to allow the `serviceConfig` mock to
   take effect when the `SessionService` constructor runs

 - Remove unnecessary custom Sinon sandbox in tests

src/sidebar/index.js

@@ -46,7 +46,7 @@ function setupApi(api, streamer) {
  * route to match the current URL.
  *
  * @param {Object} groups
- * @param {Object} session
+ * @param {import('./services/session').SessionService} session
  * @param {import('./services/router').RouterService} router
  */
 // @inject

src/sidebar/services/session.js

 import serviceConfig from '../config/service-config';
-import * as retryUtil from '../util/retry';
+import { retryPromiseOperation } from '../util/retry';
 import * as sentry from '../util/sentry';
 
 const CACHE_TTL = 5 * 60 * 1000; // 5 minutes
@@ -20,22 +20,22 @@ export class SessionService {
    * @param {import('./toast-messenger').ToastMessengerService} toastMessenger
    */
   constructor(store, api, auth, settings, toastMessenger) {
-    // Cache the result of load()
-    let lastLoad;
-    let lastLoadTime;
-
-    // Return the authority from the first service defined in the settings.
-    // Return null if there are no services defined in the settings.
-    function getAuthority() {
-      const service = serviceConfig(settings);
-      if (service === null) {
-        return null;
-      }
-      return service.authority;
-    }
+    this._api = api;
+    this._auth = auth;
+    this._store = store;
+    this._toastMessenger = toastMessenger;
+
+    this._authority = serviceConfig(settings)?.authority ?? null;
 
-    // Options to pass to `retry.operation` when fetching the user's profile.
-    const profileFetchRetryOpts = {};
+    /** @type {Promise<Profile>|null} */
+    this._lastLoad = null;
+
+    /** @type {number|null} */
+    this._lastLoadTime = null;
+
+    // Re-fetch profile when user logs in or out in another tab.
+    auth.on('oauthTokensChanged', () => this.reload());
+  }
 
   /**
    * Fetch the user's profile from the annotation service.
@@ -45,44 +45,45 @@ export class SessionService {
    *
    * @return {Promise<Profile>} A promise for the user's profile data.
    */
-    function load() {
-      if (!lastLoadTime || Date.now() - lastLoadTime > CACHE_TTL) {
+  load() {
+    if (
+      !this._lastLoad ||
+      !this._lastLoadTime ||
+      Date.now() - this._lastLoadTime > CACHE_TTL
+    ) {
       // The load attempt is automatically retried with a backoff.
       //
       // This serves to make loading the app in the extension cope better with
       // flakey connectivity but it also throttles the frequency of calls to
       // the /app endpoint.
-        lastLoadTime = Date.now();
-        lastLoad = retryUtil
-          .retryPromiseOperation(function () {
-            const authority = getAuthority();
-            const opts = {};
-            if (authority) {
-              opts.authority = authority;
-            }
-            return api.profile.read(opts);
-          }, profileFetchRetryOpts)
-          .then(function (session) {
-            update(session);
-            lastLoadTime = Date.now();
+      this._lastLoadTime = Date.now();
+      this._lastLoad = retryPromiseOperation(() => {
+        const opts = this._authority ? { authority: this._authority } : {};
+        return this._api.profile.read(opts);
+      })
+        .then(session => {
+          this.update(session);
+          this._lastLoadTime = Date.now();
           return session;
         })
-          .catch(function (err) {
-            lastLoadTime = null;
+        .catch(err => {
+          this._lastLoadTime = null;
           throw err;
         });
     }
-      return lastLoad;
+    return this._lastLoad;
   }
 
   /**
    * Store the preference server-side that the user dismissed the sidebar
    * tutorial and then update the local profile data.
    */
-    function dismissSidebarTutorial() {
-      return api.profile
-        .update({}, { preferences: { show_sidebar_tutorial: false } })
-        .then(update);
+  async dismissSidebarTutorial() {
+    const updatedProfile = await this._api.profile.update(
+      {},
+      { preferences: { show_sidebar_tutorial: false } }
+    );
+    this.update(updatedProfile);
   }
 
   /**
@@ -91,46 +92,43 @@ export class SessionService {
    * This method can be used to update the profile data in the client when new
    * data is pushed from the server via the real-time API.
    *
-     * @param {Profile} model
+   * @param {Profile} profile
    * @return {Profile} The updated profile data
    */
-    function update(model) {
-      const prevSession = store.profile();
-      const userChanged = model.userid !== prevSession.userid;
+  update(profile) {
+    const prevProfile = this._store.profile();
+    const userChanged = profile.userid !== prevProfile.userid;
 
-      store.updateProfile(model);
+    this._store.updateProfile(profile);
 
-      lastLoad = Promise.resolve(model);
-      lastLoadTime = Date.now();
+    this._lastLoad = Promise.resolve(profile);
+    this._lastLoadTime = Date.now();
 
     if (userChanged) {
       // Associate error reports with the current user in Sentry.
-        if (model.userid) {
+      if (profile.userid) {
         sentry.setUserInfo({
-            id: model.userid,
+          id: profile.userid,
         });
       } else {
         sentry.setUserInfo(null);
       }
     }
 
-      // Return the model
-      return model;
+    return profile;
   }
 
   /**
-     * Log the user out of the current session.
+   * Log the user out of the current session and re-fetch the profile.
    */
-    function logout() {
-      const loggedOut = auth.logout().then(() => {
-        // Re-fetch the logged-out user's profile.
-        return reload();
-      });
-
-      return loggedOut.catch(err => {
-        toastMessenger.error('Log out failed');
+  async logout() {
+    try {
+      await this._auth.logout();
+      return this.reload();
+    } catch (err) {
+      this._toastMessenger.error('Log out failed');
       throw new Error(err);
-      });
+    }
   }
 
   /**
@@ -140,24 +138,9 @@ export class SessionService {
    *
    * @return {Promise<Profile>}
    */
-    function reload() {
-      lastLoad = null;
-      lastLoadTime = null;
-      return load();
-    }
-
-    auth.on('oauthTokensChanged', () => {
-      reload();
-    });
-
-    this.dismissSidebarTutorial = dismissSidebarTutorial;
-    this.load = load;
-    this.logout = logout;
-    this.reload = reload;
-
-    // Exposed for use in tests
-    this.profileFetchRetryOpts = profileFetchRetryOpts;
-
-    this.update = update;
+  reload() {
+    this._lastLoad = null;
+    this._lastLoadTime = null;
+    return this.load();
   }
 }

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

 import EventEmitter from 'tiny-emitter';
 
 import { SessionService, $imports } from '../session';
-import { Injector } from '../../../shared/injector';
 
-describe('SessionService', function () {
+describe('SessionService', () => {
+  let fakeApi;
   let fakeAuth;
   let fakeSentry;
   let fakeServiceConfig;
   let fakeSettings;
   let fakeStore;
   let fakeToastMessenger;
-  let fakeApi;
-  let sandbox;
-
-  // The instance of the `session` service.
-  let session;
-
-  beforeEach(function () {
-    sandbox = sinon.createSandbox();
 
+  beforeEach(() => {
     let currentProfile = {
       userid: null,
     };
@@ -30,47 +23,59 @@ describe('SessionService', function () {
       }),
     };
     fakeAuth = Object.assign(new EventEmitter(), {
-      login: sandbox.stub().returns(Promise.resolve()),
+      login: sinon.stub().returns(Promise.resolve()),
       logout: sinon.stub().resolves(),
     });
     fakeSentry = {
-      setUserInfo: sandbox.spy(),
+      setUserInfo: sinon.spy(),
     };
     fakeApi = {
       profile: {
-        read: sandbox.stub().resolves(),
-        update: sandbox.stub().resolves({}),
+        read: sinon.stub().resolves(),
+        update: sinon.stub().resolves({}),
       },
     };
     fakeServiceConfig = sinon.stub().returns(null);
     fakeSettings = {
       serviceUrl: 'https://test.hypothes.is/root/',
     };
-    fakeToastMessenger = { error: sandbox.spy() };
+    fakeToastMessenger = { error: sinon.spy() };
+
+    const retryPromiseOperation = async callback => {
+      // eslint-disable-next-line no-constant-condition
+      while (true) {
+        try {
+          return await callback();
+        } catch (err) {
+          // Try again
+        }
+      }
+    };
 
     $imports.$mock({
       '../config/service-config': fakeServiceConfig,
+      '../util/retry': { retryPromiseOperation },
       '../util/sentry': fakeSentry,
     });
-
-    session = new Injector()
-      .register('store', { value: fakeStore })
-      .register('api', { value: fakeApi })
-      .register('auth', { value: fakeAuth })
-      .register('settings', { value: fakeSettings })
-      .register('session', SessionService)
-      .register('toastMessenger', { value: fakeToastMessenger })
-      .get('session');
   });
 
-  afterEach(function () {
+  afterEach(() => {
     $imports.$restore();
-    sandbox.restore();
   });
 
-  describe('#load', function () {
-    context('when the host page provides an OAuth grant token', function () {
-      beforeEach(function () {
+  function createService() {
+    return new SessionService(
+      fakeStore,
+      fakeApi,
+      fakeAuth,
+      fakeSettings,
+      fakeToastMessenger
+    );
+  }
+
+  describe('#load', () => {
+    context('when the host page provides an OAuth grant token', () => {
+      beforeEach(() => {
         fakeServiceConfig.returns({
           authority: 'publisher.org',
           grantToken: 'a.jwt.token',
@@ -82,16 +87,18 @@ describe('SessionService', function () {
         );
       });
 
-      it('should pass the "authority" param when fetching the profile', function () {
-        return session.load().then(function () {
+      it('should pass the "authority" param when fetching the profile', () => {
+        const session = createService();
+        return session.load().then(() => {
           assert.calledWith(fakeApi.profile.read, {
             authority: 'publisher.org',
           });
         });
       });
 
-      it('should update the session with the profile data from the API', function () {
-        return session.load().then(function () {
+      it('should update the session with the profile data from the API', () => {
+        const session = createService();
+        return session.load().then(() => {
           assert.calledWith(fakeStore.updateProfile, {
             userid: 'acct:user@publisher.org',
           });
@@ -117,6 +124,7 @@ describe('SessionService', function () {
       });
 
       it('should fetch profile data from the API', () => {
+        const session = createService();
         return session.load().then(() => {
           assert.calledWith(fakeApi.profile.read);
         });
@@ -133,9 +141,7 @@ describe('SessionService', function () {
           .returns(Promise.reject(new Error('Server error')));
         fakeApi.profile.read.onCall(1).returns(Promise.resolve(fetchedProfile));
 
-        // Shorten the delay before retrying the fetch.
-        session.profileFetchRetryOpts.minTimeout = 50;
-
+        const session = createService();
         return session.load().then(() => {
           assert.calledOnce(fakeStore.updateProfile);
           assert.calledWith(fakeStore.updateProfile, fetchedProfile);
@@ -143,7 +149,8 @@ describe('SessionService', function () {
       });
 
       it('should update the session with the profile data from the API', () => {
-        return session.load().then(function () {
+        const session = createService();
+        return session.load().then(() => {
           assert.calledOnce(fakeStore.updateProfile);
           assert.calledWith(fakeStore.updateProfile, {
             userid: 'acct:user@hypothes.is',
@@ -152,6 +159,7 @@ describe('SessionService', function () {
       });
 
       it('should cache the returned profile data', () => {
+        const session = createService();
         return session
           .load()
           .then(() => {
@@ -166,6 +174,7 @@ describe('SessionService', function () {
         clock = sinon.useFakeTimers();
         const CACHE_TTL = 5 * 60 * 1000; // 5 minutes
 
+        const session = createService();
         return session
           .load()
           .then(() => {
@@ -179,8 +188,9 @@ describe('SessionService', function () {
     });
   });
 
-  describe('#update', function () {
-    it('updates the user ID for Sentry error reports', function () {
+  describe('#update', () => {
+    it('updates the user ID for Sentry error reports', () => {
+      const session = createService();
       session.update({
         userid: 'anne',
       });
@@ -190,8 +200,8 @@ describe('SessionService', function () {
     });
   });
 
-  describe('#dismissSidebarTutorial', function () {
-    beforeEach(function () {
+  describe('#dismissSidebarTutorial', () => {
+    beforeEach(() => {
       fakeApi.profile.update.returns(
         Promise.resolve({
           preferences: {},
@@ -199,7 +209,8 @@ describe('SessionService', function () {
       );
     });
 
-    it('disables the tutorial for the user', function () {
+    it('disables the tutorial for the user', () => {
+      const session = createService();
       session.dismissSidebarTutorial();
       assert.calledWith(
         fakeApi.profile.update,
@@ -208,8 +219,9 @@ describe('SessionService', function () {
       );
     });
 
-    it('should update the session with the response from the API', function () {
-      return session.dismissSidebarTutorial().then(function () {
+    it('should update the session with the response from the API', () => {
+      const session = createService();
+      return session.dismissSidebarTutorial().then(() => {
         assert.calledOnce(fakeStore.updateProfile);
         assert.calledWith(fakeStore.updateProfile, {
           preferences: {},
@@ -226,6 +238,7 @@ describe('SessionService', function () {
           userid: 'acct:user_a@hypothes.is',
         })
       );
+      const session = createService();
       return session.load();
     });
 
@@ -238,6 +251,7 @@ describe('SessionService', function () {
 
       fakeStore.updateProfile.resetHistory();
 
+      const session = createService();
       return session.reload().then(() => {
         assert.calledOnce(fakeStore.updateProfile);
         assert.calledWith(fakeStore.updateProfile, {
@@ -247,7 +261,7 @@ describe('SessionService', function () {
     });
   });
 
-  describe('#logout', function () {
+  describe('#logout', () => {
     const loggedOutProfile = {
       userid: null,
 
@@ -261,12 +275,14 @@ describe('SessionService', function () {
     });
 
     it('logs the user out', () => {
+      const session = createService();
       return session.logout().then(() => {
         assert.called(fakeAuth.logout);
       });
     });
 
     it('updates the profile after logging out', () => {
+      const session = createService();
       return session.logout().then(() => {
         assert.calledOnce(fakeStore.updateProfile);
         assert.calledWith(fakeStore.updateProfile, loggedOutProfile);
@@ -275,6 +291,7 @@ describe('SessionService', function () {
 
     it('displays an error if logging out fails', async () => {
       fakeAuth.logout.rejects(new Error('Could not revoke token'));
+      const session = createService();
       try {
         await session.logout();
       } catch (e) {
@@ -292,6 +309,7 @@ describe('SessionService', function () {
         })
       );
 
+      const session = createService();
       return session
         .load()
         .then(() => {