Make src/annotator/ and src/boot/ typecheck with `noImplicitAny`

 - Add missing types in src/annotator and src/boot.

 - Simplify the return types of some config parsing functions by
   coercing invalid values to `null` or some other default. This reduces
   complexity with the types elsewhere and also potentially avoids some
   confusing errors if invalid values are specified here.

 - Add type definitions for hammerjs, scroll-into-view and
   lodash.debounce. The definitions for hammerjs and scroll-into-view
   come from DefinitelyTyped. For lodash a minimal custom definition has
   been written because the @types/lodash.debounce package includes
   types for the whole of lodash.

   In order to use these types the `allowSyntheticDefaultImports` flag
   had to be enabled in the TS configs.

   A consequence of adding types for scroll-into-view is that some
   missing null checks in sidebar components were found.

package.json

@@ -21,6 +21,8 @@
     "@rollup/plugin-virtual": "^2.0.3",
     "@sentry/browser": "^6.0.2",
     "@sentry/cli": "^1.71.0",
+    "@types/hammerjs": "^2.0.41",
+    "@types/scroll-into-view": "^1.16.0",
     "approx-string-match": "^2.0.0",
     "autoprefixer": "^10.0.1",
     "aws-sdk": "^2.345.0",

src/annotator/anchoring/html.js

 import { RangeAnchor, TextPositionAnchor, TextQuoteAnchor } from './types';
 
 /**
+ * @typedef {import('../../types/api').RangeSelector} RangeSelector
  * @typedef {import('../../types/api').Selector} Selector
+ * @typedef {import('../../types/api').TextPositionSelector} TextPositionSelector
+ * @typedef {import('../../types/api').TextQuoteSelector} TextQuoteSelector
  */
 
 /**
@@ -26,9 +29,9 @@ async function querySelector(anchor, options = {}) {
  *   @param {number} [options.hint]
  */
 export function anchor(root, selectors, options = {}) {
-  let position = null;
-  let quote = null;
-  let range = null;
+  let position = /** @type {TextPositionSelector|null} */ (null);
+  let quote = /** @type {TextQuoteSelector|null} */ (null);
+  let range = /** @type {RangeSelector|null} */ (null);
 
   // Collect all the selectors
   for (let selector of selectors) {
@@ -64,22 +67,25 @@ export function anchor(root, selectors, options = {}) {
   let promise = Promise.reject('unable to anchor');
 
   if (range) {
+    const range_ = range;
     promise = promise.catch(() => {
-      let anchor = RangeAnchor.fromSelector(root, range);
+      let anchor = RangeAnchor.fromSelector(root, range_);
       return querySelector(anchor, options).then(maybeAssertQuote);
     });
   }
 
   if (position) {
+    const position_ = position;
     promise = promise.catch(() => {
-      let anchor = TextPositionAnchor.fromSelector(root, position);
+      let anchor = TextPositionAnchor.fromSelector(root, position_);
       return querySelector(anchor, options).then(maybeAssertQuote);
     });
   }
 
   if (quote) {
+    const quote_ = quote;
     promise = promise.catch(() => {
-      let anchor = TextQuoteAnchor.fromSelector(root, quote);
+      let anchor = TextQuoteAnchor.fromSelector(root, quote_);
       return querySelector(anchor, options);
     });
   }

src/annotator/annotation-counts.js

@@ -7,15 +7,16 @@ const ANNOTATION_COUNT_ATTR = 'data-hypothesis-annotation-count';
  *
  * @param {Element} rootEl - The DOM element which contains the elements that
  *   display annotation count.
- * @param {import('../shared/messaging').PortRPC} rpc - Channel for host-sidebar communication
+ * @param {import('../shared/messaging').PortRPC<'publicAnnotationCountChanged', string>} rpc - Channel for host-sidebar communication
  */
 export function annotationCounts(rootEl, rpc) {
   rpc.on('publicAnnotationCountChanged', updateAnnotationCountElems);
 
+  /** @param {number} newCount */
   function updateAnnotationCountElems(newCount) {
     const elems = rootEl.querySelectorAll('[' + ANNOTATION_COUNT_ATTR + ']');
     Array.from(elems).forEach(elem => {
-      elem.textContent = newCount;
+      elem.textContent = newCount.toString();
     });
   }
 }

src/annotator/config/app.js

@@ -3,10 +3,11 @@
  * be passed to the sidebar or notebook applications.
  *
  * @param {string} appURL - URL from which the application will be served
- * @param {Record<string, any>} config
- * @return {object}
+ * @param {Record<string, unknown>} config
+ * @return {Record<string, unknown>}
  */
 export function createAppConfig(appURL, config) {
+  /** @type {Record<string, unknown>} */
   const appConfig = {};
 
   for (let [key, value] of Object.entries(config)) {
@@ -47,7 +48,7 @@ export function createAppConfig(appURL, config) {
   // functions) and will be omitted when the config is JSON-stringified.
   // Add a JSON-stringifiable option for each of these so that the sidebar can
   // at least know whether the callback functions were provided or not.
-  if (appConfig.services?.length > 0) {
+  if (Array.isArray(appConfig.services) && appConfig.services?.length > 0) {
     const service = appConfig.services[0];
     if (service.onLoginRequest) {
       service.onLoginRequestProvided = true;

src/annotator/config/config-func-settings-from.js

 /**
  * @typedef HypothesisWindowProps
- * @prop {() => object} [hypothesisConfig] - Function that returns configuration
+ * @prop {() => Record<string, unknown>} [hypothesisConfig] - Function that returns configuration
  *   for the Hypothesis client
  */
 
@@ -19,8 +19,7 @@
  * If there's no window.hypothesisConfig() function then return {}.
  *
  * @param {Window & HypothesisWindowProps} window_ - The window to search for a hypothesisConfig() function
- * @return {object} - Any config settings returned by hypothesisConfig()
- *
+ * @return {Record<string, unknown>} - Any config settings returned by hypothesisConfig()
  */
 export function configFuncSettingsFrom(window_) {
   if (!window_.hasOwnProperty('hypothesisConfig')) {

src/annotator/config/index.js

@@ -193,7 +193,10 @@ const configDefinitions = {
  */
 export function getConfig(appContext = 'annotator', window_ = window) {
   const settings = settingsFrom(window_);
+
+  /** @type {Record<string, unknown>} */
   const config = {};
+
   // Filter the config based on the application context as some config values
   // may be inappropriate or erroneous for some applications.
   let filteredKeys = configurationKeys(appContext);

src/annotator/config/settings.js

@@ -13,10 +13,14 @@ import { urlFromLinkTag } from './url-from-link-tag';
  * @prop {string} clientUrl
  * @prop {string} sidebarAppUrl
  * @prop {string} notebookAppUrl
- 
- * @prop {(name: string, options?: object) => (string|null)} hostPageSetting
+ * @prop {(name: string) => unknown} hostPageSetting
  */
 
+/** @param {unknown} value */
+function checkIfString(value) {
+  return typeof value === 'string' ? value : null;
+}
+
 /**
  * @param {Window} window_
  * @return {SettingsGetters}
@@ -27,6 +31,7 @@ export function settingsFrom(window_) {
   // In addition, Via sets the `ignoreOtherConfiguration` option to prevent configuration merging.
   const configFuncSettings = configFuncSettingsFrom(window_);
 
+  /** @type {Record<string, unknown>} */
   let jsonConfigs;
   if (toBoolean(configFuncSettings.ignoreOtherConfiguration)) {
     jsonConfigs = {};
@@ -56,7 +61,7 @@ export function settingsFrom(window_) {
       return null;
     }
 
-    return jsonConfigs.annotations || annotationsFromURL();
+    return checkIfString(jsonConfigs.annotations) || annotationsFromURL();
   }
 
   /**
@@ -78,23 +83,24 @@ export function settingsFrom(window_) {
       return null;
     }
 
-    return jsonConfigs.group || groupFromURL();
+    return checkIfString(jsonConfigs.group) || groupFromURL();
   }
 
-  // TODO: Move this to a coerce method
   function showHighlights() {
-    let showHighlights_ = hostPageSetting('showHighlights');
-
-    if (showHighlights_ === undefined) {
-      showHighlights_ = 'always'; // The default value is 'always'.
+    const value = hostPageSetting('showHighlights');
+
+    switch (value) {
+      case 'always':
+      case 'never':
+      case 'whenSidebarOpen':
+        return value;
+      case true:
+        return 'always';
+      case false:
+        return 'never';
+      default:
+        return 'always';
     }
-
-    // Convert legacy keys/values to corresponding current configuration.
-    if (typeof showHighlights_ === 'boolean') {
-      return showHighlights_ ? 'always' : 'never';
-    }
-
-    return showHighlights_;
   }
 
   /**
@@ -126,7 +132,7 @@ export function settingsFrom(window_) {
       return null;
     }
 
-    return jsonConfigs.query || queryFromURL();
+    return checkIfString(jsonConfigs.query) || queryFromURL();
   }
 
   /**

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

@@ -289,40 +289,15 @@ describe('annotator/config/settingsFrom', () => {
         input: false,
         output: 'never',
       },
-      {
-        it: 'passes invalid string values through unmodified',
-        input: 'invalid',
-        output: 'invalid',
-      },
-      {
-        it: 'passes numbers through unmodified',
-        input: 42,
-        output: 42,
-      },
       {
         it: 'defaults to "always"',
         input: undefined,
         output: 'always',
       },
       {
-        it: 'passes null through unmodified',
-        input: null,
-        output: null,
-      },
-      {
-        it: 'passes arrays through unmodified',
-        input: [1, 2, 3],
-        output: [1, 2, 3],
-      },
-      {
-        it: 'passes objects through unmodified',
-        input: { foo: 'bar' },
-        output: { foo: 'bar' },
-      },
-      {
-        it: 'passes regular expressions through unmodified',
-        input: /regex/,
-        output: /regex/,
+        it: 'converts invalid values to "always"',
+        input: { someInvalidValue: true },
+        output: 'always',
       },
     ].forEach(test => {
       it(test.it, () => {

src/annotator/features.js

@@ -10,7 +10,7 @@ const _set = features => {
 
 export const features = {
   /**
-   * @param {import('../shared/messaging').PortRPC} rpc - Channel for host-sidebar communication
+   * @param {import('../shared/messaging').PortRPC<'featureFlagsUpdated', string>} rpc - Channel for host-sidebar communication
    */
   init: function (rpc) {
     rpc.on('featureFlagsUpdated', _set);

src/annotator/frame-observer.js

@@ -162,7 +162,9 @@ export function onNextDocumentReady(frame) {
  * @return {() => void} Callback that unsubscribes from future changes
  */
 export function onDocumentReady(frame, callback, { pollInterval = 10 } = {}) {
+  /** @type {number|undefined} */
   let pollTimer;
+  /** @type {() => void} */
   let pollForDocumentChange;
 
   // Visited documents for which we have fired the callback or are waiting
@@ -171,7 +173,7 @@ export function onDocumentReady(frame, callback, { pollInterval = 10 } = {}) {
 
   const cancelPoll = () => {
     clearTimeout(pollTimer);
-    pollTimer = null;
+    pollTimer = undefined;
   };
 
   // Begin polling for a document change when the current document is about

src/annotator/index.js

@@ -61,7 +61,9 @@ function init() {
 
     const sidebarConfig = getConfig('sidebar');
 
-    const hypothesisAppsOrigin = new URL(sidebarConfig.sidebarAppUrl).origin;
+    const hypothesisAppsOrigin = new URL(
+      /** @type {string} */ (sidebarConfig.sidebarAppUrl)
+    ).origin;
     const portProvider = new PortProvider(hypothesisAppsOrigin);
 
     const eventBus = new EventBus();

src/annotator/sidebar-trigger.js

@@ -13,11 +13,9 @@ export function sidebarTrigger(rootEl, showFn) {
   );
 
   Array.from(triggerElems).forEach(triggerElem => {
-    triggerElem.addEventListener('click', handleCommand);
+    triggerElem.addEventListener('click', e => {
+      showFn();
+      e.stopPropagation();
+    });
   });
-
-  function handleCommand(event) {
-    showFn();
-    event.stopPropagation();
-  }
 }

src/annotator/sidebar.js

-import Hammer from 'hammerjs';
+import * as Hammer from 'hammerjs';
 
 import { addConfigFragment } from '../shared/config-fragment';
 import { sendErrorsTo } from '../shared/frame-error-capture';
@@ -30,12 +30,14 @@ export const MIN_RESIZE = 280;
 /**
  * Create the iframe that will load the sidebar application.
  *
+ * @param {Record<string, unknown>} config
  * @return {HTMLIFrameElement}
  */
 function createSidebarIframe(config) {
+  const sidebarURL = /** @type {string} */ (config.sidebarAppUrl);
   const sidebarAppSrc = addConfigFragment(
-    config.sidebarAppUrl,
-    createAppConfig(config.sidebarAppUrl, config)
+    sidebarURL,
+    createAppConfig(sidebarURL, config)
   );
 
   const sidebarFrame = document.createElement('iframe');
@@ -70,7 +72,7 @@ export class Sidebar {
      * Tracks which `Guest` has a text selection. `null` indicates to default
      * to the first connected guest frame.
      *
-     * @type {PortRPC|null}
+     * @type {PortRPC<GuestToHostEvent, HostToGuestEvent>|null}
      */
     this._guestWithSelection = null;
 
@@ -365,7 +367,8 @@ export class Sidebar {
   _setupGestures() {
     const toggleButton = this.toolbar.sidebarToggleButton;
     if (toggleButton) {
-      this._hammerManager = new Hammer.Manager(toggleButton).on(
+      this._hammerManager = new Hammer.Manager(toggleButton);
+      this._hammerManager.on(
         'panstart panend panleft panright',
         /* istanbul ignore next */
         event => this._onPan(event)
@@ -482,6 +485,7 @@ export class Sidebar {
     }
   }
 
+  /** @param {HammerInput} event */
   _onPan(event) {
     const frame = this.iframeContainer;
     if (!frame) {

src/boot/boot.js

 import { requiredPolyfillSets } from './polyfills';
 
+/** @typedef {import('./polyfills').PolyfillSet} PolyfillSet */
+
 /**
  * Polyfills used by both the annotator and sidebar app.
  */
-const commonPolyfills = [
+const commonPolyfills = /** @type {PolyfillSet[]} */ ([
   // ES APIs
   'es2017',
   'es2018',
 
   // Any other polyfills which may rely on certain ES APIs should be listed here.
-];
+]);
 
 /**
  * @typedef SidebarAppConfig
@@ -156,6 +158,7 @@ function injectAssets(doc, config, assets, { forceModuleReload } = {}) {
   });
 }
 
+/** @param {PolyfillSet[]} needed */
 function polyfillBundles(needed) {
   return requiredPolyfillSets(needed).map(
     set => `scripts/polyfills-${set}.bundle.js`

src/boot/index.js

@@ -14,20 +14,33 @@ import { isBrowserSupported } from './browser-check';
 // @ts-ignore - This file is generated before the boot bundle is built.
 import manifest from '../../build/manifest.json';
 
+/**
+ * @typedef {import('./boot').AnnotatorConfig} AnnotatorConfig
+ * @typedef {import('./boot').SidebarAppConfig} SidebarAppConfig
+ */
+
 if (isBrowserSupported()) {
-  const settings = parseJsonConfig(document);
-  const assetRoot = processUrlTemplate(settings.assetRoot || '__ASSET_ROOT__');
+  const config = /** @type {AnnotatorConfig|SidebarAppConfig} */ (
+    parseJsonConfig(document)
+  );
+  const assetRoot = processUrlTemplate(config.assetRoot || '__ASSET_ROOT__');
 
   // Check whether this is the sidebar app (indicated by the presence of a
   // `<hypothesis-app>` element) and load the appropriate part of the client.
   if (document.querySelector('hypothesis-app')) {
-    bootSidebarApp(document, { assetRoot, manifest, apiUrl: settings.apiUrl });
+    const sidebarConfig = /** @type {SidebarAppConfig} */ (config);
+    bootSidebarApp(document, {
+      assetRoot,
+      manifest,
+      apiUrl: sidebarConfig.apiUrl,
+    });
   } else {
+    const annotatorConfig = /** @type {AnnotatorConfig} */ (config);
     const notebookAppUrl = processUrlTemplate(
-      settings.notebookAppUrl || '__NOTEBOOK_APP_URL__'
+      annotatorConfig.notebookAppUrl || '__NOTEBOOK_APP_URL__'
     );
     const sidebarAppUrl = processUrlTemplate(
-      settings.sidebarAppUrl || '__SIDEBAR_APP_URL__'
+      annotatorConfig.sidebarAppUrl || '__SIDEBAR_APP_URL__'
     );
     bootHypothesisClient(document, {
       assetRoot,

src/boot/parse-json-config.js

-// `Object.assign()`-like helper. Used because this script needs to work
-// in IE 10/11 without polyfills.
+/**
+ * `Object.assign()`-like helper. Used because this script needs to work
+ * in IE 10/11 without polyfills.
+ *
+ * @param {Record<string, unknown>} dest
+ * @param {Record<string, unknown>} src
+ */
 function assign(dest, src) {
   for (const k in src) {
     if (src.hasOwnProperty(k)) {
@@ -26,6 +31,7 @@ function assign(dest, src) {
  * @param {Document|Element} document - The root element to search.
  */
 export function parseJsonConfig(document) {
+  /** @type {Record<string, unknown>} */
   const config = {};
   const settingsElements = document.querySelectorAll(
     'script.js-hypothesis-config'

src/boot/polyfills/index.js

@@ -9,6 +9,9 @@
 
 /**
  * Return true if `obj` has all of the methods in `methods`.
+ *
+ * @param {any} obj
+ * @param {string[]} methods
  */
 function hasMethods(obj, ...methods) {
   return methods.every(method => typeof obj[method] === 'function');
@@ -33,9 +36,17 @@ const needsPolyfill = {
   },
 };
 
+/**
+ * Name of a polyfill set that can be loaded.
+ *
+ * @typedef {keyof needsPolyfill} PolyfillSet
+ */
+
 /**
  * Return the subset of polyfill sets from `needed`  which are needed by the
  * current browser.
+ *
+ * @param {PolyfillSet[]} needed
  */
 export function requiredPolyfillSets(needed) {
   return needed.filter(set => {

src/boot/url-template.js

@@ -4,6 +4,8 @@
  * We don't use the URL constructor here because IE and early versions of Edge
  * do not support it and this code runs early in the life of the app before any
  * polyfills can be loaded.
+ *
+ * @param {string} url
  */
 function extractOrigin(url) {
   const match = url.match(/(https?):\/\/([^:/]+)/);

src/sidebar/components/NotebookView.js

@@ -116,7 +116,9 @@ function NotebookView({ loadAnnotationsService, streamer }) {
   useLayoutEffect(() => {
     // TODO: Transition and effects here should be improved
     if (paginationPage !== lastPaginationPage.current) {
-      scrollIntoView(threadListScrollTop.current);
+      if (threadListScrollTop.current) {
+        scrollIntoView(threadListScrollTop.current);
+      }
       lastPaginationPage.current = paginationPage;
     }
   }, [paginationPage]);

src/sidebar/components/SidebarPanel.js

@@ -45,7 +45,7 @@ export default function SidebarPanel({
   useEffect(() => {
     if (panelWasActive.current !== panelIsActive) {
       panelWasActive.current = panelIsActive;
-      if (panelIsActive) {
+      if (panelIsActive && panelElement.current) {
         scrollIntoView(panelElement.current);
       }
       if (typeof onActiveChanged === 'function') {

src/sidebar/helpers/groups.js

@@ -4,7 +4,6 @@
  * @typedef {import('../../types/api').GroupIdentifier} GroupIdentifier
  */
 
-// @ts-expect-error - Ignore error about default-importing a CommonJS module.
 import escapeStringRegexp from 'escape-string-regexp';
 
 import { serviceConfig } from '../config/service-config';

src/sidebar/services/tags.js

-// @ts-expect-error - Ignore error about default-importing a CommonJS module.
 import escapeStringRegexp from 'escape-string-regexp';
 
 /**

src/tsconfig.json

 {
   "compilerOptions": {
     "allowJs": true,
+
+    // Needed for some npm packages
+    "allowSyntheticDefaultImports": true,
+
     "checkJs": true,
     "lib": ["es2020", "dom", "dom.iterable"],
     "jsx": "react-jsx",

src/tsconfig.no-any.json

 {
   "compilerOptions": {
     "allowJs": true,
+
+    // Needed for some npm packages
     "allowSyntheticDefaultImports": true,
+
     "checkJs": true,
     "lib": ["es2020", "dom", "dom.iterable"],
     "jsx": "react-jsx",
@@ -20,7 +23,12 @@
     // code for the browser.
     "types": []
   },
-  "include": ["shared/**/*.js", "types/process.d.ts"],
+  "include": [
+    "annotator/**/*.js",
+    "boot/**/*.js",
+    "shared/**/*.js",
+    "types/*.d.ts"
+  ],
   "exclude": [
     // Tests are not checked.
     "**/test/**/*.js",

src/types/lodash.debounce.d.ts

+// Self-contained types for lodash.debounce that include only the functionality
+// we use and avoids adding types for the whole of lodash to our dependencies.
+declare module 'lodash.debounce' {
+  interface DebouncedFunction {
+    (): void;
+    cancel(): void;
+    flush(): void;
+  }
+
+  interface DebounceOptions {
+    maxWait?: number;
+  }
+
+  export default function debounce(
+    callback: () => void,
+    options?: DebounceOptions
+  ): DebouncedFunction;
+
+  export default function debounce(
+    callback: () => void,
+    delay: number
+  ): DebouncedFunction;
+}

yarn.lock

@@ -1428,6 +1428,11 @@
   resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.39.tgz#e177e699ee1b8c22d23174caaa7422644389509f"
   integrity sha512-EYNwp3bU+98cpU4lAWYYL7Zz+2gryWH1qbdDTidVd6hkiR6weksdbMadyXKXNPEkQFhXM+hVO9ZygomHXp+AIw==
 
+"@types/hammerjs@^2.0.41":
+  version "2.0.41"
+  resolved "https://registry.yarnpkg.com/@types/hammerjs/-/hammerjs-2.0.41.tgz#f6ecf57d1b12d2befcce00e928a6a097c22980aa"
+  integrity sha512-ewXv/ceBaJprikMcxCmWU1FKyMAQ2X7a9Gtmzw8fcg2kIePI1crERDM818W+XYrxqdBBOdlf2rm137bU+BltCA==
+
 "@types/node@*", "@types/node@>= 8":
   version "14.14.43"
   resolved "https://registry.yarnpkg.com/@types/node/-/node-14.14.43.tgz#26bcbb0595b305400e8ceaf9a127a7f905ae49c8"
@@ -1450,6 +1455,11 @@
   dependencies:
     "@types/node" "*"
 
+"@types/scroll-into-view@^1.16.0":
+  version "1.16.0"
+  resolved "https://registry.yarnpkg.com/@types/scroll-into-view/-/scroll-into-view-1.16.0.tgz#bd02094307624cc0243c34dca478510a0a8515e8"
+  integrity sha512-WT0YBP7CLi3XH/gDbWdtBf4mQVyE7zQrpEZ2rNrxz9tVoIPJf97zGlfRqnkECj7P8rPkFxVlo1KbOOMetcchdA==
+
 "@types/yauzl@^2.9.1":
   version "2.9.1"
   resolved "https://registry.yarnpkg.com/@types/yauzl/-/yauzl-2.9.1.tgz#d10f69f9f522eef3cf98e30afb684a1e1ec923af"