Support MessageChannel in `bridge.js` #3566

esanzgar · 2021-07-06T16:00:20Z

Initially, I tried to if/else portions of the code to accommodate for
MessagePort, aiming to avoid duplication of the code (like I did for
shared/frame-rpc.js #3565). However, I found that, unlike
shared/frame-rpc.js, this resulted into a spaghetti-type of code, not
very understandable.

Then, I followed @robertknight suggestion:

Package up the data needed to create a channel for a window into one object, then make createChannel accept either that object or a MessagePort, and only have createChannel as the public API
Have two separate public methods, createChannelForWindow and createChannelForPort, which work with a Window and MessagePort respectively. Internally these could delegate to a shared helper to do most of the work

The two internal methods to support the current communication using Window and the new MessagePort, leads to clearer results, although some code is duplicated in both methods.

This PR will result on a reduction in code coverage, which will be fix
by #3598. (Edit: Additional unit tests have been added in this PR, so this comment no longer applies - @robertknight)

codecov · 2021-07-06T16:08:53Z

Codecov Report

Merging #3566 (a3c2040) into master (cb38a10) will decrease coverage by 0.02%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master    #3566      +/-   ##
==========================================
- Coverage   98.62%   98.60%   -0.03%     
==========================================
  Files         211      211              
  Lines        7723     7738      +15     
  Branches     1756     1758       +2     
==========================================
+ Hits         7617     7630      +13     
- Misses        106      108       +2

Impacted Files	Coverage Δ
src/sidebar/services/frame-sync.js	`98.96% <50.00%> (-1.04%)`	⬇️
src/shared/bridge.js	`97.36% <94.44%> (-1.00%)`	⬇️
src/annotator/cross-frame.js	`100.00% <100.00%> (ø)`

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 cb38a10...a3c2040. Read the comment docs.

robertknight

I've had a first look through this and left some feedback. Here are my high-level thoughts:

The code modernization/simplification and JSDoc improvements in shared/bridge.js and annotator/cross-frame.js looks great. It might be a good idea to split those changes off into a separate smaller PR which can be landed first.
The Bridge class provides two separate ways to create a channel that works with a MessagePort and there is some duplication between the code paths create a channel for a Window vs a MessagePort. I would prefer that there is only one public API to create a Bridge for a MessagePort and to avoid the duplication, since the only thing that changes depending on the message source is the RPC constructor arguments.
I suggested to omit the frame-type argument to createChannelFromPort for the moment, because when the time comes that we want to target messages at specific frames, we might want to revisit the use of Bridge altogether. The main function of Bridge is to send messages to multiple frames, so perhaps we don't need it at all if we're targeting a specific frame.
The existing Bridge tests are unit tests, although they don't use $imports.$mock like we do in more recent code, but the new ones are integration tests. Mixing and matching unit and integration tests in the same module creates confusion for subsequent developers because it becomes unclear which approach they should use for new tests. While I think we will want some integration tests for the inter-frame communication, the RPC interface is simple enough that I think we can mock it in bridge-test.js, but using $imports.$mock. It might be a good idea to create a separate PR that modernizes the mocking of RPC in bridge-test.js before making the functional change to support MessagePort.

robertknight · 2021-07-07T18:52:03Z

src/shared/bridge.js

@@ -153,6 +189,9 @@ export default class Bridge {
  /**
   * Unregister any callbacks registered with `on`.
   *
+   * Attention: for this to have the intended effect, it needs to be called
+   * before `createChannel`, because at that point the methods are registered
+   * with the `RPC` class and there is no way to remove the listeners.
   * @param {string} method
   */
  off(method) {


It doesn't look like we ever call this method anywhere. On that basis we could just remove it and the associated tests. This would remove the need for the caveat above.

I agree 100%.

I also would suggest to rename on(...) to register(...). It makes clear that you have to:

register the method

create the channel

call the method

robertknight · 2021-07-08T06:43:29Z

src/annotator/cross-frame.js

@@ -89,7 +89,7 @@ export class CrossFrame {
     * Subscribe to an event from the sidebar.
     *
     * @param {string} event
-     * @param {Function} callback
+     * @param {(...args: any[]) => void} callback


These functions can pass a result callback as the last argument. Unfortunately I don't think there is a way to specify that at present (see microsoft/TypeScript#1360). This will have to do for now.

I took a look too, but iI didn't find an easy way to add types to the arguments and callback.

However, an approach that in my opinion is better is to change the signature of the call method in FrameRPC and CrossFrame to allow for an object:

@@ -110,17 +110,18 @@ export class RPC { * If the final argument in `args` is a function, it is treated as a callback * which is invoked with the response. * - * @param {string} method - * @param {any[]} args + * @param {object} options + * @param {string} options.method + * @param {any[]} options.args + * @param {(...args: any[]) => void} [options.callback] */ - call(method, ...args) { + call({ method, args, callback }) { if (this._destroyed) { return; } const seq = this._sequence++; - if (typeof args[args.length - 1] === 'function') { - this._callbacks[seq] = args[args.length - 1]; - args = args.slice(0, -1); + if (callback) { + this._callbacks[seq] = callback; } this.destFrame.postMessage( {

If would make the use of it a bit more verbose, but also more explicit:

- bridge.call('sumMethod', 1, 2, 3, total => ....)` + bridge.call({method: 'sumMethod', args: [1, 2, 3], callback: total => ....})`

Alternatively, this other variant may require a little bit less work refactoring old code:

- bridge.call('sumMethod', 1, 2, 3, total => ....)` + bridge.call('sumMethod', {args: [1, 2, 3], callback: total => ....})`

robertknight · 2021-07-08T06:43:44Z

src/shared/bridge.js

@@ -10,8 +10,11 @@ import { RPC } from './frame-rpc';
 */
 export default class Bridge {
  constructor() {
+    /** @type {Array<RPC>} */


Suggested change

/** @type {Array<RPC>} */

/** @type {RPC[]} */

I was debating about that, but I thought Array was more consistent with the onConnectListeners's type two lines after:

/** @type {Array<(...args: any[]) => void>} */ this.onConnectListeners = [];

robertknight · 2021-07-08T06:45:11Z

src/shared/bridge.js

@@ -21,7 +24,7 @@ export default class Bridge {
   * This removes the event listeners for messages arriving from other windows.
   */
  destroy() {
-    Array.from(this.links).map(link => link.channel.destroy());
+    this.links.forEach(channel => channel.destroy());


The Array.from usage here was probably a hangover from semi-automated CoffeeScript => JS conversion. The decaffeinate tool that we used for some of this conversion added Array.from in some places to be conservative.

I makes sense.

robertknight · 2021-07-08T06:46:27Z

src/shared/bridge.js

+  }
+
+  /**
+   * Create a communication channel using `MessageChannel.MessagePort`.


Suggested change

* Create a communication channel using `MessageChannel.MessagePort`.

* Create a communication channel using a `MessagePort`

The previous wording suggests to me that MessagePort is a property of MessageChannel, but it is its own global.

robertknight · 2021-07-08T07:29:41Z

src/shared/test/bridge-test.js

+          });
+          clock.tick(1000);
+        } finally {
+          clock.restore();


As this test is testing what happens in the event of a timeout I expected to see an error occur. I had to go back and look at the Bridge code to see that a null result indicates that a timeout occurred. This is surprising API design IMO, although I can see it is not new behavior on this branch.

I agree with you, it is surprising. Like you, I asked myself why not to reject the promise instead of returning a [null]. Rejecting the promise causes the channel to be closed, which is also surprising.

robertknight · 2021-07-08T07:31:04Z

src/shared/test/bridge-test.js

+        try {
+          await reciprocalBridge.call('method1', 'params1');
+        } catch (err) {
+          assert.equal(err, error);
+        }


Suggested change

try {

await reciprocalBridge.call('method1', 'params1');

} catch (err) {

assert.equal(err, error);

}

let err;

try {

await reciprocalBridge.call('method1', 'params1');

} catch (err) {

err = e;

}

assert.equal(err, <expected error>)

If the assert is inside the catch then the test can incorrectly pass if the code fails to throw an error.

Good point.

robertknight · 2021-07-08T07:33:04Z

src/shared/test/bridge-test.js

+        try {
+          reciprocalBridge.destroy();
+
+          // It timeouts using bridge


This seems like a rather roundabout way of testing that the channels were destroyed. The existing #destroy test just checks that the destroy method was called on the channel.

Yes, it is a roundabout, but it is exactly what a user of the Bridge API will expect, a timeout after 1s resulting on a [null] to be returned.

robertknight · 2021-07-08T08:01:21Z

src/shared/test/bridge-test.js

@@ -10,246 +10,476 @@ describe('shared/bridge', () => {
  beforeEach(() => {


The existing Bridge tests are unit tests that stub RPC.call. The new tests are integration tests that end up testing a lot of RPC-specific details. We generally try to avoid mixing unit and integration tests in the same module, as becomes unclear for developers making changes later which approach they should follow when adding a new test. Sometimes there might be cases where there is justification to have a test that deviates from the other tests in terms of its unit-ness. In that case it is good to call out the fact that these are an exception. For example with a specific "integration tests" describe block.

At the time when the Bridge tests were originally written I don't think we had a good mocking tool ($imports.$mock) which is probably why they create a real RPC and then stub call each time. This could be simplified to use $imports.$mock now. eg:

// Active channels let channels = []; class FakeRPC { constructor(sourceFrame, ...) { this.sourceFrame = sourceFrame; ... this.call = sinon.stub().resolves(...) channels.push(this); } destroy() { channels.splice(channels.indexOf(this), 1); } } ... $imports.$mock({ './frame-rpc': { RPC: FakeRPC }})

I think we are eventually going to want some integration tests for inter-frame messaging. Following the examples of src/{annotator, sidebar}/test/integration these could go in src/shared/test/integration/messaging-test.js.

It is good to know about the integration tests.

Until this #3565, FrameRPC didn't have unit testing. I think the Bridge was supposed to unit test FrameRPC, however, because of the difficulty of creating two frames to communicate with each other, most of the methods in FrameRPC were mocked.

robertknight · 2021-07-08T08:09:27Z

src/shared/bridge.js

   * @param {string} origin - The origin of the document in `source`.
   * @param {string} token
   * @return {RPC} - Channel for communicating with the window.
+   *
+   * @deprecated
   */
  createChannel(source, origin, token) {


There are currently two different public APIs for creating a channel from a MessagePort - calling createChannel or calling createChannelFromPort directly. This seems redundant to me. I can suggest a few different ways to rectify this:

Package up the data needed to create a channel for a window into one object, then make createChannel accept either that object or a MessagePort, and only have createChannel as the public API

Have two separate public methods, createChannelForWindow and createChannelForPort, which work with a Window and MessagePort respectively. Internally these could delegate to a shared helper to do most of the work

Yes, I can do something along those lines. The two implementations are 95% similar, that's why I thought it was OK to have both of them, since one of them is marked @deprecated and will be removed.

Initially, I tried to if/else portions of the code to accommodate for `MessagePort`, aiming to avoid duplication of the code (like I did for `shared/frame-rpc.js` #3565). However, I found that, unlike `shared/frame-rpc.js`, this resulted into a spaghetti-type of code, not very understandable. Then, I decided to create two internal methods to support both the current communication using `Window` and the new `MessagePort`. This in my opinion leads to a clearer results, although some code is duplicated in both methods. This PR will result on a reduction in code coverage, which will be fix by #3590.

The need for this "testing only" code can be removed by improving the way that we simulate the `connect` message being handled in tests.

This argument was not used by any callers of `Bridge#onConnect` and removing it eliminates a difference between creating a channel that uses `Window.postMessage` vs one that uses `MessagePort`.

robertknight

The revised PR looks better. The main feedback I have is regarding this comment:

This PR will result on a reduction in code coverage, which will be fix by #3598.

PR #3598 adds an integration test, but I don't see that as a reason not to have unit tests as well. Put another way, whether you run the integration tests or not shouldn't affect code coverage for most modules, with some rare exceptions.

Since you're away for the rest of this week, I've gone ahead and pushed a few additional unit tests as well as a couple of other minor changes based on my comments here. I will get this landed first thing tomorrow.

robertknight · 2021-07-20T12:36:57Z

src/shared/bridge.js

@@ -27,18 +27,83 @@ export default class Bridge {
    this.links.forEach(channel => channel.destroy());
  }

+  /**
+   * Deprecated - Remove after MessagePort conversion
+   * @typedef windowOptions


Suggested change

* @typedef windowOptions

* @typedef WindowOptions

Type names should use PascalCase. I'd probably have gone for something without an Options suffix since that tends to imply optional parameters. In this case the object is really a combination of a Window + origin. Maybe something like WindowTarget (as in fields needed to send messages to a particular window target)

I pushed a commit that keeps the name but just converts it to PascalCase.

robertknight · 2021-07-20T12:41:04Z

src/shared/bridge.js

+  /**
+   * Deprecated - Remove after MessagePort conversion
+   * @typedef windowOptions
+   * @prop {Window} source - The source window


I think using the term source is potentially a little confusing here because this window is both a source of messages that trigger on callbacks and a target of messages sent with call. Also when the RPC channel is constructed, the source is passed as the destFrameOrPort argument.

Naming this target would be misleading for the same reason and naming it window could cause it to be confused with the window global. So perhaps just revising the documentation to something like @prop {Window} source - The window to communicate with would be enough for now.

robertknight · 2021-07-20T12:46:39Z

src/shared/bridge.js

+   */
+  createChannel(options) {
+    if (options instanceof MessagePort) {
+      return this._createChannelForPort(options);


This PR will result on a reduction in code coverage, which will be fix by #3598.

PR #3566 adds integration tests, but I don't see a reason not to add unit tests here.

I pushed a commit that adds a few extra tests to cover the new code paths.

robertknight · 2021-07-20T14:59:01Z

src/shared/bridge.js

@@ -53,7 +118,7 @@ export default class Bridge {
    const connect = (_token, cb) => {
      if (_token === token) {
        cb();
-        ready();
+        ready(); // This is necessary for testing only.


This raised eyebrows. Why do we have testing-only code in this callback? Since there is a callback being invoked here it seems to me that the tests should try to leverage that.

I pushed a commit that reworks the tests to avoid the need for the ready() callback here. This means that the MessagePort and Window code paths are now the same except that the Window code path checks the token before invoking the callback.

I left the code as it was originally (since it will be removed anyway), but I added the comment, because like you said, it didn't look right.

robertknight · 2021-07-20T14:59:27Z

src/shared/bridge.js

@@ -162,7 +227,7 @@ export default class Bridge {
  /**
   * Add a listener to be called upon a new connection.
   *
-   * @param {(channel: RPC, window: Window) => void} listener
+   * @param {(channel: RPC, window?: Window) => void} listener


Looking at the callers of onConnect, none of them make use of the window argument, so we can just drop it.

The Guest class invokes onConnect indirectly (via CrossFrame) with no arguments

The FrameSync service invokes onConnect but only uses the first argument

I pushed a commit that removes the window argument.

esanzgar mentioned this pull request Jul 6, 2021

Epic: overhaul the inter-frame communication system #3533

Closed

13 tasks

robertknight requested changes Jul 8, 2021

View reviewed changes

esanzgar force-pushed the support-messagechannel-frame-rpc branch 3 times, most recently from f6ae496 to a6bb101 Compare July 14, 2021 17:27

Base automatically changed from support-messagechannel-frame-rpc to master July 14, 2021 17:56

esanzgar force-pushed the support-messagechannel-bridge branch 2 times, most recently from 8886f60 to 32dbb68 Compare July 19, 2021 16:48

esanzgar linked an issue Jul 19, 2021 that may be closed by this pull request

Support MessageChannel in bridge.js #3581

Closed

esanzgar requested a review from robertknight July 19, 2021 17:26

esanzgar force-pushed the support-messagechannel-bridge branch from 32dbb68 to 6dad72c Compare July 20, 2021 07:53

robertknight added 2 commits July 20, 2021 14:51

Add unit tests for calling Bridge#createChannel with a MessagePort

b30ed20

Remove the need for some test-only code in Bridge

5e45547

The need for this "testing only" code can be removed by improving the way that we simulate the `connect` message being handled in tests.

robertknight self-assigned this Jul 20, 2021

robertknight added 2 commits July 20, 2021 16:45

Use PascalCase naming for types

a0302db

Remove unused Window argument to onConnect callback

a3c2040

This argument was not used by any callers of `Bridge#onConnect` and removing it eliminates a difference between creating a channel that uses `Window.postMessage` vs one that uses `MessagePort`.

robertknight approved these changes Jul 20, 2021

View reviewed changes

robertknight merged commit 42369e6 into master Jul 21, 2021

robertknight deleted the support-messagechannel-bridge branch July 21, 2021 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support MessageChannel in `bridge.js` #3566

Support MessageChannel in `bridge.js` #3566

esanzgar commented Jul 6, 2021 •

edited by robertknight

Loading

codecov bot commented Jul 6, 2021 •

edited

Loading

robertknight left a comment

robertknight Jul 7, 2021

esanzgar Jul 14, 2021

robertknight Jul 8, 2021

esanzgar Jul 14, 2021 •

edited

Loading

robertknight Jul 8, 2021

esanzgar Jul 14, 2021 •

edited

Loading

robertknight Jul 8, 2021

esanzgar Jul 14, 2021

robertknight Jul 8, 2021

robertknight Jul 8, 2021

esanzgar Jul 14, 2021

robertknight Jul 8, 2021

esanzgar Jul 14, 2021

robertknight Jul 8, 2021

esanzgar Jul 14, 2021

robertknight Jul 8, 2021

esanzgar Jul 14, 2021

esanzgar Jul 14, 2021 •

edited

Loading

robertknight Jul 8, 2021

esanzgar Jul 14, 2021

robertknight left a comment

robertknight Jul 20, 2021

robertknight Jul 20, 2021

robertknight Jul 20, 2021

robertknight Jul 20, 2021

robertknight Jul 20, 2021

robertknight Jul 20, 2021

robertknight Jul 20, 2021

esanzgar Jul 26, 2021

robertknight Jul 20, 2021

robertknight Jul 20, 2021

esanzgar Jul 26, 2021

	* Create a communication channel using `MessageChannel.MessagePort`.
	* Create a communication channel using a `MessagePort`

		@@ -10,246 +10,476 @@ describe('shared/bridge', () => {
		beforeEach(() => {

Support MessageChannel in bridge.js #3566

Support MessageChannel in bridge.js #3566

Conversation

esanzgar commented Jul 6, 2021 • edited by robertknight Loading

codecov bot commented Jul 6, 2021 • edited Loading

Codecov Report

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

esanzgar Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esanzgar Jul 14, 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

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 Jul 14, 2021 • edited Loading

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

Support MessageChannel in `bridge.js` #3566

Support MessageChannel in `bridge.js` #3566

esanzgar commented Jul 6, 2021 •

edited by robertknight

Loading

codecov bot commented Jul 6, 2021 •

edited

Loading

esanzgar Jul 14, 2021 •

edited

Loading

esanzgar Jul 14, 2021 •

edited

Loading

esanzgar Jul 14, 2021 •

edited

Loading