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.

Sign up for GitHub

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

Strengthen the types for Bridge events #3838

Merged
merged 2 commits into from
Oct 19, 2021
Merged

Strengthen the types for Bridge events #3838

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1748ca4
Strengthen the types for `Bridge` events
esanzgar Oct 8, 2021
b36b8e6
Apply suggestions from code review
esanzgar Oct 18, 2021
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
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@
"scripts": {
"build": "cross-env NODE_ENV=production gulp build",
"lint": "eslint .",
"checkformatting": "prettier --check '**/*.{js,scss}'",
"format": "prettier --list-different --write '**/*.{js,scss}'",
"checkformatting": "prettier --check '**/*.{js,scss,d.ts}'",
"format": "prettier --list-different --write '**/*.{js,scss,d.ts}'",
"test": "gulp test",
"typecheck": "tsc --build tsconfig.json",
"report-coverage": "codecov -f coverage/coverage-final.json",
Expand Down
5 changes: 2 additions & 3 deletions src/annotator/annotation-counts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import events from '../shared/bridge-events';

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that since no template parameters are supplied here, the type will be Bridge<any, any>. As a result, for calls to on and call, TS won't even check if the first argument is a string.

Passing in the bridge here is not really the best separation of responsibilities. What I would suggest we do in future is make the sidebar responsible for listening for the message and instead put only the logic for updating the page elements (the contents of updateAnnotationCountElems here) in a function in a separate module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue about any type doesn't seem to happen in version 4.5:
https://www.staging-typescript.org/play?ts=4.5.0-dev.20211015&filetype=js#code/PQKhCgAIUgBAXApgWwA4BsCGTIG8DkAFouugPb4A++ARgJ6L4C+kAKlDLKpgE6bJ5WLTB2DgAxlgDOUyMjoBhabNxRI60BHXrO3PgNxDII7dDGnxmUgApMASlWmLZAHZSy6RADpyAc1t2ANxq6kzgYeDgmnDwdKiIePJKmDIAPEQk5PgAfCwgYuKuUvCQAJZu8Jgu4ogAjJAAvJAuiADucorK1kHg5cVVNbVeljYZpBQ9UWAxcQm4Scrp9Iy5ZhJFJX2V1YgATI3NbR3JMt3BWwN7w1bo1rQM+JOFFWUVlwDMBy3tCylSZ703jt3tdRsRxo9zkCaiCRrd7owekA

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing in the bridge here is not really the best separation of responsibilities. What I would suggest we do in future is make the sidebar responsible for listening for the message and instead put only the logic for updating the page elements (the contents of updateAnnotationCountElems here) in a function in a separate module.

I agree.

*/

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

function updateAnnotationCountElems(newCount) {
const elems = rootEl.querySelectorAll('[' + ANNOTATION_COUNT_ATTR + ']');
Expand Down
21 changes: 14 additions & 7 deletions src/annotator/annotation-sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
/**
* @typedef {import('../shared/bridge').Bridge} Bridge
* @typedef {import('../types/annotator').AnnotationData} AnnotationData
* @typedef {import('../types/annotator').Destroyable} Destroyable
* @typedef {import('../types/bridge-events').GuestToSidebarEvent} GuestToSidebarEvent
* @typedef {import('../types/bridge-events').SidebarToGuestEvent} SidebarToGuestEvent
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these typedefs still needed given the change to the Bridge typedef above?

* @typedef {import('./util/emitter').EventBus} EventBus
*/

/**
* @template {GuestToSidebarEvent} T
* @template {SidebarToGuestEvent} U
* @typedef {import('../shared/bridge').Bridge<T,U>} Bridge
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we only ever use Bridge with one pair of type arguments in this file, you could just make the type arguments part of this typedef. eg.

/**
 * @typedef {import('../shared/bridge').Bridge<GuestToSidebarEvent, SidebarToGuestEvent>} SidebarBridge
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

*/

/**
* Message sent between the sidebar and annotator about an annotation that has
* changed.
Expand All @@ -26,11 +33,11 @@
export class AnnotationSync {
/**
* @param {EventBus} eventBus - Event bus for communicating with the annotator code (eg. the Guest)
* @param {Bridge} bridge - Channel for communicating with the sidebar
* @param {Bridge<GuestToSidebarEvent,SidebarToGuestEvent>} bridge - Channel for communicating with the sidebar
*/
constructor(eventBus, bridge) {
this._emitter = eventBus.createEmitter();
this.bridge = bridge;
this._sidebar = bridge;

/**
* Mapping from annotation tags to annotation objects for annotations which
Expand All @@ -43,7 +50,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._sidebar.on('deleteAnnotation', (body, callback) => {
if (this.destroyed) {
callback(null);
return;
Expand All @@ -55,7 +62,7 @@ export class AnnotationSync {
callback(null);
});

this.bridge.on('loadAnnotations', (bodies, callback) => {
this._sidebar.on('loadAnnotations', (bodies, callback) => {
if (this.destroyed) {
callback(null);
return;
Expand All @@ -70,7 +77,7 @@ export class AnnotationSync {
if (annotation.$tag) {
return;
}
this.bridge.call('beforeCreateAnnotation', this._format(annotation));
this._sidebar.call('beforeCreateAnnotation', this._format(annotation));
});
}

Expand All @@ -87,7 +94,7 @@ export class AnnotationSync {
return;
}

this.bridge.call(
this._sidebar.call(
'sync',
annotations.map(ann => this._format(ann))
);
Expand Down
3 changes: 1 addition & 2 deletions src/annotator/features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import events from '../shared/bridge-events';
import warnOnce from '../shared/warn-once';

let _features = {};
Expand All @@ -12,7 +11,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('featureFlagsUpdated', _set);
},

reset: function () {
Expand Down
8 changes: 7 additions & 1 deletion src/annotator/guest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import { normalizeURI } from './util/url';
* @typedef {import('../types/annotator').Destroyable} Destroyable
* @typedef {import('../types/annotator').SidebarLayout} SidebarLayout
* @typedef {import('../types/api').Target} Target
* @typedef {import('../types/bridge-events').GuestToSidebarEvent} GuestToSidebarEvent
* @typedef {import('../types/bridge-events').SidebarToGuestEvent} SidebarToGuestEvent
* @typedef {import('./util/emitter').EventBus} EventBus
*/

Expand Down Expand Up @@ -163,7 +165,11 @@ export default class Guest {
// 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;

// Set up listeners for messages coming from the sidebar to this guest.
/**
* Channel for sidebar-guest communication.
*
* @type {Bridge<GuestToSidebarEvent,SidebarToGuestEvent>}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to remove the type here and you will see that the it has a default template argument:

Screenshot 2021-10-18 at 17 26 54

*/
this._bridge = new Bridge();
this._bridge.onConnect(() => {
this._emitter.publish('panelReady');
Expand Down
19 changes: 13 additions & 6 deletions src/annotator/sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import Hammer from 'hammerjs';

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

import { annotationCounts } from './annotation-counts';
Expand All @@ -14,6 +13,8 @@ import { createShadowRoot } from './util/shadow-root';

/**
* @typedef {import('./guest').default} Guest
* @typedef {import('../types/bridge-events').HostToSidebarEvent} HostToSidebarEvent
* @typedef {import('../types/bridge-events').SidebarToHostEvent} SidebarToHostEvent
* @typedef {import('../types/annotator').SidebarLayout} SidebarLayout
* @typedef {import('../types/annotator').Destroyable} Destroyable
*/
Expand Down Expand Up @@ -64,6 +65,11 @@ export default class Sidebar {
constructor(element, eventBus, guest, config = {}) {
this._emitter = eventBus.createEmitter();

/**
* Channel for sidebar-host communication.
*
* @type {Bridge<HostToSidebarEvent,SidebarToHostEvent>}
*/
this._sidebarRPC = new Bridge();

/**
Expand Down Expand Up @@ -252,12 +258,13 @@ export default class Sidebar {
this.show();
});

/** @type {Array<[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],
['loginRequested', this.onLoginRequest],
['logoutRequested', this.onLogoutRequest],
['signupRequested', this.onSignupRequest],
['profileRequested', this.onProfileRequest],
['helpRequested', this.onHelpRequest],
];
eventHandlers.forEach(([event, handler]) => {
if (handler) {
Expand Down
5 changes: 2 additions & 3 deletions src/annotator/test/annotation-sync-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { EventBus } from '../util/emitter';

import { AnnotationSync } from '../annotation-sync';
import { EventBus } from '../util/emitter';

describe('AnnotationSync', () => {
let createAnnotationSync;
Expand Down Expand Up @@ -49,7 +48,7 @@ describe('AnnotationSync', () => {
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);
Expand Down
3 changes: 1 addition & 2 deletions src/annotator/test/features-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import events from '../../shared/bridge-events';
import { features, $imports } from '../features';

describe('features - annotation layer', () => {
Expand All @@ -22,7 +21,7 @@ describe('features - annotation layer', () => {

features.init({
on: function (topic, handler) {
if (topic === events.FEATURE_FLAGS_UPDATED) {
if (topic === 'featureFlagsUpdated') {
featureFlagsUpdateHandler = handler;
}
},
Expand Down
29 changes: 14 additions & 15 deletions src/annotator/test/guest-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import Guest from '../guest';
import Guest, { $imports } from '../guest';
import { EventBus } from '../util/emitter';
import { $imports } from '../guest';

class FakeAdder {
constructor(container, options) {
Expand Down Expand Up @@ -234,7 +233,7 @@ describe('Guest', () => {
});

describe('events from sidebar', () => {
const emitGuestEvent = (event, ...args) => {
const emitSidebarEvent = (event, ...args) => {
for (let [evt, fn] of fakeBridge.on.args) {
if (event === evt) {
fn(...args);
Expand All @@ -252,7 +251,7 @@ describe('Guest', () => {
{ annotation: { $tag: 'tag2' }, highlights: [highlight1] },
];

emitGuestEvent('focusAnnotations', ['tag1']);
emitSidebarEvent('focusAnnotations', ['tag1']);

assert.calledWith(
highlighter.setHighlightsFocused,
Expand All @@ -270,7 +269,7 @@ describe('Guest', () => {
{ annotation: { $tag: 'tag2' }, highlights: [highlight1] },
];

emitGuestEvent('focusAnnotations', ['tag1']);
emitSidebarEvent('focusAnnotations', ['tag1']);

assert.calledWith(
highlighter.setHighlightsFocused,
Expand All @@ -282,8 +281,8 @@ describe('Guest', () => {
it('updates focused tag set', () => {
const guest = createGuest();

emitGuestEvent('focusAnnotations', ['tag1']);
emitGuestEvent('focusAnnotations', ['tag2', 'tag3']);
emitSidebarEvent('focusAnnotations', ['tag1']);
emitSidebarEvent('focusAnnotations', ['tag2', 'tag3']);

assert.deepEqual([...guest.focusedAnnotationTags], ['tag2', 'tag3']);
});
Expand All @@ -302,7 +301,7 @@ describe('Guest', () => {
},
];

emitGuestEvent('scrollToAnnotation', 'tag1');
emitSidebarEvent('scrollToAnnotation', 'tag1');

assert.called(fakeIntegration.scrollToAnchor);
assert.calledWith(fakeIntegration.scrollToAnchor, guest.anchors[0]);
Expand All @@ -326,7 +325,7 @@ describe('Guest', () => {
resolve();
});

emitGuestEvent('scrollToAnnotation', 'tag1');
emitSidebarEvent('scrollToAnnotation', 'tag1');
});
});

Expand All @@ -345,7 +344,7 @@ describe('Guest', () => {
event.preventDefault()
);

emitGuestEvent('scrollToAnnotation', 'tag1');
emitSidebarEvent('scrollToAnnotation', 'tag1');

assert.notCalled(fakeIntegration.scrollToAnchor);
});
Expand All @@ -354,7 +353,7 @@ describe('Guest', () => {
const guest = createGuest();

guest.anchors = [{ annotation: { $tag: 'tag1' } }];
emitGuestEvent('scrollToAnnotation', 'tag1');
emitSidebarEvent('scrollToAnnotation', 'tag1');

assert.notCalled(fakeIntegration.scrollToAnchor);
});
Expand All @@ -374,7 +373,7 @@ describe('Guest', () => {
const eventEmitted = sandbox.stub();
guest.element.addEventListener('scrolltorange', eventEmitted);

emitGuestEvent('scrollToAnnotation', 'tag1');
emitSidebarEvent('scrollToAnnotation', 'tag1');

assert.notCalled(eventEmitted);
assert.notCalled(fakeIntegration.scrollToAnchor);
Expand Down Expand Up @@ -408,7 +407,7 @@ describe('Guest', () => {

fakeIntegration.getMetadata.resolves(metadata);

emitGuestEvent(
emitSidebarEvent(
'getDocumentInfo',
createCallback('https://example.com/test.pdf', metadata, done)
);
Expand All @@ -419,14 +418,14 @@ describe('Guest', () => {
it('sets visibility of highlights in document', () => {
const guest = createGuest();

emitGuestEvent('setVisibleHighlights', true);
emitSidebarEvent('setVisibleHighlights', true);
assert.calledWith(
highlighter.setHighlightsVisible,
guest.element,
true
);

emitGuestEvent('setVisibleHighlights', false);
emitSidebarEvent('setVisibleHighlights', false);
assert.calledWith(
highlighter.setHighlightsVisible,
guest.element,
Expand Down
Loading