Types for bridge events #3829

esanzgar · 2021-10-11T13:56:38Z

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:

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.

codecov · 2021-10-11T14:00:04Z

Codecov Report

Merging #3829 (f27cef3) into master (910b3da) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f27cef3 differs from pull request most recent head adaf1f7. Consider uploading reports for the commit adaf1f7 to get more accurate results

@@           Coverage Diff           @@
##           master    #3829   +/-   ##
=======================================
  Coverage   99.02%   99.02%           
=======================================
  Files         211      212    +1     
  Lines        7797     7808   +11     
  Branches     1757     1757           
=======================================
+ Hits         7721     7732   +11     
  Misses         76       76

Impacted Files	Coverage Δ
src/annotator/cross-frame.js	`100.00% <ø> (ø)`
src/annotator/sidebar.js	`98.13% <ø> (ø)`
src/shared/bridge.js	`100.00% <ø> (ø)`
src/annotator/annotation-counts.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%> (ø)`
src/sidebar/services/frame-sync.js	`100.00% <100.00%> (ø)`
... and 4 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...adaf1f7. Read the comment docs.

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.

According to our conventions.

esanzgar · 2021-10-11T14:25:54Z

src/sidebar/services/frame-sync.js

@@ -282,7 +278,7 @@ export class FrameSyncService {
  /**
   * Send an RPC message to the host frame.
   *
-   * @param {string} method
+   * @param {import('../../types/bridge-events').BrideEvents} method


We could make it more strict by allowing only to use only the SidebarToHost events.

Yeah, I think we do want to use the more specific type here to make it easier for the reader to see what the allowed values are.

esanzgar · 2021-10-11T14:27:12Z

src/types/bridge-events.d.ts

+  SHOW_ANNOTATIONS: 'showAnnotations';
+
+  /**
+   * The guest is asking the sidebar to sync the annotations


The description could be better here. Any help?

This event is used by the guest to notify the sidebar about the anchoring status of annotations (did we find it in the page). More generally we might in future want to use this event to relay information about the annotation that is only available after anchoring, such as the matched text if we didn't find an exact match for the stored quote, or the location in the document where the match was found (eg. what page number?)

esanzgar · 2021-10-11T14:31:08Z

src/types/bridge-events.d.ts

+  /**
+   * The guest is asking the sidebar to create an annotation
+   */
+  BEFORE_CREATE_ANNOTATION: 'beforeCreateAnnotation';


I would suggest to change the name to this event to: createAnnotation.

This is the event that creates the annotation.

I think that would make sense too. I think the original name may have been intended to mean that the annotation is not fully created until it is saved on the server. createAnnotation seems more obvious.

robertknight

I found a typo (BrideEvents) and had some suggestions regarding the structure of the types (they can just be a union rather than a map). Adding types to describe the events that are allowed in the different channels (guest <-> sidebar, guest <-> host, sidebar <-> host) etc. seems useful.

Enabling support for writing type-only modules in .d.ts files is good with me. It does mean adding an additional extension/format, but tools (formatting, syntax highlighting etc.) have better support for it. I had a question about the effects of using .d.ts vs .ts. My understanding is that the main difference is that .d.ts files don't generate any JS output and can't be used in non-type contexts. Is there anything else we should be aware of?

One practical disadvantage I realized of changing from an enum to a union is that because the values are just strings rather than imported symbols, you can't use go-to-definition or find-references for them. Instead you have to first look at the definition of the parameter (eg. for notifyHost or call or on) and then look at the definition of that type. I suppose that's the same as any other context where we are using a union of string literals. Modern TS does support a way of defining enums with objects that can be used in JSDoc as of TS 4.5 (next, but not current release).

I think here we just need to make a decision/have some guidelines about when to use one or the other of these options (enum vs union).

robertknight · 2021-10-12T08:19:41Z

src/annotator/sidebar.js

@@ -252,12 +251,13 @@ export default class Sidebar {
      this.show();
    });

+    /** @type{Array<[import('../types/bridge-events').BrideEvents, function]>} */


There is a typo here - BrideEvents. Also there should be a space after @type.

robertknight · 2021-10-12T08:23:41Z

src/types/bridge-events.d.ts

+/**
+ * Events that the sidebar sends to the host
+ */
+type SidebarToHostEvents = {


I think this could be a union rather than an object, avoiding the need for the unused ALL_CAPS names for events. I believe this will still allow you to add comments for individual entries.

type SidebarToHostEvents = 'closeSidebar' | 'openSidebar' ...

Also, I think it would be more idiomatic to use a singular name for unions. ie. SidebarToHostEvent, BridgeEvent, because an individual value is only one of those possibilities. On the other hand if the value was a bitmask (eg. what modifiers were held down when a key was pressed) then plural would be appropriate.

I agree. I reused the bridge-event.js file does why I keep the structure. I will change the name of the type to be singular.

robertknight · 2021-10-12T08:25:13Z

src/types/bridge-events.d.ts

+  /**
+   * The guest is asking the sidebar to create an annotation
+   */
+  BEFORE_CREATE_ANNOTATION: 'beforeCreateAnnotation';


I think that would make sense too. I think the original name may have been intended to mean that the annotation is not fully created until it is saved on the server. createAnnotation seems more obvious.

robertknight · 2021-10-12T08:27:03Z

src/types/bridge-events.d.ts

+  SHOW_ANNOTATIONS: 'showAnnotations';
+
+  /**
+   * The guest is asking the sidebar to sync the annotations


This event is used by the guest to notify the sidebar about the anchoring status of annotations (did we find it in the page). More generally we might in future want to use this event to relay information about the annotation that is only available after anchoring, such as the matched text if we didn't find an exact match for the stored quote, or the location in the document where the match was found (eg. what page number?)

robertknight · 2021-10-12T08:27:13Z

src/types/bridge-events.d.ts

+  TOGGLE_ANNOTATION_SELECTION: 'toggleAnnotationSelection';
+};
+
+export type BrideEvents =


Suggested change

export type BrideEvents =

export type BridgeEvents =

robertknight · 2021-10-12T08:29:00Z

src/types/bridge-events.d.ts

@@ -0,0 +1,150 @@
+/**


What effects does using a .d.ts extension have over using .ts? My understanding is that the main distinction is that .d.ts files can't contain any executable code.

Correct and because they can contain executable code are meant to define types.

I won't recommend using *.ts because this is a JavaScript project with types. If we introduce *.ts files we are giving the impression that it's a mix JavaScript-TypeScript project where you can choose in what language to write your code.

robertknight · 2021-10-12T08:31:56Z

src/sidebar/services/frame-sync.js

@@ -282,7 +278,7 @@ export class FrameSyncService {
  /**
   * Send an RPC message to the host frame.
   *
-   * @param {string} method
+   * @param {import('../../types/bridge-events').BrideEvents} method


Yeah, I think we do want to use the more specific type here to make it easier for the reader to see what the allowed values are.

robertknight · 2021-10-12T08:39:53Z

src/shared/bridge.js

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


This makes Bridge a little less general but on the other hand it will catch things like typos in method names. If we wanted an added layer of checking we could make the method names template parameter(s) of the class.

Misspelled `BridgeEvents`

Instead of using an enum, I use an union.

esanzgar · 2021-10-12T13:03:27Z

We have decided that event thought JavaScript enum are a little bit more verbose, they provide a level of indirection that is useful for documentation.

I will close this and replace it for another PR.

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

This is the event that creates the annotation, so it is better to name it in a more obvious way. Context: #3829 (comment)

esanzgar added 2 commits October 11, 2021 16:17

Order imports alphabetically

429048a

According to our conventions.

esanzgar force-pushed the types-for-bridge-events branch from f27cef3 to 429048a Compare October 11, 2021 14:17

esanzgar commented Oct 11, 2021

View reviewed changes

robertknight reviewed Oct 12, 2021

View reviewed changes

esanzgar added 3 commits October 12, 2021 13:20

Fix typo

d91b8d5

Misspelled `BridgeEvents`

Change name of the types to be singular

f86dfbf

Simplification of the types

adaf1f7

Instead of using an enum, I use an union.

esanzgar closed this Oct 12, 2021

esanzgar deleted the types-for-bridge-events branch October 12, 2021 13:03

esanzgar mentioned this pull request Oct 14, 2021

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

Closed

esanzgar added a commit that referenced this pull request Oct 14, 2021

Rename beforeCreateAnnotation to createAnnotation

957b44f

This is the event that creates the annotation, so it is better to name it in a more obvious way. Context: #3829 (comment)

esanzgar added a commit that referenced this pull request Oct 15, 2021

Rename beforeCreateAnnotation to createAnnotation

b047eed

This is the event that creates the annotation, so it is better to name it in a more obvious way. Context: #3829 (comment)

esanzgar added a commit that referenced this pull request Oct 15, 2021

Rename beforeCreateAnnotation to createAnnotation

e2e347d

This is the event that creates the annotation, so it is better to name it in a more obvious way. Context: #3829 (comment)

esanzgar added a commit that referenced this pull request Oct 15, 2021

Rename beforeCreateAnnotation to createAnnotation

750b30f

This is the event that creates the annotation, so it is better to name it in a more obvious way. Context: #3829 (comment)

esanzgar added a commit that referenced this pull request Oct 19, 2021

Rename beforeCreateAnnotation to createAnnotation

a6607d9

This is the event that creates the annotation, so it is better to name it in a more obvious way. Context: #3829 (comment)

esanzgar added a commit that referenced this pull request Oct 19, 2021

Rename beforeCreateAnnotation to createAnnotation

05d47ef

This is the event that creates the annotation, so it is better to name it in a more obvious way. Context: #3829 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Types for bridge events #3829

Types for bridge events #3829

esanzgar commented Oct 11, 2021

codecov bot commented Oct 11, 2021 •

edited

Loading

esanzgar Oct 11, 2021 •

edited

Loading

robertknight Oct 12, 2021

esanzgar Oct 11, 2021 •

edited

Loading

robertknight Oct 12, 2021

esanzgar Oct 11, 2021

esanzgar Oct 11, 2021

robertknight Oct 12, 2021

robertknight left a comment

robertknight Oct 12, 2021

esanzgar Oct 12, 2021

robertknight Oct 12, 2021

esanzgar Oct 12, 2021

robertknight Oct 12, 2021

robertknight Oct 12, 2021

robertknight Oct 12, 2021

robertknight Oct 12, 2021

esanzgar Oct 12, 2021

esanzgar Oct 12, 2021

robertknight Oct 12, 2021

robertknight Oct 12, 2021

esanzgar commented Oct 12, 2021

Types for bridge events #3829

Types for bridge events #3829

Conversation

esanzgar commented Oct 11, 2021

codecov bot commented Oct 11, 2021 • edited Loading

Codecov Report

esanzgar Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esanzgar Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esanzgar commented Oct 12, 2021

codecov bot commented Oct 11, 2021 •

edited

Loading

esanzgar Oct 11, 2021 •

edited

Loading

esanzgar Oct 11, 2021 •

edited

Loading