Skip to content

Commit

Permalink
Replace lookup by TS type
Browse files Browse the repository at this point in the history
I have created type definitions for all the names of events that are
sent across the different frames using various `Bridge`s. It is based on
the previous `bridge-events.js`. I broke down the events in four
sections based on the direction of the messages:

* host -> sidebar events
* sidebar -> host events
* sidebar -> guest/s events
* guest -> sidebar events

For those events that didn't have a description I added one.

This is more stringent and less verbose than the previous lookup system.
  • Loading branch information
esanzgar committed Oct 11, 2021
1 parent 0356d20 commit ab7d2c0
Show file tree
Hide file tree
Showing 20 changed files with 194 additions and 134 deletions.
4 changes: 2 additions & 2 deletions package.json
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
7 changes: 1 addition & 6 deletions src/annotator/annotation-counts.js
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,10 +9,7 @@ const ANNOTATION_COUNT_ATTR = 'data-hypothesis-annotation-count';
*/

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

function updateAnnotationCountElems(newCount) {
const elems = rootEl.querySelectorAll('[' + ANNOTATION_COUNT_ATTR + ']');
Expand Down
4 changes: 2 additions & 2 deletions src/annotator/cross-frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export class CrossFrame {
/**
* Subscribe to an event from the sidebar.
*
* @param {string} method
* @param {import('../types/bridge-events').BrideEvents} method
* @param {(...args: any[]) => void} listener
*
* @see {Bridge.on} for details.
Expand All @@ -76,7 +76,7 @@ export class CrossFrame {
/**
* Call an RPC method exposed by the sidebar to the annotator.
*
* @param {string} method - Name of remote method to call.
* @param {import('../types/bridge-events').BrideEvents} method - Name of remote method to call.
* @param {any[]} args
*
* @see {Bridge.call} for details.
Expand Down
3 changes: 1 addition & 2 deletions src/annotator/features.js
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 @@ -9,7 +8,7 @@ const _set = features => {

export default {
init: function (crossframe) {
crossframe.on(events.FEATURE_FLAGS_UPDATED, _set);
crossframe.on('featureFlagsUpdated', _set);
},

reset: function () {
Expand Down
12 changes: 6 additions & 6 deletions src/annotator/sidebar.js
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 Down Expand Up @@ -252,12 +251,13 @@ export default class Sidebar {
this.show();
});

/** @type{Array<[import('../types/bridge-events').BrideEvents, 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
3 changes: 1 addition & 2 deletions src/annotator/test/features-test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import events from '../../shared/bridge-events';
import features from '../features';
import { $imports } from '../features';

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

features.init({
on: function (topic, handler) {
if (topic === events.FEATURE_FLAGS_UPDATED) {
if (topic === 'featureFlagsUpdated') {
featureFlagsUpdateHandler = handler;
}
},
Expand Down
25 changes: 11 additions & 14 deletions src/annotator/test/sidebar-test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
import events from '../../shared/bridge-events';

import Sidebar, { MIN_RESIZE } from '../sidebar';
import { $imports } from '../sidebar';
import Sidebar, { MIN_RESIZE, $imports } from '../sidebar';
import { EventBus } from '../util/emitter';

const DEFAULT_WIDTH = 350;
Expand Down Expand Up @@ -366,7 +363,7 @@ describe('Sidebar', () => {
const onLoginRequest = sandbox.stub();
createSidebar({ services: [{ onLoginRequest }] });

emitEvent(events.LOGIN_REQUESTED);
emitEvent('loginRequested');

assert.called(onLoginRequest);
});
Expand All @@ -386,7 +383,7 @@ describe('Sidebar', () => {
],
});

emitEvent(events.LOGIN_REQUESTED);
emitEvent('loginRequested');

assert.called(firstOnLogin);
assert.notCalled(secondOnLogin);
Expand All @@ -406,25 +403,25 @@ describe('Sidebar', () => {
],
});

emitEvent(events.LOGIN_REQUESTED);
emitEvent('loginRequested');

assert.notCalled(secondOnLogin);
assert.notCalled(thirdOnLogin);
});

it('does not crash if there is no services', () => {
createSidebar(); // No config.services
emitEvent(events.LOGIN_REQUESTED);
emitEvent('loginRequested');
});

it('does not crash if services is an empty array', () => {
createSidebar({ services: [] });
emitEvent(events.LOGIN_REQUESTED);
emitEvent('loginRequested');
});

it('does not crash if the first service has no onLoginRequest', () => {
createSidebar({ services: [{}] });
emitEvent(events.LOGIN_REQUESTED);
emitEvent('loginRequested');
});
});

Expand All @@ -433,7 +430,7 @@ describe('Sidebar', () => {
const onLogoutRequest = sandbox.stub();
createSidebar({ services: [{ onLogoutRequest }] });

emitEvent(events.LOGOUT_REQUESTED);
emitEvent('logoutRequested');

assert.called(onLogoutRequest);
}));
Expand All @@ -443,7 +440,7 @@ describe('Sidebar', () => {
const onSignupRequest = sandbox.stub();
createSidebar({ services: [{ onSignupRequest }] });

emitEvent(events.SIGNUP_REQUESTED);
emitEvent('signupRequested');

assert.called(onSignupRequest);
}));
Expand All @@ -453,7 +450,7 @@ describe('Sidebar', () => {
const onProfileRequest = sandbox.stub();
createSidebar({ services: [{ onProfileRequest }] });

emitEvent(events.PROFILE_REQUESTED);
emitEvent('profileRequested');

assert.called(onProfileRequest);
}));
Expand All @@ -463,7 +460,7 @@ describe('Sidebar', () => {
const onHelpRequest = sandbox.stub();
createSidebar({ services: [{ onHelpRequest }] });

emitEvent(events.HELP_REQUESTED);
emitEvent('helpRequested');

assert.called(onHelpRequest);
}));
Expand Down
48 changes: 0 additions & 48 deletions src/shared/bridge-events.js

This file was deleted.

4 changes: 2 additions & 2 deletions src/shared/bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export class Bridge {
* Make a method call on all channels, collect the results and pass them to a
* callback when all results are collected.
*
* @param {string} method - Name of remote method to call.
* @param {import('../types/bridge-events').BrideEvents} method - Name of remote method to call.
* @param {any[]} args - Arguments to method. Final argument is an optional
* callback with this type: `(error: string|Error|null, ...result: any[]) => void`.
* This callback, if any, will be triggered once a response (via `postMessage`)
Expand Down Expand Up @@ -132,7 +132,7 @@ export class Bridge {
* Register a listener to be invoked when any connected channel sends a
* message to this `Bridge`.
*
* @param {string} method
* @param {import('../types/bridge-events').BrideEvents} method
* @param {(...args: any[]) => void} listener -- Final argument is an optional
* callback of the type: `(error: string|Error|null, ...result: any[]) => void`.
* This callback must be invoked in order to respond (via `postMessage`)
Expand Down
7 changes: 3 additions & 4 deletions src/sidebar/components/HypothesisApp.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import classnames from 'classnames';
import { useEffect, useMemo } from 'preact/hooks';

import bridgeEvents from '../../shared/bridge-events';
import { confirm } from '../../shared/prompts';
import { serviceConfig } from '../config/service-config';
import { useStoreProxy } from '../store/use-store';
Expand Down Expand Up @@ -98,7 +97,7 @@ function HypothesisApp({ auth, frameSync, settings, session, toastMessenger }) {
const login = async () => {
if (serviceConfig(settings)) {
// Let the host page handle the login request
frameSync.notifyHost(bridgeEvents.LOGIN_REQUESTED);
frameSync.notifyHost('loginRequested');
return;
}

Expand All @@ -116,7 +115,7 @@ function HypothesisApp({ auth, frameSync, settings, session, toastMessenger }) {
const signUp = () => {
if (serviceConfig(settings)) {
// Let the host page handle the signup request
frameSync.notifyHost(bridgeEvents.SIGNUP_REQUESTED);
frameSync.notifyHost('signupRequested');
return;
}
window.open(store.getLink('signup'));
Expand Down Expand Up @@ -156,7 +155,7 @@ function HypothesisApp({ auth, frameSync, settings, session, toastMessenger }) {
store.discardAllDrafts();

if (serviceConfig(settings)) {
frameSync.notifyHost(bridgeEvents.LOGOUT_REQUESTED);
frameSync.notifyHost('logoutRequested');
return;
}

Expand Down
3 changes: 1 addition & 2 deletions src/sidebar/components/TopBar.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { IconButton, LinkButton } from '@hypothesis/frontend-shared';

import bridgeEvents from '../../shared/bridge-events';
import { serviceConfig } from '../config/service-config';
import { useStoreProxy } from '../store/use-store';
import { isThirdPartyService } from '../helpers/is-third-party-service';
Expand Down Expand Up @@ -71,7 +70,7 @@ function TopBar({
const requestHelp = () => {
const service = serviceConfig(settings);
if (service && service.onHelpRequestProvided) {
frameSync.notifyHost(bridgeEvents.HELP_REQUESTED);
frameSync.notifyHost('helpRequested');
} else {
store.toggleSidebarPanel('help');
}
Expand Down
3 changes: 1 addition & 2 deletions src/sidebar/components/UserMenu.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { SvgIcon } from '@hypothesis/frontend-shared';
import { useState } from 'preact/hooks';

import bridgeEvents from '../../shared/bridge-events';
import { serviceConfig } from '../config/service-config';
import { isThirdPartyUser } from '../helpers/account-id';
import { useStoreProxy } from '../store/use-store';
Expand Down Expand Up @@ -70,7 +69,7 @@ function UserMenu({ auth, frameSync, onLogout, settings }) {
};

const onProfileSelected = () =>
isThirdParty && frameSync.notifyHost(bridgeEvents.PROFILE_REQUESTED);
isThirdParty && frameSync.notifyHost('profileRequested');

// Generate dynamic props for the profile <MenuItem> component
const profileItemProps = (() => {
Expand Down
14 changes: 3 additions & 11 deletions src/sidebar/components/test/HypothesisApp-test.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { mount } from 'enzyme';

import bridgeEvents from '../../../shared/bridge-events';
import mockImportedComponents from '../../../test-util/mock-imported-components';

import HypothesisApp, { $imports } from '../HypothesisApp';

describe('HypothesisApp', () => {
Expand Down Expand Up @@ -228,10 +226,7 @@ describe('HypothesisApp', () => {
it('sends SIGNUP_REQUESTED event', () => {
const wrapper = createComponent();
clickSignUp(wrapper);
assert.calledWith(
fakeFrameSync.notifyHost,
bridgeEvents.SIGNUP_REQUESTED
);
assert.calledWith(fakeFrameSync.notifyHost, 'signupRequested');
});

it('does not open a URL directly', () => {
Expand Down Expand Up @@ -304,7 +299,7 @@ describe('HypothesisApp', () => {

assert.equal(fakeFrameSync.notifyHost.callCount, 1);
assert.isTrue(
fakeFrameSync.notifyHost.calledWithExactly(bridgeEvents.LOGIN_REQUESTED)
fakeFrameSync.notifyHost.calledWithExactly('loginRequested')
);
});
});
Expand Down Expand Up @@ -412,10 +407,7 @@ describe('HypothesisApp', () => {
await clickLogOut(wrapper);

assert.calledOnce(fakeFrameSync.notifyHost);
assert.calledWithExactly(
fakeFrameSync.notifyHost,
bridgeEvents.LOGOUT_REQUESTED
);
assert.calledWithExactly(fakeFrameSync.notifyHost, 'logoutRequested');
});

it('does not send LOGOUT_REQUESTED if the user cancels the prompt', async () => {
Expand Down
10 changes: 2 additions & 8 deletions src/sidebar/components/test/TopBar-test.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import { mount } from 'enzyme';

import bridgeEvents from '../../../shared/bridge-events';
import TopBar from '../TopBar';
import { $imports } from '../TopBar';

import TopBar, { $imports } from '../TopBar';
import { checkAccessibility } from '../../../test-util/accessibility';
import mockImportedComponents from '../../../test-util/mock-imported-components';

Expand Down Expand Up @@ -123,10 +120,7 @@ describe('TopBar', () => {
helpButton.props().onClick();

assert.equal(fakeStore.toggleSidebarPanel.callCount, 0);
assert.calledWith(
fakeFrameSync.notifyHost,
bridgeEvents.HELP_REQUESTED
);
assert.calledWith(fakeFrameSync.notifyHost, 'helpRequested');
});
});
});
Expand Down
Loading

0 comments on commit ab7d2c0

Please sign in to comment.