Support MessageChannel in `frame-rpc.js` #3565

esanzgar · 2021-07-05T17:08:54Z

added support for MessageChannel in the RPC class. I also simplified some internals.
bridge-test.js refactor to use arrow functions
added frame-rpc-test.js to test specifically the RPC class with MessageChannel.

LMS007

Eduardo, I don't have too much feedback on the overall direction. It does look reasonable to me, but you have a much better handle on this than I do. I think about the only details I can comment on here are adding fake timers for the tests to speed them up, and then getting codecov to pass 100%.

src/shared/frame-rpc.js

src/shared/test/frame-rpc-test.js

robertknight

The general approach of using if/else to guard the new/old branches is reasonable since there are only two cases. I thought it might be useful to document alternative strategies that could have been used, so I posted a comment that showed an alternative pattern using an adapter. This would isolate all the deprecated logic into one place. I also included a more lightweight change compared to this PR which just wraps up all the window.postMessage-related fields in one object so it is obvious that they should all be removed together in future. Since we don't plan to keep this code around for long, it isn't essential to change this PR to use either of them - it's just something to consider if a similar situation occurs in future.

There are some things that do need to be resolved though:

The _isValidSender method is not called, but tested for falsey-ness
The comment about MessageChannel being unidirectional is not correct. I think I know what you mean, but we should correct it.
@deprecated should only be used for symbols (fields, methods). For blocks of code or branches just use ordinary comments. Similarly /** comments should only be used to document symbols (functions, classes, fields etc.). Use // or /* ... */ for other comments.
The frame-rpc-test.js module has the wrong name in the top-level describe block

src/shared/frame-rpc.js

src/shared/test/frame-rpc-test.js

src/shared/frame-rpc.js

robertknight · 2021-07-07T18:28:27Z

src/shared/frame-rpc.js

-  constructor(sourceFrame, destFrame, origin, methods) {
-    this.sourceFrame = sourceFrame;
-    this.destFrame = destFrame;
+  constructor(sourceFrame, destFrameOrPort, origin, methods) {


Since we're only switching from one implementation to another, if/else statements are a reasonable way to go. An alternative migration strategy might have been to wrap the existing window.postMessage logic up in a MessagePort-like interface adapter. This would isolate all the legacy window.postMessage logic in one place.

An example:

diff --git a/src/shared/bridge.js b/src/shared/bridge.js index 195ff80e5..a60a49715 100644 --- a/src/shared/bridge.js +++ b/src/shared/bridge.js @@ -1,4 +1,4 @@ -import { RPC } from './frame-rpc'; +import { RPC, WindowPort } from './frame-rpc'; /** @typedef {import('../types/annotator').Destroyable} Destroyable */ @@ -59,7 +59,8 @@ export default class Bridge { const listeners = { connect, ...this.channelListeners }; // Set up a channel - channel = new RPC(window, source, origin, listeners); + const port = new WindowPort(window, source, origin); + channel = new RPC(port, listeners); // Fire off a connection attempt channel.call('connect', token, ready); diff --git a/src/shared/frame-rpc.js b/src/shared/frame-rpc.js index 72780ab2a..1314215fb 100644 --- a/src/shared/frame-rpc.js +++ b/src/shared/frame-rpc.js @@ -1,5 +1,3 @@ -import { ListenerCollection } from '../annotator/util/listener-collection'; - /* This module was adapted from `index.js` in https://github.com/substack/frame-rpc. @@ -50,6 +48,53 @@ const PROTOCOL = 'frame-rpc'; * @typedef {import('../types/annotator').Destroyable} Destroyable */ +/** + * Implementation of the `MessagePort` interface that uses `window.postMessage`. + * + * This exists to allow the `RPC` class to be migrated from working with + * `window.postMessage` to `MessagePort`. + */ +export class WindowPort { + /** + * @param {Window} sourceWindow + * @param {Window} destWindow + * @param {string} origin + */ + constructor(sourceWindow, destWindow, origin) { + this.sourceWindow = sourceWindow; + this.destWindow = destWindow; + this.origin = new URL(origin).origin; + + this._listener = null; + this._handleMessage = event => { + if ( + (origin !== '*' && event.origin !== origin) || + event.source !== destWindow + ) { + return; + } + this._listener?.(event); + }; + this.sourceWindow.addEventListener('message', this._handleMessage); + } + + /** @param {(e: MessageEvent) => void} handler */ + set onmessage(handler) { + this._listener = handler; + } + + close() { + this.sourceWindow.removeEventListener('message', this._handleMessage); + } + + /** + * @param {any} message + */ + postMessage(message) { + this.destWindow.postMessage(message, this.origin); + } +} + /** * Class for making RPC requests between two frames. * @@ -72,28 +117,12 @@ export class RPC { * be able to cleanup this class. We have added `@deprecated` statements in * the pieces of code that need to be removed. * - * @param {Window} sourceFrame -- @deprecated - * @param {Window|MessagePort} destFrameOrPort - * @param {string} origin - Origin of destination frame @deprecated + * @param {WindowPort|MessagePort} port * @param {Record<string, (...args: any[]) => void>} methods - Map of method * name to method handler */ - constructor(sourceFrame, destFrameOrPort, origin, methods) { - this.sourceFrame = sourceFrame; // sourceFrame is ignored if using MessagePort - - if (destFrameOrPort instanceof MessagePort) { - this._port = destFrameOrPort; - } else { - /** @deprecated */ - this.destFrame = destFrameOrPort; - } - - /** @deprecated */ - if (origin === '*') { - this.origin = '*'; - } else { - this.origin = new URL(origin).origin; - } + constructor(port, methods) { + this._port = port; this._methods = methods; @@ -101,26 +130,8 @@ export class RPC { this._callbacks = {}; this._destroyed = false; - this._listeners = new ListenerCollection(); - - if (this._port) { - this._listeners.add(this._port, 'message', event => - this._handle(/** @type {MessageEvent} */ (event)) - ); - this._port.start(); - } else { - /** @deprecated */ - /** @param {MessageEvent} event */ - const onmessage = event => { - if (!this._isValidSender) { - return; - } - this._handle(event); - }; - this._listeners.add(this.sourceFrame, 'message', event => - onmessage(/** @type {MessageEvent} */ (event)) - ); - } + this._handle = this._handle.bind(this); + this._port.onmessage = this._handle; } /** @@ -129,10 +140,7 @@ export class RPC { */ destroy() { this._destroyed = true; - this._listeners.removeAll(); - if (this._port) { - this._port.close(); - } + this._port.close(); } /** @@ -165,31 +173,7 @@ export class RPC { version: VERSION, }; - if (this._port) { - this._port.postMessage(message); - } - - /** @deprecated */ - if (this.destFrame) { - this.destFrame.postMessage(message, this.origin); - } - } - - /** - * Validate sender - * - * @param {MessageEvent} event - * @deprecated - */ - _isValidSender(event) { - if (this.destFrame !== event.source) { - return false; - } - if (this.origin !== '*' && event.origin !== this.origin) { - return false; - } - - return true; + this._port.postMessage(message); } /** @@ -239,14 +223,7 @@ export class RPC { version: VERSION, }; - if (this._port) { - this._port.postMessage(message); - } - - /** @deprecated */ - if (this.destFrame) { - this.destFrame.postMessage(message, this.origin); - } + this._port.postMessage(message); }; this._methods[msg.method].call(this._methods, ...msg.arguments, callback); } else if ('response' in msg) {

A more lightweight change compared to this PR might have been to bundle up the properties that related to the old communication method in one object, such that the _port field refers either to the MessagePort or the object containing the source/dest window and the origin. For example:

diff --git a/src/shared/bridge.js b/src/shared/bridge.js index 195ff80e5..78e6ba679 100644 --- a/src/shared/bridge.js +++ b/src/shared/bridge.js @@ -59,7 +59,7 @@ export default class Bridge { const listeners = { connect, ...this.channelListeners }; // Set up a channel - channel = new RPC(window, source, origin, listeners); + channel = new RPC({ source: window, dest: source, origin }, listeners); // Fire off a connection attempt channel.call('connect', token, ready); diff --git a/src/shared/frame-rpc.js b/src/shared/frame-rpc.js index 72780ab2a..469c83bff 100644 --- a/src/shared/frame-rpc.js +++ b/src/shared/frame-rpc.js @@ -50,6 +50,13 @@ const PROTOCOL = 'frame-rpc'; * @typedef {import('../types/annotator').Destroyable} Destroyable */ +/** + * @typedef MessageSource + * @prop {Window} dest + * @prop {Window} source + * @prop {string} origin + */ + /** * Class for making RPC requests between two frames. * @@ -72,27 +79,15 @@ export class RPC { * be able to cleanup this class. We have added `@deprecated` statements in * the pieces of code that need to be removed. * - * @param {Window} sourceFrame -- @deprecated - * @param {Window|MessagePort} destFrameOrPort - * @param {string} origin - Origin of destination frame @deprecated + * @param {MessageSource|MessagePort} port * @param {Record<string, (...args: any[]) => void>} methods - Map of method * name to method handler */ - constructor(sourceFrame, destFrameOrPort, origin, methods) { - this.sourceFrame = sourceFrame; // sourceFrame is ignored if using MessagePort + constructor(port, methods) { + this._port = port; - if (destFrameOrPort instanceof MessagePort) { - this._port = destFrameOrPort; - } else { - /** @deprecated */ - this.destFrame = destFrameOrPort; - } - - /** @deprecated */ - if (origin === '*') { - this.origin = '*'; - } else { - this.origin = new URL(origin).origin; + if (!(port instanceof MessagePort)) { + port.origin = new URL(port.origin).origin; } this._methods = methods; @@ -103,21 +98,24 @@ export class RPC { this._listeners = new ListenerCollection(); - if (this._port) { + if (this._port instanceof MessagePort) { this._listeners.add(this._port, 'message', event => this._handle(/** @type {MessageEvent} */ (event)) ); this._port.start(); } else { - /** @deprecated */ + const port = this._port; /** @param {MessageEvent} event */ const onmessage = event => { - if (!this._isValidSender) { + if (port.dest !== event.source) { + return; + } + if (port.origin !== '*' && event.origin !== port.origin) { return; } this._handle(event); }; - this._listeners.add(this.sourceFrame, 'message', event => + this._listeners.add(this._port.dest, 'message', event => onmessage(/** @type {MessageEvent} */ (event)) ); } @@ -130,7 +128,7 @@ export class RPC { destroy() { this._destroyed = true; this._listeners.removeAll(); - if (this._port) { + if (this._port instanceof MessagePort) { this._port.close(); } } @@ -165,31 +163,11 @@ export class RPC { version: VERSION, }; - if (this._port) { + if (this._port instanceof MessagePort) { this._port.postMessage(message); + } else { + this._port.dest.postMessage(message, this._port.origin); } - - /** @deprecated */ - if (this.destFrame) { - this.destFrame.postMessage(message, this.origin); - } - } - - /** - * Validate sender - * - * @param {MessageEvent} event - * @deprecated - */ - _isValidSender(event) { - if (this.destFrame !== event.source) { - return false; - } - if (this.origin !== '*' && event.origin !== this.origin) { - return false; - } - - return true; } /** @@ -239,13 +217,10 @@ export class RPC { version: VERSION, }; - if (this._port) { + if (this._port instanceof MessagePort) { this._port.postMessage(message); - } - - /** @deprecated */ - if (this.destFrame) { - this.destFrame.postMessage(message, this.origin); + } else { + this._port.dest.postMessage(message, this._port.origin); } }; this._methods[msg.method].call(this._methods, ...msg.arguments, callback); diff --git a/src/shared/test/frame-rpc-test.js b/src/shared/test/frame-rpc-test.js index 2d24ac10f..6765baafa 100644 --- a/src/shared/test/frame-rpc-test.js +++ b/src/shared/test/frame-rpc-test.js @@ -19,14 +19,9 @@ describe('shared/bridge', () => { callback(result); }; - rpc1 = new RPC( - /** dummy when using ports */ window, - port1, - /** dummy when using ports */ '*', - { - concat, - } - ); + rpc1 = new RPC(port1, { + concat, + }); // `plusOne` method for rpc2 plusOne = sinon.stub().callsFake((...numbers) => { @@ -35,14 +30,9 @@ describe('shared/bridge', () => { callback(result); }); - rpc2 = new RPC( - /** dummy when using ports */ window, - port2, - /** dummy when using ports */ '*', - { - plusOne, - } - ); + rpc2 = new RPC(port2, { + plusOne, + }); }); afterEach(() => {

Thank you very much for the recommendation. I did think about doing some similar to what you suggested.

I thought that removing the few branches that are marked as deprecated would have been easier.

An important aspect is that I want to support MessagePort in RPC and Bridge maintaining the support for Window. In case of unexpected issues I want to be able to rollback easily.

src/shared/test/frame-rpc-test.js

codecov · 2021-07-08T14:40:19Z

Codecov Report

Merging #3565 (f6ae496) into master (dc1e560) will decrease coverage by 0.05%.
The diff coverage is 70.90%.

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

@@            Coverage Diff             @@
##           master    #3565      +/-   ##
==========================================
- Coverage   98.68%   98.62%   -0.06%     
==========================================
  Files         211      211              
  Lines        7694     7724      +30     
  Branches     1749     1755       +6     
==========================================
+ Hits         7593     7618      +25     
- Misses        101      106       +5

Impacted Files	Coverage Δ
src/shared/frame-rpc.js	`79.48% <70.90%> (+0.32%)`	⬆️
src/shared/bridge.js	`98.43% <0.00%> (+1.56%)`	⬆️

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 dc1e560...a6bb101. Read the comment docs.

esanzgar

Thank you @robertknight and @LMS007 for you thoughtful comments.

I have acted on your feedback. You could see the last commit contains your suggestions.

src/shared/frame-rpc.js

src/shared/test/frame-rpc-test.js

esanzgar · 2021-07-08T14:38:10Z

src/shared/test/frame-rpc-test.js

+    port1.postMessage('dummy');
+
+    setTimeout(() => {
+      assert.calledOnce(plusOne);


I parametrised the test, but I found it difficult to display the message if the test failed.

esanzgar · 2021-07-08T14:58:50Z

src/shared/frame-rpc.js

-  constructor(sourceFrame, destFrame, origin, methods) {
-    this.sourceFrame = sourceFrame;
-    this.destFrame = destFrame;
+  constructor(sourceFrame, destFrameOrPort, origin, methods) {


Thank you very much for the recommendation. I did think about doing some similar to what you suggested.

I thought that removing the few branches that are marked as deprecated would have been easier.

An important aspect is that I want to support MessagePort in RPC and Bridge maintaining the support for Window. In case of unexpected issues I want to be able to rollback easily.

robertknight

Hi Eduardo,

I noticed that reason was unused in the parametrized test and I had a query about a comment above that:

I parametrised the test, but I found it difficult to display the message if the test failed.

I also had a suggestion about wrapping up the setTimeout(..., 0) logic used to await message delivery into a helper (eg. waitForMessageDelivery) which documents what the logic is for and why it works.

src/shared/frame-rpc.js

src/shared/test/bridge-test.js

src/shared/test/frame-rpc-test.js

robertknight · 2021-07-09T08:05:26Z

src/shared/test/frame-rpc-test.js

+    port1.postMessage('dummy');
+
+    setTimeout(() => {
+      assert.calledOnce(plusOne);


I parametrised the test, but I found it difficult to display the message if the test failed.

Which message are you referring to?

It is a bit of a Frankenstein. The plan is to clean up on the next commits.

It simplifies the code.

Created a couple of utility methods to avoid duplication of code and enable to decouple RPC for `Window` or for `MessagePort`.

RPC: * added `@deprecated` statements and additional comments * removed `this._destroyed` for the listener checks, because the listener is removed when called `destroy()`. * added `this._port.close()` in addition to removing the listner. `bridge-test`: * refactor all the `function () {}` to arrow functions `frame-rpc-test.js`: * added these tests to check the `MessageChannel` functionality.

robertknight

LGTM. I had some minor comments on the frame-rpc code. I found some issues with the way bridge tests mocked RPC and mixed unit/integration tests which we discussed in the front-end sync this afternoon. For this I pushed a commit with my suggested solution per the discussion.

robertknight · 2021-07-14T13:22:13Z

src/shared/frame-rpc.js

    if (origin === '*') {
      this.origin = '*';
    } else {
-      const url = new URL(origin);
-      this.origin = url.protocol + '//' + url.host;
+      this.origin = new URL(origin).origin;


👍 I would guess the .origin property was not used originally because it required newer browsers than were supported at the time. It's OK with our current browser support baseline.

src/shared/frame-rpc.js

robertknight · 2021-07-14T13:23:20Z

src/shared/frame-rpc.js

+    } else {
+      /**
+       * @param {MessageEvent} event
+       * @deprecated


The "deprecated" part refers to the whole else code block though, not just this internal helper.

robertknight · 2021-07-14T13:23:58Z

src/shared/frame-rpc.js

@@ -101,7 +131,10 @@ export class RPC {
   */
  destroy() {
    this._destroyed = true;
-    this.sourceFrame.removeEventListener('message', this._onmessage);
+    this._listeners.removeAll();
+    if (this._port) {


Could also use this._port?.close().

Indeed, changed.

robertknight · 2021-07-14T13:26:35Z

src/shared/frame-rpc.js

+    if (!data || typeof data !== 'object') {
+      return null;
+    }
+    if (data.protocol !== PROTOCOL) {


This if statement and the two below could be combined into one.

Originally, all these statements were all combined into one. However, I have split into individuals if statements because it is more readable for developers and easier to interpret in the code coverage.

I can combined them back, if you wish it.

easier to check code coverage.

That's true. OK, let's leave as is.

robertknight · 2021-07-14T13:26:59Z

src/shared/frame-rpc.js

+          this._port.postMessage(message);
+        }
+
+        /** @deprecated */


Should just be a regular // Deprecated - ... comment here.

Yes, it should.

robertknight · 2021-07-14T16:17:50Z

src/shared/test/bridge-test.js

@@ -1,5 +1,16 @@
-import Bridge from '../bridge';
-import { RPC } from '../frame-rpc';
+import { default as Bridge, $imports } from '../bridge';


The Bridge tests were somewhat confusing because they mixed unit tests for the Bridge class alone, which mocked RPC in an unconventional way, with integration tests for Bridge + RPC that were not clearly demarcated. I pushed a commit that changes the tests in this module to all be unit tests which mock RPC using $imports.$mock, using the FakeRPC class below.

* change `@deprecated` for a comment * added a test to cover some missing lines in `_isValidSender` function. * `_isValidMessage` renamed to `_parseMessage` * created helper function `waitForMessageDelivery` * parametrise test

The `Bridge` code was written before we had good mocking tools in the client. As a result the `Bridge` class mocks `RPC` in an unconventional way where it constructs the real class, but then stubs out specific methods. Also tests varied in the degree to which they tested only `Bridge` vs Bridge + RPC together. This commit mocks `RPC` in bridge-test.js using our standard mocking tools and makes all tests in the file unit tests. This involved some significant changes to the `onConnect` tests.

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.

esanzgar changed the title ~~Support messagechannel frame rpc~~ Support MessageChannel in frame-rpc.js Jul 5, 2021

esanzgar requested review from robertknight and LMS007 July 5, 2021 17:16

LMS007 reviewed Jul 6, 2021

View reviewed changes

src/shared/frame-rpc.js Outdated Show resolved Hide resolved

src/shared/frame-rpc.js Show resolved Hide resolved

src/shared/test/frame-rpc-test.js Outdated Show resolved Hide resolved

LMS007 reviewed Jul 6, 2021

View reviewed changes

src/shared/test/frame-rpc-test.js Outdated Show resolved Hide resolved

esanzgar mentioned this pull request Jul 6, 2021

Epic: overhaul the inter-frame communication system #3533

Closed

13 tasks

robertknight requested changes Jul 7, 2021

View reviewed changes

esanzgar force-pushed the support-messagechannel-frame-rpc branch from 7f3c654 to 01f18cc Compare July 8, 2021 14:36

esanzgar force-pushed the support-messagechannel-frame-rpc branch from 01f18cc to 1c01876 Compare July 8, 2021 14:41

esanzgar commented Jul 8, 2021

View reviewed changes

robertknight requested changes Jul 9, 2021

View reviewed changes

esanzgar added 5 commits July 14, 2021 13:05

Initial support of MessageChannel on RPC

3aff972

It is a bit of a Frankenstein. The plan is to clean up on the next commits.

Add ListenerCollection utility to handle listeners

bb30fda

It simplifies the code.

Simplify internals

ef64bdf

Created a couple of utility methods to avoid duplication of code and enable to decouple RPC for `Window` or for `MessagePort`.

Strengthen types

886869c

esanzgar force-pushed the support-messagechannel-frame-rpc branch from 1c01876 to e9924e0 Compare July 14, 2021 11:13

esanzgar mentioned this pull request Jul 14, 2021

Support MessageChannel in bridge.js #3566

Merged

robertknight approved these changes Jul 14, 2021

View reviewed changes

esanzgar and others added 2 commits July 14, 2021 19:18

Various improvements based on PR feedback

e36f8b8

* change `@deprecated` for a comment * added a test to cover some missing lines in `_isValidSender` function. * `_isValidMessage` renamed to `_parseMessage` * created helper function `waitForMessageDelivery` * parametrise test

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

esanzgar merged commit b5e717a into master Jul 14, 2021

esanzgar deleted the support-messagechannel-frame-rpc branch July 14, 2021 17:56

esanzgar mentioned this pull request Jul 15, 2021

Add integration test between RPC and Bridge classes #3579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support MessageChannel in `frame-rpc.js` #3565

Support MessageChannel in `frame-rpc.js` #3565

esanzgar commented Jul 5, 2021 •

edited

Loading

LMS007 left a comment

robertknight left a comment

robertknight Jul 7, 2021

esanzgar Jul 8, 2021

codecov bot commented Jul 8, 2021 •

edited

Loading

esanzgar left a comment •

edited

Loading

esanzgar Jul 8, 2021

esanzgar Jul 8, 2021

robertknight left a comment

robertknight Jul 9, 2021

robertknight left a comment

robertknight Jul 14, 2021

robertknight Jul 14, 2021

robertknight Jul 14, 2021

esanzgar Jul 14, 2021

robertknight Jul 14, 2021

esanzgar Jul 14, 2021 •

edited

Loading

robertknight Jul 14, 2021

robertknight Jul 14, 2021

esanzgar Jul 14, 2021

robertknight Jul 14, 2021

Support MessageChannel in frame-rpc.js #3565

Support MessageChannel in frame-rpc.js #3565

Conversation

esanzgar commented Jul 5, 2021 • edited Loading

LMS007 left a 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

codecov bot commented Jul 8, 2021 • edited Loading

Codecov Report

esanzgar left a comment • 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

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

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

Support MessageChannel in `frame-rpc.js` #3565

Support MessageChannel in `frame-rpc.js` #3565

esanzgar commented Jul 5, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

esanzgar left a comment •

edited

Loading

esanzgar Jul 14, 2021 •

edited

Loading