Skip to content

Commit

Permalink
Add support for MessagePort in shared/bridge.js
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
esanzgar committed Jul 19, 2021
1 parent cb38a10 commit 6dad72c
Show file tree
Hide file tree
Showing 5 changed files with 108 additions and 33 deletions.
6 changes: 3 additions & 3 deletions src/annotator/cross-frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ export class CrossFrame {
};

// Initiate connection to the sidebar.
const onDiscoveryCallback = (source, origin, token) =>
bridge.createChannel(source, origin, token);
discovery.startDiscovery(onDiscoveryCallback);
discovery.startDiscovery((source, origin, token) =>
bridge.createChannel({ source, origin, token })
);
frameObserver.observe(injectIntoFrame, iframeUnloaded);

/**
Expand Down
6 changes: 5 additions & 1 deletion src/annotator/test/cross-frame-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,11 @@ describe('CrossFrame', () => {
createCrossFrame();
fakeDiscovery.startDiscovery.yield('SOURCE', 'ORIGIN', 'TOKEN');
assert.called(fakeBridge.createChannel);
assert.calledWith(fakeBridge.createChannel, 'SOURCE', 'ORIGIN', 'TOKEN');
assert.calledWith(fakeBridge.createChannel, {
source: 'SOURCE',
origin: 'ORIGIN',
token: 'TOKEN',
});
});
});

Expand Down
79 changes: 72 additions & 7 deletions src/shared/bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export default class Bridge {
this.links = [];
/** @type {Record<string, (...args: any[]) => void>} */
this.channelListeners = {};
/** @type {Array<(channel: RPC, window: Window) => void>} */
/** @type {Array<(channel: RPC, window?: Window) => void>} */
this.onConnectListeners = [];
}

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

/**
* Deprecated - Remove after MessagePort conversion
* @typedef windowOptions
* @prop {Window} source - The source window
* @prop {string} origin - The origin of the document in `source`
* @prop {string} token - Shared token between the `source` and this window
* agreed during the discovery process
*/

/**
* Create a communication channel between frames using either `MessagePort` or
* `Window`.
*
* The created channel is added to the list of channels which `call`
* and `on` send and receive messages over.
*
* @param {windowOptions|MessagePort} options
* @return {RPC} - Channel for communicating with the window.
*/
createChannel(options) {
if (options instanceof MessagePort) {
return this._createChannelForPort(options);
} else {
// Deprecated - Remove after MessagePort conversion
return this._createChannelForWindow(options);
}
}

/**
* Create a communication channel using a `MessagePort`.
*
* The created channel is added to the list of channels which `call`
* and `on` send and receive messages over.
*
* @param {MessagePort} port
* @return {RPC} - Channel for communicating with the window.
*/
_createChannelForPort(port) {
const listeners = { connect: cb => cb(), ...this.channelListeners };

// Set up a channel
const channel = new RPC(
window /* dummy */,
port,
'*' /* dummy */,
listeners
);

let connected = false;
const ready = () => {
if (connected) {
return;
}
connected = true;
this.onConnectListeners.forEach(cb => cb(channel));
};

// Fire off a connection attempt
channel.call('connect', ready);

// Store the newly created channel in our collection
this.links.push(channel);

return channel;
}

/**
* Create a communication channel between this window and `source`.
*
* The created channel is added to the list of channels which `call`
* and `on` send and receive messages over.
*
* @param {Window} source - The source window.
* @param {string} origin - The origin of the document in `source`.
* @param {string} token
* @param {windowOptions} options
* @return {RPC} - Channel for communicating with the window.
* @deprecated
*/
createChannel(source, origin, token) {
_createChannelForWindow({ source, origin, token }) {
let channel = null;
let connected = false;

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

Expand Down Expand Up @@ -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
*/
onConnect(listener) {
this.onConnectListeners.push(listener);
Expand Down
46 changes: 25 additions & 21 deletions src/shared/test/bridge-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ describe('shared/bridge', () => {
bridge = new Bridge();

createChannel = () =>
bridge.createChannel(fakeWindow, 'http://example.com', 'TOKEN');
bridge.createChannel({
source: fakeWindow,
origin: 'http://example.com',
token: 'TOKEN',
});

fakeWindow = {
postMessage: sandbox.stub(),
Expand Down Expand Up @@ -85,11 +89,11 @@ describe('shared/bridge', () => {

it('calls a callback when all channels return successfully', done => {
const channel1 = createChannel();
const channel2 = bridge.createChannel(
fakeWindow,
'http://example.com',
'NEKOT'
);
const channel2 = bridge.createChannel({
source: fakeWindow,
origin: 'http://example.com',
token: 'NEKOT',
});
channel1.call.yields(null, 'result1');
channel2.call.yields(null, 'result2');

Expand All @@ -105,11 +109,11 @@ describe('shared/bridge', () => {
it('calls a callback with an error when a channels fails', done => {
const error = new Error('Uh oh');
const channel1 = createChannel();
const channel2 = bridge.createChannel(
fakeWindow,
'http://example.com',
'NEKOT'
);
const channel2 = bridge.createChannel({
source: fakeWindow,
origin: 'http://example.com',
token: 'NEKOT',
});
channel1.call.throws(error);
channel2.call.yields(null, 'result2');

Expand Down Expand Up @@ -260,16 +264,16 @@ describe('shared/bridge', () => {

describe('#destroy', () =>
it('destroys all opened channels', () => {
const channel1 = bridge.createChannel(
fakeWindow,
'http://example.com',
'foo'
);
const channel2 = bridge.createChannel(
fakeWindow,
'http://example.com',
'bar'
);
const channel1 = bridge.createChannel({
source: fakeWindow,
origin: 'http://example.com',
token: 'foo',
});
const channel2 = bridge.createChannel({
source: fakeWindow,
origin: 'http://example.com',
token: 'bar',
});

bridge.destroy();

Expand Down
4 changes: 3 additions & 1 deletion src/sidebar/services/frame-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,9 @@ export class FrameSyncService {
};

const discovery = new Discovery(window, { server: true });
discovery.startDiscovery(this._bridge.createChannel.bind(this._bridge));
discovery.startDiscovery((source, origin, token) =>
this._bridge.createChannel({ source, origin, token })
);
this._bridge.onConnect(addFrame);

this._setupSyncToFrame();
Expand Down

0 comments on commit 6dad72c

Please sign in to comment.