-
Notifications
You must be signed in to change notification settings - Fork 198
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
Support MessageChannel in bridge.js
#3566
Changes from 1 commit
6dad72c
b30ed20
5e45547
a0302db
a3c2040
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 = []; | ||||||
} | ||||||
|
||||||
|
@@ -27,18 +27,83 @@ export default class Bridge { | |||||
this.links.forEach(channel => channel.destroy()); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Deprecated - Remove after MessagePort conversion | ||||||
* @typedef windowOptions | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Type names should use PascalCase. I'd probably have gone for something without an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed a commit that keeps the name but just converts it to PascalCase. |
||||||
* @prop {Window} source - The source window | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think using the term Naming this |
||||||
* @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); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed a commit that adds a few extra tests to cover the new code paths. |
||||||
} 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; | ||||||
|
||||||
|
@@ -53,7 +118,7 @@ export default class Bridge { | |||||
const connect = (_token, cb) => { | ||||||
if (_token === token) { | ||||||
cb(); | ||||||
ready(); | ||||||
ready(); // This is necessary for testing only. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed a commit that reworks the tests to avoid the need for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
} | ||||||
}; | ||||||
|
||||||
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the callers of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed a commit that removes the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏼 |
||||||
*/ | ||||||
onConnect(listener) { | ||||||
this.onConnectListeners.push(listener); | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 addedArray.from
in some places to be conservative.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I makes sense.