Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enforce the use of enums for Bridge events #3833

Closed
wants to merge 3 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/annotator/annotation-counts.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import events from '../shared/bridge-events';
import { sidebarToHostEvents } from '../shared/bridge-events';

const ANNOTATION_COUNT_ATTR = 'data-hypothesis-annotation-count';

@@ -11,8 +11,12 @@ const ANNOTATION_COUNT_ATTR = 'data-hypothesis-annotation-count';
* display annotation count.
* @param {import('../shared/bridge').Bridge} bridge - Channel for host-sidebar communication
*/

export function annotationCounts(rootEl, bridge) {
bridge.on(events.PUBLIC_ANNOTATION_COUNT_CHANGED, updateAnnotationCountElems);
bridge.on(
sidebarToHostEvents.PUBLIC_ANNOTATION_COUNT_CHANGED,
updateAnnotationCountElems
);

function updateAnnotationCountElems(newCount) {
const elems = rootEl.querySelectorAll('[' + ANNOTATION_COUNT_ATTR + ']');
31 changes: 21 additions & 10 deletions src/annotator/annotation-sync.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
import {
guestToSidebarEvents,
sidebarToGuestEvents,
} from '../shared/bridge-events';

/**
* @typedef {import('../shared/bridge').Bridge} Bridge
* @typedef {import('../types/annotator').AnnotationData} AnnotationData
@@ -43,7 +48,7 @@ export class AnnotationSync {
this.destroyed = false;

// Relay events from the sidebar to the rest of the annotator.
this.bridge.on('deleteAnnotation', (body, callback) => {
this.bridge.on(sidebarToGuestEvents.DELETE_ANNOTATION, (body, callback) => {
if (this.destroyed) {
callback(null);
return;
@@ -55,22 +60,28 @@ export class AnnotationSync {
callback(null);
});

this.bridge.on('loadAnnotations', (bodies, callback) => {
if (this.destroyed) {
this.bridge.on(
sidebarToGuestEvents.LOAD_ANNOTATIONS,
(bodies, callback) => {
if (this.destroyed) {
callback(null);
return;
}
const annotations = bodies.map(body => this._parse(body));
this._emitter.publish('annotationsLoaded', annotations);
callback(null);
return;
}
const annotations = bodies.map(body => this._parse(body));
this._emitter.publish('annotationsLoaded', annotations);
callback(null);
});
);

// Relay events from annotator to sidebar.
this._emitter.subscribe('beforeAnnotationCreated', annotation => {
if (annotation.$tag) {
return;
}
this.bridge.call('beforeCreateAnnotation', this._format(annotation));
this.bridge.call(
guestToSidebarEvents.BEFORE_CREATE_ANNOTATION,
this._format(annotation)
);
});
}

@@ -88,7 +99,7 @@ export class AnnotationSync {
}

this.bridge.call(
'sync',
guestToSidebarEvents.SYNC,
annotations.map(ann => this._format(ann))
);
}
4 changes: 2 additions & 2 deletions src/annotator/features.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import events from '../shared/bridge-events';
import { sidebarToHostEvents } from '../shared/bridge-events';
import warnOnce from '../shared/warn-once';

let _features = {};
@@ -12,7 +12,7 @@ export const features = {
* @param {import('../shared/bridge').Bridge} bridge - Channel for host-sidebar communication
*/
init: function (bridge) {
bridge.on(events.FEATURE_FLAGS_UPDATED, _set);
bridge.on(sidebarToHostEvents.FEATURE_FLAGS_UPDATED, _set);
},

reset: function () {
31 changes: 19 additions & 12 deletions src/annotator/guest.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
import {
guestToSidebarEvents,
sidebarToGuestEvents,
} from '../shared/bridge-events';
import { Bridge } from '../shared/bridge';
import { ListenerCollection } from '../shared/listener-collection';

@@ -221,7 +225,7 @@ export default class Guest {
// Don't hide the sidebar if the event comes from an element that contains a highlight
return;
}
this._bridge.call('closeSidebar');
this._bridge.call(guestToSidebarEvents.CLOSE_SIDEBAR);
};

this._listeners.add(this.element, 'mouseup', event => {
@@ -310,7 +314,7 @@ export default class Guest {
_connectSidebarEvents() {
// Handlers for events sent when user hovers or clicks on an annotation card
// in the sidebar.
this._bridge.on('focusAnnotations', (tags = []) => {
this._bridge.on(sidebarToGuestEvents.FOCUS_ANNOTATIONS, (tags = []) => {
this._focusedAnnotations.clear();
tags.forEach(tag => this._focusedAnnotations.add(tag));

@@ -322,7 +326,7 @@ export default class Guest {
}
});

this._bridge.on('scrollToAnnotation', tag => {
this._bridge.on(sidebarToGuestEvents.SCROLL_TO_ANNOTATION, tag => {
const anchor = this.anchors.find(a => a.annotation.$tag === tag);
if (!anchor?.highlights) {
return;
@@ -348,16 +352,19 @@ export default class Guest {
});

// Handler for when sidebar requests metadata for the current document
this._bridge.on('getDocumentInfo', cb => {
this._bridge.on(sidebarToGuestEvents.GET_DOCUMENT_INFO, cb => {
this.getDocumentInfo()
.then(info => cb(null, info))
.catch(reason => cb(reason));
});

// Handler for controls on the sidebar
this._bridge.on('setVisibleHighlights', showHighlights => {
this.setVisibleHighlights(showHighlights);
});
this._bridge.on(
sidebarToGuestEvents.SET_VISIBLE_HIGHLIGHTS,
showHighlights => {
this.setVisibleHighlights(showHighlights);
}
);
}

destroy() {
@@ -578,7 +585,7 @@ export default class Guest {
this.anchor(annotation);

if (!annotation.$highlight) {
this._bridge.call('openSidebar');
this._bridge.call(guestToSidebarEvents.OPEN_SIDEBAR);
}

return annotation;
@@ -592,7 +599,7 @@ export default class Guest {
*/
_focusAnnotations(annotations) {
const tags = annotations.map(a => a.$tag);
this._bridge.call('focusAnnotations', tags);
this._bridge.call(guestToSidebarEvents.FOCUS_ANNOTATIONS, tags);
}

/**
@@ -643,11 +650,11 @@ export default class Guest {
selectAnnotations(annotations, toggle = false) {
const tags = annotations.map(a => a.$tag);
if (toggle) {
this._bridge.call('toggleAnnotationSelection', tags);
this._bridge.call(guestToSidebarEvents.TOGGLE_ANNOTATION_SELECTION, tags);
} else {
this._bridge.call('showAnnotations', tags);
this._bridge.call(guestToSidebarEvents.SHOW_ANNOTATIONS, tags);
}
this._bridge.call('openSidebar');
this._bridge.call(guestToSidebarEvents.OPEN_SIDEBAR);
}

/**
43 changes: 28 additions & 15 deletions src/annotator/sidebar.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import Hammer from 'hammerjs';

import { Bridge } from '../shared/bridge';
import events from '../shared/bridge-events';
import {
hostToSidebarEvents,
sidebarToHostEvents,
} from '../shared/bridge-events';
import { ListenerCollection } from '../shared/listener-collection';

import { annotationCounts } from './annotation-counts';
@@ -217,7 +220,10 @@ export default class Sidebar {
this._listeners.add(window, 'message', event => {
const { data } = /** @type {MessageEvent} */ (event);
if (data?.type === 'hypothesisGuestUnloaded') {
this._sidebarRPC.call('destroyFrame', data.frameIdentifier);
this._sidebarRPC.call(
hostToSidebarEvents.DESTROY_FRAME,
data.frameIdentifier
);
}
});
}
@@ -239,25 +245,29 @@ export default class Sidebar {
sidebarTrigger(document.body, () => this.open());
features.init(this._sidebarRPC);

this._sidebarRPC.on('openSidebar', () => this.open());
this._sidebarRPC.on('closeSidebar', () => this.close());
this._sidebarRPC.on(sidebarToHostEvents.OPEN_SIDEBAR, () => this.open());
this._sidebarRPC.on(sidebarToHostEvents.CLOSE_SIDEBAR, () => this.close());

// Sidebar listens to the `openNotebook` event coming from the sidebar's
// iframe and re-publishes it via the emitter to the Notebook
this._sidebarRPC.on('openNotebook', (/** @type {string} */ groupId) => {
this.hide();
this._emitter.publish('openNotebook', groupId);
});
this._sidebarRPC.on(
sidebarToHostEvents.OPEN_NOTEBOOK,
(/** @type {string} */ groupId) => {
this.hide();
this._emitter.publish('openNotebook', groupId);
}
);
this._emitter.subscribe('closeNotebook', () => {
this.show();
});

/** @type {Array<[import('../shared/bridge-events').SidebarToHostEvent, function]>} */
const eventHandlers = [
[events.LOGIN_REQUESTED, this.onLoginRequest],
[events.LOGOUT_REQUESTED, this.onLogoutRequest],
[events.SIGNUP_REQUESTED, this.onSignupRequest],
[events.PROFILE_REQUESTED, this.onProfileRequest],
[events.HELP_REQUESTED, this.onHelpRequest],
[sidebarToHostEvents.LOGIN_REQUESTED, this.onLoginRequest],
[sidebarToHostEvents.LOGOUT_REQUESTED, this.onLogoutRequest],
[sidebarToHostEvents.SIGNUP_REQUESTED, this.onSignupRequest],
[sidebarToHostEvents.PROFILE_REQUESTED, this.onProfileRequest],
[sidebarToHostEvents.HELP_REQUESTED, this.onHelpRequest],
];
eventHandlers.forEach(([event, handler]) => {
if (handler) {
@@ -439,7 +449,7 @@ export default class Sidebar {
}

open() {
this._sidebarRPC.call('sidebarOpened');
this._sidebarRPC.call(hostToSidebarEvents.SIDEBAR_OPENED);
this._emitter.publish('sidebarOpened');

if (this.iframeContainer) {
@@ -478,7 +488,10 @@ export default class Sidebar {
* @param {boolean} shouldShowHighlights
*/
setAllVisibleHighlights(shouldShowHighlights) {
this._sidebarRPC.call('setVisibleHighlights', shouldShowHighlights);
this._sidebarRPC.call(
hostToSidebarEvents.SET_VISIBLE_HIGHLIGHTS,
shouldShowHighlights
);
}

/**
3 changes: 2 additions & 1 deletion src/annotator/test/annotation-counts-test.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { sidebarToHostEvents } from '../../shared/bridge-events';
import { annotationCounts } from '../annotation-counts';

describe('annotationCounts', () => {
@@ -57,7 +58,7 @@ describe('annotationCounts', () => {
const newCount = 10;
annotationCounts(document.body, fakeCrossFrame);

emitEvent('publicAnnotationCountChanged', newCount);
emitEvent(sidebarToHostEvents.PUBLIC_ANNOTATION_COUNT_CHANGED, newCount);

assert.equal(countEl1.textContent, newCount);
assert.equal(countEl2.textContent, newCount);
39 changes: 24 additions & 15 deletions src/annotator/test/annotation-sync-test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { EventBus } from '../util/emitter';

import {
guestToSidebarEvents,
sidebarToGuestEvents,
} from '../../shared/bridge-events';
import { AnnotationSync } from '../annotation-sync';
import { EventBus } from '../util/emitter';

describe('AnnotationSync', () => {
let createAnnotationSync;
@@ -44,12 +47,12 @@ describe('AnnotationSync', () => {
emitter.subscribe('annotationDeleted', eventStub);
createAnnotationSync();

publish('deleteAnnotation', { msg: ann }, () => {});
publish(sidebarToGuestEvents.DELETE_ANNOTATION, { msg: ann }, () => {});

assert.calledWith(eventStub, ann);
});

it("calls the 'deleteAnnotation' event's callback function", done => {
it('calls the "deleteAnnotation" event\'s callback function', done => {
const ann = { id: 1, $tag: 'tag1' };
const callback = function (err, result) {
assert.isNull(err);
@@ -58,7 +61,7 @@ describe('AnnotationSync', () => {
};
createAnnotationSync();

publish('deleteAnnotation', { msg: ann }, callback);
publish(sidebarToGuestEvents.DELETE_ANNOTATION, { msg: ann }, callback);
});

it('deletes any existing annotation from its cache before publishing event to the annotator', done => {
@@ -70,15 +73,15 @@ describe('AnnotationSync', () => {
done();
});

publish('deleteAnnotation', { msg: ann }, () => {});
publish(sidebarToGuestEvents.DELETE_ANNOTATION, { msg: ann }, () => {});
});

it('deletes any existing annotation from its cache', () => {
const ann = { id: 1, $tag: 'tag1' };
const annSync = createAnnotationSync();
annSync.cache.tag1 = ann;

publish('deleteAnnotation', { msg: ann }, () => {});
publish(sidebarToGuestEvents.DELETE_ANNOTATION, { msg: ann }, () => {});

assert.isUndefined(annSync.cache.tag1);
});
@@ -100,7 +103,7 @@ describe('AnnotationSync', () => {
emitter.subscribe('annotationsLoaded', loadedStub);
createAnnotationSync();

publish('loadAnnotations', bodies, () => {});
publish(sidebarToGuestEvents.LOAD_ANNOTATIONS, bodies, () => {});

assert.calledWith(loadedStub, annotations);
});
@@ -118,10 +121,14 @@ describe('AnnotationSync', () => {
emitter.publish('beforeAnnotationCreated', ann);

assert.called(fakeBridge.call);
assert.calledWith(fakeBridge.call, 'beforeCreateAnnotation', {
msg: ann,
tag: ann.$tag,
});
assert.calledWith(
fakeBridge.call,
guestToSidebarEvents.BEFORE_CREATE_ANNOTATION,
{
msg: ann,
tag: ann.$tag,
}
);
});

it('assigns a non-empty tag to the annotation', () => {
@@ -162,7 +169,9 @@ describe('AnnotationSync', () => {

annotationSync.sync([ann]);

assert.calledWith(fakeBridge.call, 'sync', [{ msg: ann, tag: ann.$tag }]);
assert.calledWith(fakeBridge.call, guestToSidebarEvents.SYNC, [
{ msg: ann, tag: ann.$tag },
]);
});
});

@@ -173,8 +182,8 @@ describe('AnnotationSync', () => {
annotationSync.destroy();
const cb = sinon.stub();

publish('loadAnnotations', [ann], cb);
publish('deleteAnnotation', ann, cb);
publish(sidebarToGuestEvents.LOAD_ANNOTATIONS, [ann], cb);
publish(sidebarToGuestEvents.DELETE_ANNOTATION, ann, cb);

assert.calledTwice(cb);
assert.calledWith(cb.firstCall, null);
Loading