Replace the "plugin" functionality in the Guest

Replace the "plugin" functionality in the Guest that indirectly
instantiates the `DocumentMeta`, `PDF` and `CrossFrame` classes with
direct instantiation of those classes.

The annotator part of the Hypothesis client is no longer a pluggable
toolbox which can be arbitrarily extended by user-provided plugins.
Removing the "plugins" logic and replacing it with direct construction
of the necessary classes removes a lot of indirection that made the code
harder to follow and also makes it easier for TypeScript to statically
check the code. It will also enable the `config` object that is passed
to `Guest` to be paired down to just the fields that are actually needed
and properly typed.

In future we will need some kind of plugin/integration facility to
support different document viewers. For we will build a more specific
document type integration facility when the time comes.

 - Remove `this.plugins` field from `Guest` class and `pluginClasses`
   config and replace with direct construction of `PDFIntegration`,
   `DocumentMeta` and `CrossFrame` classes

 - Update the `AnchoringImpl` typedef to correctly reflect that some anchoring
   implementations (HTML) have a synchronous `describe` method

 - Remove unnecessary `config` argument to `PDF` constructor

 - Replace the `PDF: {}` content in the guest config with `documentType:
   'pdf'` as a more obvious way for the annotator entry point to
   configure which document viewer integration is loaded. In future we
   can generalize this to support different types of viewer integration.

src/annotator/config/sidebar.js

@@ -33,9 +33,7 @@ export function createSidebarConfig(config) {
   // nb. We don't currently strip all the annotator-only properties here.
   // That's OK because validation / filtering happens in the sidebar app itself.
   // It just results in unnecessary content in the sidebar iframe's URL string.
-  ['notebookAppUrl', 'sidebarAppUrl', 'pluginClasses'].forEach(
-    key => delete sidebarConfig[key]
-  );
+  ['notebookAppUrl', 'sidebarAppUrl'].forEach(key => delete sidebarConfig[key]);
 
   return sidebarConfig;
 }

src/annotator/guest.js

@@ -2,6 +2,9 @@ import scrollIntoView from 'scroll-into-view';
 
 import Delegator from './delegator';
 import { Adder } from './adder';
+import CrossFrame from './plugin/cross-frame';
+import DocumentMeta from './plugin/document';
+import PDFIntegration from './plugin/pdf';
 
 import * as htmlAnchoring from './anchoring/html';
 import { TextRange } from './anchoring/text-range';
@@ -106,16 +109,11 @@ export default class Guest extends Delegator {
    * @param {HTMLElement} element -
    *   The root element in which the `Guest` instance should be able to anchor
    *   or create annotations. In an ordinary web page this typically `document.body`.
-   * @param {Record<string, any>} config
-   * @param {typeof htmlAnchoring} anchoring - Anchoring implementation
+   * @param {Record<string, any>} [config]
+   * @param {typeof htmlAnchoring} [anchoring] - Anchoring implementation
    */
-  constructor(element, config, anchoring = htmlAnchoring) {
-    const defaultConfig = {
-      // This causes the `Document` plugin to be initialized.
-      Document: {},
-    };
-
-    super(element, { ...defaultConfig, ...config });
+  constructor(element, config = {}, anchoring = htmlAnchoring) {
+    super(element, config);
 
     this.visibleHighlights = false;
 
@@ -146,7 +144,14 @@ export default class Guest extends Delegator {
       }
     });
 
-    this.plugins = {};
+    // Set the initial anchoring module. Note that for PDFs this is replaced by
+    // `PDFIntegration` below.
+    this.anchoring = anchoring;
+
+    this.documentMeta = new DocumentMeta(this.element);
+    if (config.documentType === 'pdf') {
+      this.pdfIntegration = new PDFIntegration(this.element, this);
+    }
 
     /** @type {Anchor[]} */
     this.anchors = [];
@@ -155,8 +160,6 @@ export default class Guest extends Delegator {
     // The "top" guest instance will have this as null since it's in a top frame not a sub frame
     this.frameIdentifier = config.subFrameIdentifier || null;
 
-    this.anchoring = anchoring;
-
     const cfOptions = {
       config,
       on: (event, handler) => {
@@ -167,8 +170,7 @@ export default class Guest extends Delegator {
       },
     };
 
-    this.addPlugin('CrossFrame', cfOptions);
-    this.crossframe = this.plugins.CrossFrame;
+    this.crossframe = new CrossFrame(this.element, cfOptions);
     this.crossframe.onConnect(() => this._setupInitialState(config));
 
     // Whether clicks on non-highlighted text should close the sidebar
@@ -176,14 +178,6 @@ export default class Guest extends Delegator {
     this._connectAnnotationSync();
     this._connectAnnotationUISync(this.crossframe);
 
-    // Load plugins
-    for (let name of Object.keys(this.options)) {
-      const opts = this.options[name];
-      if (!this.plugins[name] && this.options.pluginClasses[name]) {
-        this.addPlugin(name, opts);
-      }
-    }
-
     // Setup event handlers on the root element
     this._elementEventListeners = [];
     this._setupElementEvents();
@@ -247,26 +241,19 @@ export default class Guest extends Delegator {
     });
   }
 
-  addPlugin(name, options) {
-    const Klass = this.options.pluginClasses[name];
-    this.plugins[name] = new Klass(this.element, options, this);
-  }
-
   /**
    * Retrieve metadata for the current document.
    */
   getDocumentInfo() {
     let metadataPromise;
     let uriPromise;
-    if (this.plugins.PDF) {
-      metadataPromise = Promise.resolve(this.plugins.PDF.getMetadata());
-      uriPromise = Promise.resolve(this.plugins.PDF.uri());
-    } else if (this.plugins.Document) {
-      uriPromise = Promise.resolve(this.plugins.Document.uri());
-      metadataPromise = Promise.resolve(this.plugins.Document.metadata);
+
+    if (this.pdfIntegration) {
+      metadataPromise = this.pdfIntegration.getMetadata();
+      uriPromise = this.pdfIntegration.uri();
     } else {
-      uriPromise = Promise.reject();
-      metadataPromise = Promise.reject();
+      uriPromise = Promise.resolve(this.documentMeta.uri());
+      metadataPromise = Promise.resolve(this.documentMeta.metadata);
     }
 
     uriPromise = uriPromise.catch(() =>
@@ -355,9 +342,8 @@ export default class Guest extends Delegator {
 
     removeAllHighlights(this.element);
 
-    for (let name of Object.keys(this.plugins)) {
-      this.plugins[name].destroy();
-    }
+    this.documentMeta.destroy();
+    this.pdfIntegration?.destroy();
 
     super.destroy();
   }
@@ -478,8 +464,8 @@ export default class Guest extends Delegator {
       // Add the anchors for this annotation to instance storage.
       this._updateAnchors(this.anchors.concat(anchors));
 
-      // Let plugins know about the new information.
-      this.plugins.CrossFrame?.sync([annotation]);
+      // Let the sidebar know about the new annotation.
+      this.crossframe.sync([annotation]);
 
       return anchors;
     };

src/annotator/index.js

@@ -18,23 +18,11 @@ import iconSet from './icons';
 registerIcons(iconSet);
 
 import configFrom from './config/index';
-import CrossFramePlugin from './plugin/cross-frame';
-import DocumentPlugin from './plugin/document';
 import Guest from './guest';
 import Notebook from './notebook';
-import PDFPlugin from './plugin/pdf';
 import PdfSidebar from './pdf-sidebar';
 import Sidebar from './sidebar';
 
-const pluginClasses = {
-  // Document type plugins
-  PDF: PDFPlugin,
-  Document: DocumentPlugin,
-
-  // Cross-frame communication
-  CrossFrame: CrossFramePlugin,
-};
-
 const window_ = /** @type {HypothesisWindow} */ (window);
 
 // Look up the URL of the sidebar. This element is added to the page by the
@@ -60,12 +48,8 @@ function init() {
     window_.__hypothesis_frame = true;
   }
 
-  config.pluginClasses = pluginClasses;
-
   // Load the PDF anchoring/metadata integration.
-  if (isPDF) {
-    config.PDF = {};
-  }
+  config.documentType = isPDF ? 'pdf' : 'html';
 
   const guest = new Guest(document.body, config);
   const sidebar = SidebarClass

src/annotator/plugin/document.js

@@ -63,7 +63,7 @@ function createMetadata() {
  * populates the `document` property of new annotations.
  */
 export default class DocumentMeta extends Delegator {
-  constructor(element, options) {
+  constructor(element, options = {}) {
     super(element, options);
 
     this.metadata = createMetadata();

src/annotator/plugin/pdf.js

@@ -46,8 +46,8 @@ export default class PDF extends Delegator {
   /**
    * @param {Annotator} annotator
    */
-  constructor(element, config, annotator) {
-    super(element, config);
+  constructor(element, annotator) {
+    super(element);
 
     this.annotator = annotator;
     annotator.anchoring = pdfAnchoring;

src/annotator/plugin/test/pdf-test.js

@@ -23,7 +23,7 @@ describe('annotator/plugin/pdf', () => {
   let pdfPlugin;
 
   function createPDFPlugin() {
-    return new PDF(document.body, {}, fakeAnnotator);
+    return new PDF(document.body, fakeAnnotator);
   }
 
   beforeEach(() => {

src/annotator/test/guest-test.js

@@ -15,15 +15,6 @@ class FakeAdder {
 }
 FakeAdder.instance = null;
 
-class FakePlugin extends Delegator {
-  constructor(element, config, annotator) {
-    super(element, config);
-    this.annotator = annotator;
-    FakePlugin.instance = this;
-  }
-}
-FakePlugin.instance = null;
-
 class FakeTextRange {
   constructor(range) {
     this.range = range;
@@ -59,7 +50,7 @@ describe('Guest', () => {
 
   beforeEach(() => {
     FakeAdder.instance = null;
-    guestConfig = { pluginClasses: {} };
+    guestConfig = {};
     highlighter = {
       highlightRange: sinon.stub().returns([]),
       removeHighlights: sinon.stub(),
@@ -93,7 +84,6 @@ describe('Guest', () => {
     }
 
     CrossFrame = sandbox.stub().returns(fakeCrossFrame);
-    guestConfig.pluginClasses.CrossFrame = CrossFrame;
 
     $imports.$mock({
       './adder': { Adder: FakeAdder },
@@ -103,6 +93,7 @@ describe('Guest', () => {
       },
       './highlighter': highlighter,
       './range-util': rangeUtil,
+      './plugin/cross-frame': CrossFrame,
       './selection-observer': {
         SelectionObserver: FakeSelectionObserver,
       },
@@ -116,28 +107,6 @@ describe('Guest', () => {
     $imports.$restore();
   });
 
-  describe('plugins', () => {
-    let fakePlugin;
-    let guest;
-
-    beforeEach(() => {
-      FakePlugin.instance = null;
-      guestConfig.pluginClasses.FakePlugin = FakePlugin;
-      guest = createGuest({ FakePlugin: {} });
-      fakePlugin = FakePlugin.instance;
-    });
-
-    it('passes guest reference to constructor', () => {
-      assert.equal(fakePlugin.annotator, guest);
-    });
-
-    it('calls `destroy` method of plugins when guest is destroyed', () => {
-      sandbox.spy(fakePlugin, 'destroy');
-      guest.destroy();
-      assert.called(fakePlugin.destroy);
-    });
-  });
-
   describe('cross frame', () => {
     it('provides an event bus for the annotation sync module', () => {
       createGuest();
@@ -361,9 +330,9 @@ describe('Guest', () => {
       beforeEach(() => {
         document.title = 'hi';
         guest = createGuest();
-        guest.plugins.PDF = {
-          uri: sandbox.stub().returns(window.location.href),
-          getMetadata: sandbox.stub(),
+        guest.pdfIntegration = {
+          uri: sinon.stub().resolves(window.location.href),
+          getMetadata: sinon.stub().resolves({}),
         };
       });
 
@@ -384,7 +353,7 @@ describe('Guest', () => {
         };
 
         const promise = Promise.resolve(metadata);
-        guest.plugins.PDF.getMetadata.returns(promise);
+        guest.pdfIntegration.getMetadata.returns(promise);
 
         emitGuestEvent('getDocumentInfo', assertComplete);
       });
@@ -405,7 +374,7 @@ describe('Guest', () => {
         };
 
         const promise = Promise.reject(new Error('Not a PDF document'));
-        guest.plugins.PDF.getMetadata.returns(promise);
+        guest.pdfIntegration.getMetadata.returns(promise);
 
         emitGuestEvent('getDocumentInfo', assertComplete);
       });
@@ -450,7 +419,7 @@ describe('Guest', () => {
     it('hides sidebar when the user taps or clicks in the page', () => {
       for (let event of ['click', 'touchstart']) {
         rootElement.dispatchEvent(new Event(event));
-        assert.calledWith(guest.plugins.CrossFrame.call, 'closeSidebar');
+        assert.calledWith(guest.crossframe.call, 'closeSidebar');
       }
     });
 
@@ -458,7 +427,7 @@ describe('Guest', () => {
       for (let event of ['click', 'touchstart']) {
         guest.closeSidebarOnDocumentClick = false;
         rootElement.dispatchEvent(new Event(event));
-        assert.notCalled(guest.plugins.CrossFrame.call);
+        assert.notCalled(guest.crossframe.call);
       }
     });
 
@@ -469,7 +438,7 @@ describe('Guest', () => {
 
       for (let event of ['click', 'touchstart']) {
         fakeSidebarFrame.dispatchEvent(new Event(event, { bubbles: true }));
-        assert.notCalled(guest.plugins.CrossFrame.call);
+        assert.notCalled(guest.crossframe.call);
       }
     });
   });
@@ -574,17 +543,15 @@ describe('Guest', () => {
 
     beforeEach(() => {
       guest = createGuest();
-      guest.plugins.PDF = {
-        uri() {
-          return 'urn:x-pdf:001122';
-        },
-        getMetadata: sandbox.stub(),
+      guest.pdfIntegration = {
+        uri: sandbox.stub().resolves('urn:x-pdf:001122'),
+        getMetadata: sandbox.stub().resolves({}),
       };
     });
 
     it('preserves the components of the URI other than the fragment', () => {
-      guest.plugins.PDF = null;
-      guest.plugins.Document = {
+      guest.pdfIntegration = null;
+      guest.documentMeta = {
         uri() {
           return 'http://foobar.com/things?id=42';
         },
@@ -596,7 +563,7 @@ describe('Guest', () => {
     });
 
     it('removes the fragment identifier from URIs', () => {
-      guest.plugins.PDF.uri = () => 'urn:x-pdf:aabbcc#ignoreme';
+      guest.pdfIntegration.uri.resolves('urn:x-pdf:aabbcc#ignoreme');
       return guest
         .getDocumentInfo()
         .then(({ uri }) => assert.equal(uri, 'urn:x-pdf:aabbcc'));
@@ -605,13 +572,18 @@ describe('Guest', () => {
 
   describe('#createAnnotation', () => {
     it('adds metadata to the annotation object', () => {
+      class FakeDocumentMeta {
+        get metadata() {
+          return { title: 'hello' };
+        }
+
+        uri() {
+          return 'http://example.com';
+        }
+      }
+      $imports.$mock({ './plugin/document': FakeDocumentMeta });
+
       const guest = createGuest();
-      sinon.stub(guest, 'getDocumentInfo').returns(
-        Promise.resolve({
-          metadata: { title: 'hello' },
-          uri: 'http://example.com/',
-        })
-      );
       const annotation = {};
 
       guest.createAnnotation(annotation);
@@ -769,12 +741,12 @@ describe('Guest', () => {
         .then(() => assert.notCalled(htmlAnchoring.anchor));
     });
 
-    it('updates the cross frame plugin', () => {
+    it('syncs annotations to the sidebar', () => {
       const guest = createGuest();
-      guest.plugins.CrossFrame = { sync: sinon.stub() };
+      guest.crossframe = { sync: sinon.stub() };
       const annotation = {};
       return guest.anchor(annotation).then(() => {
-        assert.called(guest.plugins.CrossFrame.sync);
+        assert.called(guest.crossframe.sync);
       });
     });
 

src/annotator/test/integration/anchoring-test.js

@@ -2,6 +2,7 @@
 // with various combinations of selector are anchored.
 
 import Guest from '../../guest';
+import { $imports as guestImports } from '../../guest';
 
 function quoteSelector(quote) {
   return {
@@ -47,22 +48,27 @@ function FakeCrossFrame() {
 
 describe('anchoring', function () {
   let guest;
-  let guestConfig;
   let container;
 
-  before(function () {
-    guestConfig = { pluginClasses: { CrossFrame: FakeCrossFrame } };
+  before(() => {
+    guestImports.$mock({
+      './plugin/cross-frame': FakeCrossFrame,
+    });
+  });
+
+  after(() => {
+    guestImports.$restore({ './plugins/cross-frame': true });
   });
 
-  beforeEach(function () {
+  beforeEach(() => {
     sinon.stub(console, 'warn');
     container = document.createElement('div');
     container.innerHTML = require('./test-page.html');
     document.body.appendChild(container);
-    guest = new Guest(container, guestConfig);
+    guest = new Guest(container);
   });
 
-  afterEach(function () {
+  afterEach(() => {
     guest.destroy();
     container.parentNode.removeChild(container);
     console.warn.restore();

src/types/annotator.js

@@ -61,7 +61,7 @@
  *
  * @typedef AnchoringImpl
  * @prop {(root: HTMLElement, selectors: Selector[], options: any) => Promise<Range>} anchor
- * @prop {(root: HTMLElement, range: Range, options: any) => Promise<Selector[]>} describe
+ * @prop {(root: HTMLElement, range: Range, options: any) => Selector[]|Promise<Selector[]>} describe
  */
 
 /**