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

Strengthen the types for Bridge events #3838

Merged
merged 2 commits into from
Oct 19, 2021
Merged

Strengthen the types for Bridge events #3838

merged 2 commits into from
Oct 19, 2021

Conversation

esanzgar
Copy link
Contributor

I have created type definitions for all the event names that are sent
across the different frames using various Bridges. It is based on the
previous bridge-events.js. I broke down the events in four sections
based on the direction of the messages:

  • guest -> sidebar events
  • host -> sidebar events
  • sidebar -> guest/s events
  • sidebar -> host 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.

@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #3838 (b36b8e6) into master (910b3da) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3838      +/-   ##
==========================================
- Coverage   99.02%   99.02%   -0.01%     
==========================================
  Files         211      211              
  Lines        7797     7796       -1     
  Branches     1757     1757              
==========================================
- Hits         7721     7720       -1     
  Misses         76       76              
Impacted Files Coverage Δ
src/annotator/guest.js 99.16% <ø> (ø)
src/annotator/sidebar.js 98.13% <ø> (ø)
src/shared/bridge.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/sidebar/components/HypothesisApp.js 100.00% <100.00%> (ø)
src/sidebar/components/TopBar.js 100.00% <100.00%> (ø)
src/sidebar/components/UserMenu.js 100.00% <100.00%> (ø)
src/sidebar/services/features.js 100.00% <100.00%> (ø)
... and 1 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...b36b8e6. Read the comment docs.

@esanzgar esanzgar force-pushed the bridge-event-types branch 3 times, most recently from 545ce16 to e6b39e5 Compare October 15, 2021 13:04
I have created type definitions for all the event names 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:

* guest -> sidebar events
* host -> sidebar events
* sidebar -> guest/s events
* sidebar -> host 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.
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.

The functional code changes look good. I found a few minor typos of event names in test descriptions.

The main feedback I have is regarding the comments in the bridge-events.d.ts file. There are a few that are incorrect, some in obvious ways and others in more subtle ones, and others that reference implementation details of how the event is currently handled rather what the event means. For example when the guest sends "closeSidebar" to the sidebar app it is requesting that the sidebar be closed. How exactly the sidebar makes this happen is an implementation detail that might change in future. The most common issue I see though is where a comment restates the event and type name with the same words. Unless a comment is adding value (eg. by clarifying something about when the event is emitted, or why, or otherwise providing useful additional information), I would prefer to remove it.


/**
* The Bridge service sets up a channel between frames and provides an events
* API on top of it.
*
* @template {BridgeEvent} CallMethods - Names of methods that can be called (via {@link call})
* @template {BridgeEvent} OnMethods - Names of methods that can be handled (via {@link on})
Copy link
Member

Choose a reason for hiding this comment

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

Per a Slack thread from last week, this is where defaults for template arguments might be useful. You could use [CallMethods=BridgeEvent] and any use of Bridge without arguments would use that. As it is, there are several places where no template arguments are specified and so it will default to any.

I probably would have been inclined to just use string as the base type here, to avoid coupling Bridge to the specific events that it is used with in the client.

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 advantage of using BridgeEvent instead of string is that it autocompletes the possible event types:

Screenshot 2021-10-15 at 20 00 18

The above works if the Bridge is not redefined with a typedef.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. That seems useful, although presumably this would also work if the Bridge instance has an annotation to specify the specific events that it uses?

src/sidebar/components/test/HypothesisApp-test.js Outdated Show resolved Hide resolved
@@ -0,0 +1,161 @@
/**
* This module defines the set of global events that are dispatched across the
* the bridge(s) between the sidebar-host and sidebar-guest(s).
Copy link
Member

Choose a reason for hiding this comment

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

I would change this to be a little more general. Something like: "This module defines the events that are sent between frames with different roles in the client (guest, host, sidebar)".

I expect that we will want to add additional channels in future, eg. guest <-> host or notebook <-> host and this change would avoid needing to update this comment each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏼

src/types/bridge-events.d.ts Outdated Show resolved Hide resolved
src/types/bridge-events.d.ts Outdated Show resolved Hide resolved
/**
* @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.

👍🏼

src/annotator/test/sidebar-test.js Outdated Show resolved Hide resolved
src/annotator/test/sidebar-test.js Outdated Show resolved Hide resolved
* @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
Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate how verbose this gets. There have been some discussions about better options in microsoft/TypeScript#22160 but I don't see a solution yet.

| 'profileRequested'

/**
* The sidebar inform the host to update the number of annotations in the partner site.
Copy link
Member

Choose a reason for hiding this comment

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

I think there is ambiguity here about what "number of annotations" means. There is already a clearer explanation in https://h.readthedocs.io/projects/client/en/latest/publishers/host-page-integration/#cmdoption-arg-data-hypothesis-annotation-count which we could link to.

@@ -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
Contributor Author

Choose a reason for hiding this comment

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

@@ -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
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.

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

Choose a reason for hiding this comment

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

👍🏼

@@ -0,0 +1,161 @@
/**
* This module defines the set of global events that are dispatched across the
* the bridge(s) between the sidebar-host and sidebar-guest(s).
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 Bridge service sets up a channel between frames and provides an events
* API on top of it.
*
* @template {BridgeEvent} CallMethods - Names of methods that can be called (via {@link call})
* @template {BridgeEvent} OnMethods - Names of methods that can be handled (via {@link on})
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 advantage of using BridgeEvent instead of string is that it autocompletes the possible event types:

Screenshot 2021-10-15 at 20 00 18

The above works if the Bridge is not redefined with a typedef.

/**
* 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

@esanzgar
Copy link
Contributor Author

I have accepted your changes regarding the descriptions of the events. Most of the descriptions were created previously.

Some of the messages brings clarity to how the events flow. For example, I found it useful to know that the closeSidebar flows from guest -> sidebar -> host.

Some other events are difficult to explain even with the description: toggleAnnotationSelection vs. focusAnnotations

/**
* The host informs the sidebar that a guest frame has been destroyed
*/
| 'destroyFrame'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think frameDestroyed would be clearer? (It sounds symmetric to sidebarOpened)

Copy link
Member

Choose a reason for hiding this comment

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

Probably. Some of the RPC calls defined in this module represent events that happened, without the caller necessarily expecting a specific response from the other frame. Other calls are commands that ask the receiving frame to do something specific (eg. openSidebar). destroyFrame does seem more of an event than a command.

@@ -76,7 +76,7 @@ export type SidebarToGuestEvent =
| 'focusAnnotations'

/**
* The sidebar is asking the guest(s) to get the document metadata.
* The sidebar is asking the guest(s) the URL and other metadata about the document.
Copy link
Member

Choose a reason for hiding this comment

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

"the URL" => "for the URL"

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.

LGTM. Thanks for your patience with this.

* @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?


/**
* The Bridge service sets up a channel between frames and provides an events
* API on top of it.
*
* @template {BridgeEvent} CallMethods - Names of methods that can be called (via {@link call})
* @template {BridgeEvent} OnMethods - Names of methods that can be handled (via {@link on})
Copy link
Member

Choose a reason for hiding this comment

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

Ah. That seems useful, although presumably this would also work if the Bridge instance has an annotation to specify the specific events that it uses?

@esanzgar esanzgar merged commit 0b757c6 into master Oct 19, 2021
@esanzgar esanzgar deleted the bridge-event-types branch October 19, 2021 13:52
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