Add `allowInBrowserExt` and `defaultValue` to configDefinitions

- Logic for `allowInBrowserExt` and `defaultValue` are moved up into getConfig() from various places inside settings. There are no external changes here, but there are a few subtle changes to how a config values work.

  1. Previously if in a browser ext context and  `allowInBrowserEx` was false (default), then the value was was changed to null. This bit of logic was not entirely explicit, but now the logic is that the value will be set to the `defaultValue` if provided or otherwise the value will be omitted entirely. Currently, all values have a `defaultValue` and most of the defaults are null. This ensures things work the same, but also helps bring clarity to what values will be transformed under this condition. Later we can revisit these config values and determine if any don't need a default or simply leave them as is.

  2. The `defaultValue` was previously only used for several fields, but now its set for every field for clarity. It should also be noted that the `defaultValue` will now take precedent over the `coerce` method if settings returns an undefined value for that config key. This is in contrast to how it worked before, but was problematic under some possible circumstances. For example, if a settings value was not found or undefined, and a `defaultValue` was true and the coerce was `toBoolean`, then the value would become false rather than true and that is confusing because the default would not work due to the `coerce` taking precedent. This is now fixed.

- Remove default export from is-browser-extension

src/annotator/config/index.js

+import { isBrowserExtension } from './is-browser-extension';
 import settingsFrom from './settings';
 import { toBoolean } from '../../shared/type-coercions';
+import { urlFromLinkTag } from './url-from-link-tag';
 
 /**
  * @typedef {'sidebar'|'notebook'|'annotator'|'all'} AppContext
@@ -9,6 +11,11 @@ import { toBoolean } from '../../shared/type-coercions';
  * @typedef ConfigDefinition
  * @prop {(settings: SettingsGetters) => any} getValue -
  *  Method to retrieve the value from the incoming source
+ * @prop {boolean} allowInBrowserExt -
+ *  Allow this setting to be read in the browser extension. If this is false
+ *  and browser extension context is true, use `defaultValue` if provided otherwise
+ *  ignore the config key
+ * @prop {any} [defaultValue] - Sets a default if `getValue` returns undefined
  * @prop {(value: any) => any} [coerce] - Transform a value's type, value or both
  */
 
@@ -76,75 +83,104 @@ function configurationKeys(appContext) {
  */
 const configDefinitions = {
   annotations: {
+    allowInBrowserExt: true,
+    defaultValue: null,
     getValue: settings => settings.annotations,
   },
   appType: {
-    getValue: settings =>
-      settings.hostPageSetting('appType', {
     allowInBrowserExt: true,
-      }),
+    defaultValue: null,
+    getValue: settings => settings.hostPageSetting('appType'),
   },
   branding: {
+    defaultValue: null,
+    allowInBrowserExt: false,
     getValue: settings => settings.hostPageSetting('branding'),
   },
   // URL of the client's boot script. Used when injecting the client into
   // child iframes.
   clientUrl: {
+    allowInBrowserExt: true,
+    defaultValue: null,
     getValue: settings => settings.clientUrl,
   },
   enableExperimentalNewNoteButton: {
+    allowInBrowserExt: false,
+    defaultValue: null,
     getValue: settings =>
       settings.hostPageSetting('enableExperimentalNewNoteButton'),
   },
   group: {
+    allowInBrowserExt: true,
+    defaultValue: null,
     getValue: settings => settings.group,
   },
   focus: {
+    allowInBrowserExt: false,
+    defaultValue: null,
     getValue: settings => settings.hostPageSetting('focus'),
   },
   theme: {
+    allowInBrowserExt: false,
+    defaultValue: null,
     getValue: settings => settings.hostPageSetting('theme'),
   },
   usernameUrl: {
+    allowInBrowserExt: false,
+    defaultValue: null,
     getValue: settings => settings.hostPageSetting('usernameUrl'),
   },
   onLayoutChange: {
+    allowInBrowserExt: false,
+    defaultValue: null,
     getValue: settings => settings.hostPageSetting('onLayoutChange'),
   },
   openSidebar: {
-    coerce: toBoolean,
-    getValue: settings =>
-      settings.hostPageSetting('openSidebar', {
     allowInBrowserExt: true,
-      }),
+    defaultValue: false,
+    coerce: toBoolean,
+    getValue: settings => settings.hostPageSetting('openSidebar'),
   },
   query: {
+    allowInBrowserExt: true,
+    defaultValue: null,
     getValue: settings => settings.query,
   },
   requestConfigFromFrame: {
+    allowInBrowserExt: false,
+    defaultValue: null,
     getValue: settings => settings.hostPageSetting('requestConfigFromFrame'),
   },
   services: {
+    allowInBrowserExt: false,
+    defaultValue: null,
     getValue: settings => settings.hostPageSetting('services'),
   },
   showHighlights: {
+    allowInBrowserExt: false,
+    defaultValue: null,
     getValue: settings => settings.showHighlights,
   },
   notebookAppUrl: {
+    allowInBrowserExt: true,
+    defaultValue: null,
     getValue: settings => settings.notebookAppUrl,
   },
   sidebarAppUrl: {
+    allowInBrowserExt: true,
+    defaultValue: null,
     getValue: settings => settings.sidebarAppUrl,
   },
   // Sub-frame identifier given when a frame is being embedded into
   // by a top level client
   subFrameIdentifier: {
-    getValue: settings =>
-      settings.hostPageSetting('subFrameIdentifier', {
     allowInBrowserExt: true,
-      }),
+    defaultValue: null,
+    getValue: settings => settings.hostPageSetting('subFrameIdentifier'),
   },
   externalContainerSelector: {
+    allowInBrowserExt: false,
+    defaultValue: null,
     getValue: settings => settings.hostPageSetting('externalContainerSelector'),
   },
 };
@@ -162,8 +198,33 @@ export function getConfig(appContext = 'annotator', window_ = window) {
   let filteredKeys = configurationKeys(appContext);
   filteredKeys.forEach(name => {
     const configDef = configDefinitions[name];
+    const hasDefault = configDef.defaultValue !== undefined; // A default could be null
+    const isURLFromBrowserExtension = isBrowserExtension(
+      urlFromLinkTag(window_, 'sidebar', 'html')
+    );
+
+    // Only allow certain values in the browser extension context
+    if (!configDef.allowInBrowserExt && isURLFromBrowserExtension) {
+      // If the value is not allowed here, then set to the default if provided, otherwise ignore
+      // the key:value pair
+      if (hasDefault) {
+        config[name] = configDef.defaultValue;
+      }
+      return;
+    }
+
+    // Get the value from the configuration source
     const value = configDef.getValue(settings);
-    // Get the value from the configuration source and run through an optional coerce method
+    if (value === undefined) {
+      // If there is no value (e.g. undefined), then set to the default if provided,
+      // otherwise ignore the config key:value pair
+      if (hasDefault) {
+        config[name] = configDef.defaultValue;
+      }
+      return;
+    }
+
+    // Finally, run the value through an optional coerce method
     config[name] = configDef.coerce ? configDef.coerce(value) : value;
   });
 

src/annotator/config/is-browser-extension.js

 /**
  * Return true if the client is from a browser extension.
  *
- * @returns {boolean} true if this instance of the Hypothesis client is one
+ * @param {string} url
+ * @returns {boolean} - Returns true if this instance of the Hypothesis client is one
  *   distributed in a browser extension, false if it's one embedded in a
  *   website.
  *
  */
-export default function isBrowserExtension(app) {
-  return !(app.startsWith('http://') || app.startsWith('https://'));
+export function isBrowserExtension(url) {
+  return !(url.startsWith('http://') || url.startsWith('https://'));
 }

src/annotator/config/settings.js

@@ -2,7 +2,7 @@ import { parseJsonConfig } from '../../boot/parse-json-config';
 import { toBoolean } from '../../shared/type-coercions';
 
 import configFuncSettingsFrom from './config-func-settings-from';
-import isBrowserExtension from './is-browser-extension';
+import { isBrowserExtension } from './is-browser-extension';
 import { urlFromLinkTag } from './url-from-link-tag';
 
 /**
@@ -81,6 +81,7 @@ export default function settingsFrom(window_) {
     return jsonConfigs.group || groupFromURL();
   }
 
+  // TODO: Move this to a coerce method
   function showHighlights() {
     let showHighlights_ = hostPageSetting('showHighlights');
 
@@ -132,6 +133,7 @@ export default function settingsFrom(window_) {
     const allowInBrowserExt = options.allowInBrowserExt || false;
     const hasDefaultValue = typeof options.defaultValue !== 'undefined';
 
+    // TODO: Remove this logic
     if (
       !allowInBrowserExt &&
       isBrowserExtension(urlFromLinkTag(window_, 'sidebar', 'html'))

src/annotator/config/test/index-test.js

@@ -3,14 +3,30 @@ import { $imports } from '../index';
 
 describe('annotator/config/index', function () {
   let fakeSettingsFrom;
+  let fakeIsBrowserExtension;
 
   beforeEach(() => {
     fakeSettingsFrom = sinon.stub().returns({
-      hostPageSetting: sinon.stub(),
+      hostPageSetting: sinon.stub().returns('fakeValue'),
+      // getters
+      annotations: 'fakeValue',
+      clientUrl: 'fakeValue',
+      group: 'fakeValue',
+      notebookAppUrl: 'fakeValue',
+      showHighlights: 'fakeValue',
+      sidebarAppUrl: 'fakeValue',
+      query: 'fakeValue',
     });
+    fakeIsBrowserExtension = sinon.stub();
 
     $imports.$mock({
       './settings': fakeSettingsFrom,
+      './is-browser-extension': {
+        isBrowserExtension: fakeIsBrowserExtension,
+      },
+      './url-from-link-tag': {
+        urlFromLinkTag: sinon.stub(),
+      },
     });
   });
 
@@ -51,24 +67,117 @@ describe('annotator/config/index', function () {
     });
   });
 
-  ['appType', 'openSidebar', 'subFrameIdentifier'].forEach(function (
-    settingName
-  ) {
-    it(`reads ${settingName} from the host page, even when in a browser extension`, function () {
-      getConfig('all', 'WINDOW');
-      assert.calledWith(
-        fakeSettingsFrom().hostPageSetting,
-        settingName,
-        sinon.match({ allowInBrowserExt: true })
+  describe('browser extension', () => {
+    context('when client is loaded from the browser extension', function () {
+      it('returns only config values where `allowInBrowserExt` is true or the defaultValue if provided', () => {
+        fakeIsBrowserExtension.returns(true);
+        const config = getConfig('all', 'WINDOW');
+        assert.deepEqual(
+          {
+            appType: 'fakeValue',
+            annotations: 'fakeValue',
+            branding: null,
+            clientUrl: 'fakeValue',
+            enableExperimentalNewNoteButton: null,
+            externalContainerSelector: null,
+            focus: null,
+            group: 'fakeValue',
+            notebookAppUrl: 'fakeValue',
+            onLayoutChange: null,
+            openSidebar: true, // coerced
+            query: 'fakeValue',
+            requestConfigFromFrame: null,
+            services: null,
+            showHighlights: null,
+            sidebarAppUrl: 'fakeValue',
+            subFrameIdentifier: 'fakeValue',
+            theme: null,
+            usernameUrl: null,
+          },
+          config
         );
       });
     });
 
-  ['branding', 'services'].forEach(function (settingName) {
-    it(`reads ${settingName} from the host page only when in an embedded client`, function () {
-      getConfig('all', 'WINDOW');
+    context('when client is not loaded from browser extension', function () {
+      it('returns all config values', () => {
+        fakeIsBrowserExtension.returns(false);
+        const config = getConfig('all', 'WINDOW');
+        assert.deepEqual(
+          {
+            appType: 'fakeValue',
+            annotations: 'fakeValue',
+            branding: 'fakeValue',
+            clientUrl: 'fakeValue',
+            enableExperimentalNewNoteButton: 'fakeValue',
+            externalContainerSelector: 'fakeValue',
+            focus: 'fakeValue',
+            group: 'fakeValue',
+            notebookAppUrl: 'fakeValue',
+            onLayoutChange: 'fakeValue',
+            openSidebar: true, // coerced
+            query: 'fakeValue',
+            requestConfigFromFrame: 'fakeValue',
+            services: 'fakeValue',
+            showHighlights: 'fakeValue',
+            sidebarAppUrl: 'fakeValue',
+            subFrameIdentifier: 'fakeValue',
+            theme: 'fakeValue',
+            usernameUrl: 'fakeValue',
+          },
+          config
+        );
+      });
+    });
+  });
 
-      assert.calledWithExactly(fakeSettingsFrom().hostPageSetting, settingName);
+  describe('default values', () => {
+    beforeEach(() => {
+      // Remove all fake values
+      $imports.$mock({
+        './settings': sinon.stub().returns({
+          hostPageSetting: sinon.stub().returns(undefined),
+          annotations: undefined,
+          clientUrl: undefined,
+          group: undefined,
+          notebookAppUrl: undefined,
+          showHighlights: undefined,
+          sidebarAppUrl: undefined,
+          query: undefined,
+        }),
+      });
+    });
+
+    afterEach(() => {
+      $imports.$restore({
+        './settings': true,
+      });
+    });
+
+    it('sets corresponding default values if settings are undefined', () => {
+      const config = getConfig('all', 'WINDOW');
+
+      assert.deepEqual(config, {
+        appType: null,
+        annotations: null,
+        branding: null,
+        clientUrl: null,
+        enableExperimentalNewNoteButton: null,
+        externalContainerSelector: null,
+        focus: null,
+        group: null,
+        notebookAppUrl: null,
+        onLayoutChange: null,
+        openSidebar: false,
+        query: null,
+        requestConfigFromFrame: null,
+        services: null,
+        showHighlights: null,
+        sidebarAppUrl: null,
+        subFrameIdentifier: null,
+        theme: null,
+        usernameUrl: null,
+      });
     });
   });
 

src/annotator/config/test/is-browser-extension-test.js

-import isBrowserExtension from '../is-browser-extension';
+import { isBrowserExtension } from '../is-browser-extension';
 
 describe('annotator.config.isBrowserExtension', function () {
   [

src/annotator/config/test/settings-test.js

@@ -15,7 +15,9 @@ describe('annotator/config/settingsFrom', () => {
 
     $imports.$mock({
       './config-func-settings-from': fakeConfigFuncSettingsFrom,
-      './is-browser-extension': fakeIsBrowserExtension,
+      './is-browser-extension': {
+        isBrowserExtension: fakeIsBrowserExtension,
+      },
       './url-from-link-tag': {
         urlFromLinkTag: fakeUrlFromLinkTag,
       },