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

Enable MessageChannel for guest <--> sidebar communication #3534

Closed

Conversation

esanzgar
Copy link
Contributor

No description provided.

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.

Hi Eduardo. While I know that you're still working on this, I've had a read through the current prototype and left some comments.

I have some high-level thoughts:

  • The channel setup protocol needs to work in a way that doesn't require the sidebar to request its port for a channel, except for the host <-> sidebar channel. This is because the sidebar doesn't know when the notebook will be opened or how many guests there will be. Instead the host should just send the sidebar its port when a new guest/notebook <-> sidebar channel is created. For this to work, the host needs to know when the sidebar is ready to receive ports. Fortunately the host can already know this because the sidebar requests its port for the host <-> sidebar channel when ready.
  • If the above change is made, then the "request" messages can be simplified because they only need to specify the role (guest, sidebar, host, notebook) of the frame requesting the channel. Everything else can be determined by the host [1], assuming that all connections are between the single sidebar and a non-sidebar frame.
  • Any code that is used by both src/annotator and src/sidebar in the client needs to live in src/shared. This is to make it easier to understand which environments a particular module needs to operate in, as well as for general module dependency hygiene.
  • The changes to make frame-rpc work with MessagePort seems like something that is unlikely to be affected by higher-level changes, so you could move ahead with creating a PR to make that change as a first step to getting this into production.

[1] If the sender is the sidebar, they must be requesting a connection to the host. If the sender is not the sidebar, they must be requesting a connection to the sidebar.

src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/util/listener-collection.js Show resolved Hide resolved
src/sidebar/components/NotebookView.js Outdated Show resolved Hide resolved
src/sidebar/components/NotebookView.js Show resolved Hide resolved
src/sidebar/components/SidebarView.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
@esanzgar
Copy link
Contributor Author

esanzgar commented Jun 23, 2021

This is because the sidebar doesn't know when the notebook will be opened.

A very nice features of the MessageChannel API is that messages are queued until the receiving port is ready. This means that no message is lost and that you don't have to wait until the other port has been received and is ready to send messages.

Here is an example:

const {port1, port2} = new MessageChannel();

// Message can be sent immediately
port1.postMessage('Hello from port1');
port2.postMessage('Hello from port2');

port1.onmessage = ({data}) => console.log(data);
VM408:1 Hello from port2

port2.onmessage = ({data}) => console.log(data);
VM434:1 Hello from port1

port1.onmessage = ({data}) => console.log(data); is equivalent to these two statements together:

port1.addEventListener('message', ({data}) => console.log(data));
port1.start()

So the above is similar to this:

const {port1, port2} = new MessageChannel();

// Message can be sent immediately
port1.postMessage('Hello from port1');
port2.postMessage('Hello from port2');

port1.addEventListener('message', ({data}) => console.log(data));
port2.addEventListener('message', ({data}) => console.log(data));

// Messages are queued until the port is started
port1.start()
VM408:1 Hello from port2
port2.start()
VM434:1 Hello from port1

What this means in practice is that it doesn't matter if sidebar loads before or after notebook frame, all messages will be received.

However, I agree with you, that once the host <-> sidebar communication channel has been established, all other ports required by other channels should/could be sent through the above channel (host <-> sidebar).

Copy link
Contributor Author

@esanzgar esanzgar left a comment

Choose a reason for hiding this comment

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

Thank you @robertknight for all your thoughtful comments and suggestions.

I have made several modifications and added a few responses.

src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
src/annotator/communicator.js Outdated Show resolved Hide resolved
/**
* guest <-> sidebar TODO
* polling necessary because the `guest` frame could be loaded before the `host` frame
* @typedef {{channel: 'guestToSidebarChannel', hostFrame: Window, port: 'guest', subFrameIdentifier: string}} options0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree about dropping subFrameIdentifier (point 3).

I understand that if we rework the protocol of handling ports (point 2), we could get ride of the channel option and use the port option with three possible values: sidebar, notebook and guest. However, in the future there could be situation when just specifying the name of the requested port could be ambiguous. For example, maybe in the future we want to connect notebook with the sidebar and the host frames. In that case, if notebook frame request the notebook port it is impossible to know if it is requesting the port from notebookToSidebar channel or notebookToHost channel. Although it is a little bit more verbose to have both channel and port options, I believe is more explicit.

Regarding the hostFrame, this is the frame/window that is used to send the hostFrame.postMessage. In our current use cases, this is always window.parent. However, in the future we may allow a situation where a guest frame could be a descendant of host frame (a bit similar to the LMS case), instead of just a direct child.

@robertknight
Copy link
Member

What this means in practice is that it doesn't matter if sidebar loads before or after notebook frame, all messages will be received.

Ah, I follow. That's useful. OK, well this simplifies things for us.

esanzgar added 10 commits June 28, 2021 12:23
On this prototype, we start using `FrameConnector` on the `host` frame
and `PortFinder` on `SidebarView` and `NotebookView` components (on the
`sidebar` and `notebook` iframes, respectively)

`FrameConnector` creates `MessageChannel` for the communication between
two frames.

 There are 4 types of frames:
 - `host`:  frame where the client is initially loaded
 - `sidebar`: the main hypothesis app. It runs on an iframe and is responsible
    for communicating with the backend, fetching and saving the annotations.
 - `notebook`: it is an another hypothesis app that runs on an iframe.
 - `guest/s`: iframes with annotatable content

This layout represents the current arrangement of frames:

```
`host` frame
|-> `sidebar` iframe
|-> `notebook` iframe
|-> [`guest` iframe/s]
    |-> [`guest` iframe/s]
```

`FrameConnector` runs only on the `host` frame. The rest of the frames run the
companion class, `PortFinder`. `FrameConnector` creates a `MessageChannels`
for two frames to communicate with each other. It also listens to requests for
particular channel.port and dispatches the corresponding `MessagePort`.

```
                FrameConnector                      |                PortFinder
----------------------------------------------------|--------------------------------------------------------------

2. listens to `requests` of channel.port <--------------------- 1. `request` a channel.port using `host.postMessage`
                 |
                 V
3. sends `offers` of channel.port using `frame.postMessage` ---> 4. listens to `offers` of channel.port

```

 It is essential that `FrameConnect` initialize the listeners (step 2)
 before `PortFinder` sends the `requests` of channel.port (step 1).
 Because the `host` frame creates the `sidebar` and `notebook` iframes,
 it is guaranteed that `host` frame is ready to listen to messages
 originating from the `sidebar` and `notebook` iframes.

However, the above assumption is not true for `host` and `guest` frames.
Because `guest` iframes load in parallel, we can not assume that the
code in the `host` frame is executed before the code in a `guest` frame.
Therefore, for the `guest` frames we implement a polling strategy
(sending a message every N milliseconds until a response is received).

Next step would be to extend the prototype to connect other frames:
 - `host` <-> `sidebar`
 - `guest` <-> `sidebar`
The `notebook` iframe was re-rendered each time the notebook was opened.
With the addition of `sidebar`  <-> `notebook` communication this is no
longer required.
In addition, I removed `Channel` suffix in the channel type. The
`Channel` suffix in the channel types was redundant. In all use cases,
the channels are associated with the `channel` property.
This involves to modify the `cross-frame` service.
This involves to modify the `frame-sync` service.
In the JS community there is an aggreement that the underscore symbol
before a variable name denotes that the variable is private (this
consensus predate the [private class fields](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields).

The underscore after the variable name is a way to not override the
global variable. But it looks ugly accessing fiels: `window_.[xyz]`

It this particular case, I think it is fine to replace `window_` by
`_window`.
@esanzgar esanzgar force-pushed the messagechannel-guest-sidebar branch from 3d37b74 to efa02b9 Compare June 29, 2021 10:53
@esanzgar esanzgar linked an issue Jun 29, 2021 that may be closed by this pull request
@esanzgar esanzgar changed the base branch from master to messagechannel-host-sidebar June 29, 2021 13:06
@esanzgar esanzgar linked an issue Jun 29, 2021 that may be closed by this pull request
@esanzgar esanzgar force-pushed the messagechannel-guest-sidebar branch from be14b1a to 4f24e7c Compare June 30, 2021 11:03
@esanzgar esanzgar closed this Jul 26, 2021
@esanzgar esanzgar deleted the messagechannel-guest-sidebar 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.

Create a working prototype for guest <-> sidebar communication
2 participants