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

Conversation

esanzgar
Copy link
Contributor

In contrast to #3829, we have
decided to re-enforce the use of the lookup enums for the event names
used by Bridge.

I have added definitions for all the events that are sent across the
different frames using various Bridges. I broke down the events into
four sections based on the direction of the messages:

  • guest -> sidebar
  • host -> sidebar
  • sidebar -> guest/s
  • sidebar -> host

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #3833 (5da6cbf) into master (910b3da) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 5da6cbf differs from pull request most recent head 7bb7b76. Consider uploading reports for the commit 7bb7b76 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3833   +/-   ##
=======================================
  Coverage   99.02%   99.02%           
=======================================
  Files         211      212    +1     
  Lines        7797     7806    +9     
  Branches     1757     1757           
=======================================
+ Hits         7721     7730    +9     
  Misses         76       76           
Impacted Files Coverage Δ
src/shared/bridge.js 100.00% <ø> (ø)
src/sidebar/services/features.js 100.00% <ø> (ø)
src/annotator/annotation-counts.js 100.00% <100.00%> (ø)
src/annotator/annotation-sync.js 100.00% <100.00%> (ø)
src/annotator/features.js 100.00% <100.00%> (ø)
src/annotator/guest.js 99.17% <100.00%> (+<0.01%) ⬆️
src/annotator/sidebar.js 98.14% <100.00%> (+<0.01%) ⬆️
src/shared/bridge-events.js 100.00% <100.00%> (ø)
src/sidebar/components/HypothesisApp.js 100.00% <100.00%> (ø)
src/sidebar/components/TopBar.js 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 910b3da...7bb7b76. Read the comment docs.

Comment on lines +9 to +10
* @typedef {'destroyFrame'|'setVisibleHighlights'|'sidebarOpened'} HostToSidebarEvent
* @type {Record<'DESTROY_FRAME'|'SET_VISIBLE_HIGHLIGHTS'|'SIDEBAR_OPENED', HostToSidebarEvent>}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In next month's version of TS (microsoft/TypeScript#45418), this will probably simplify to:

Suggested change
* @typedef {'destroyFrame'|'setVisibleHighlights'|'sidebarOpened'} HostToSidebarEvent
* @type {Record<'DESTROY_FRAME'|'SET_VISIBLE_HIGHLIGHTS'|'SIDEBAR_OPENED', HostToSidebarEvent>}
* @typedef {hostToSidebarEvents[keyof hosToSidebarEvents]} HostToSidebarEvent
* @type {const}

The types can be inferred, instead of re-written.

* @typedef {'destroyFrame'|'setVisibleHighlights'|'sidebarOpened'} HostToSidebarEvent
* @type {Record<'DESTROY_FRAME'|'SET_VISIBLE_HIGHLIGHTS'|'SIDEBAR_OPENED', HostToSidebarEvent>}
*/
export const hostToSidebarEvents = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This name could be shortened to hostToSidebar, for example.

@esanzgar esanzgar force-pushed the bridge-event-enum branch 3 times, most recently from b05e133 to b1b5374 Compare October 14, 2021 11:54
@@ -234,7 +237,7 @@ describe('Guest', () => {
});

describe('events from sidebar', () => {
const emitGuestEvent = (event, ...args) => {
const emitSidebarEvent = (event, ...args) => {
Copy link
Contributor Author

@esanzgar esanzgar Oct 14, 2021

Choose a reason for hiding this comment

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

I renamed this method because the events are really sent from the sidebar frame.

@@ -361,12 +366,12 @@ describe('Sidebar', () => {
});
});

describe('on LOGIN_REQUESTED event', () => {
describe('on "loginRequest" event', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other test have the name of the actual event, so I decided to do the same here. If we prefer we can revert all the reference to event names for the key in the enum lookup.

@esanzgar esanzgar force-pushed the bridge-event-enum branch 2 times, most recently from 02b5dcd to 135e1d7 Compare October 14, 2021 12:16
In contrast to #3829, we have
decided to re-enforce the use of the lookup enums for the event names
used by `Bridge`.

I have added definitions for all the events that are sent across the
different frames using various `Bridge`s. I broke down the events into
four sections based on the direction of the messages:

* guest -> sidebar events
* host -> sidebar events
* sidebar -> guest/s events
* sidebar -> host events
For greater consistency, we decided to use the enums defined in
`brdige-events.js` everywhere, including test.
Reorder imports according to the team conventions.
@esanzgar esanzgar force-pushed the bridge-event-enum branch 2 times, most recently from 5da6cbf to 7bb7b76 Compare October 14, 2021 14:08
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

I'm going to be really annoying 😛. Now that I've been able to do a side-by-side comparison of the approach in yesterday's PR with string literals/unions (3e8cb2f) and the approach in this PR with an enum, I have to say that yesterday's version seems more appealing to me. The union-based does have the disadvantage of not supporting find-all-references, but it's also a lot more succinct and I found the code easier to scan.

I did find some mistakes in the unions though, revealed by adding types to the bridge's call and on methods:

diff --git a/src/shared/bridge.js b/src/shared/bridge.js
index 06651c226..950a8a961 100644
--- a/src/shared/bridge.js
+++ b/src/shared/bridge.js
@@ -6,6 +6,8 @@ import { PortRPC } from './port-rpc';
  * The Bridge service sets up a channel between frames and provides an events
  * API on top of it.
  *
+ * @template {string} CallMethods - Names of methods that can be called (via {@link call})
+ * @template {string} OnMethods - Names of methods that can be handled (via {@link on})
  * @implements Destroyable
  */
 export class Bridge {
@@ -66,7 +68,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 {import('../types/bridge-events').BridgeEvent} method - Name of remote method to call.
+   * @param {CallMethods} method
    * @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`)
@@ -132,7 +134,7 @@ export class Bridge {
    * Register a listener to be invoked when any connected channel sends a
    * message to this `Bridge`.
    *
-   * @param {import('../types/bridge-events').BridgeEvent} method
+   * @param {OnMethods|'connect'} 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`)
diff --git a/src/sidebar/services/frame-sync.js b/src/sidebar/services/frame-sync.js
index 71b631d7f..eee6576c4 100644
--- a/src/sidebar/services/frame-sync.js
+++ b/src/sidebar/services/frame-sync.js
@@ -5,6 +5,10 @@ import { isReply, isPublic } from '../helpers/annotation-metadata';
 import { watch } from '../util/watch';
 
 /**
+ * @typedef {import('../../types/bridge-events').SidebarToHostEvent} SidebarToHostEvent
+ * @typedef {import('../../types/bridge-events').HostToSidebarEvent} HostToSidebarEvent
+ * @typedef {import('../../types/bridge-events').SidebarToGuestEvent} SidebarToGuestEvent
+ * @typedef {import('../../types/bridge-events').GuestToSidebarEvent} GuestToSidebarEvent
  * @typedef {import('../../shared/port-rpc').PortRPC} PortRPC
  */
 
@@ -54,10 +58,18 @@ export class FrameSyncService {
    * @param {import('../store').SidebarStore} store
    */
   constructor($window, annotationsService, store) {
-    /** Channel for sidebar <-> host communication. */
+    /**
+      * Channel for sidebar <-> host communication.
+      *
+      * @type {Bridge<SidebarToHostEvent,HostToSidebarEvent>}
+      */
     this._hostRPC = new Bridge();
 
-    /** Channel for sidebar <-> guest(s) communication. */
+    /**
+      * Channel for sidebar <-> guest(s) communication.
+      *
+      * @type {Bridge<SidebarToGuestEvent,GuestToSidebarEvent>}
+      */
     this._guestRPC = new Bridge();
 
     this._store = store;

With the above diff you get some errors in frame-sync.js because some of the method names are in the wrong union or missing.

@esanzgar
Copy link
Contributor Author

We close in favour of #3838

@esanzgar esanzgar closed this Oct 15, 2021
@esanzgar esanzgar deleted the bridge-event-enum branch October 20, 2021 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants