-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
lib: add a library for cross-frame communication #2309
Conversation
39e1214
to
b4e0bbd
Compare
|
||
export interface Message { | ||
type: string, | ||
id: string, |
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.
perhaps just number 'id'?
} | ||
|
||
|
||
private postMessage(window: Window, message: string) { |
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.
nit: I get that "private" already denotes it is private, but can we be consistent about the '_' prefixes? e.g. either 'postMessage' and 'onMessage' both start with _, or both do not?
TensorBoard, in the near future, will load plugin modules in respective iframe as was discussed in RFC. This change adds a library that faciliates communication between main TensorBoard and plugin frames.
b4e0bbd
to
84a90c1
Compare
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.
LGTM, thanks!
@@ -46,6 +54,9 @@ export abstract class IPC { | |||
} | |||
|
|||
private async _onMessage(event: MessageEvent) { |
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.
nit: drop '_'?
private id = 0; | ||
private readonly responseWaits = new Map<string, PromiseResolver>(); | ||
private readonly listeners = new Map<MessageType, MessageCallback>(); | ||
|
||
constructor() { | ||
window.addEventListener('message', this._onMessage.bind(this)); | ||
const randomArray = new Uint8Array(16); | ||
window.crypto.getRandomValues(randomArray); |
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.
This is creative!
Our API is good, so let's continue with this. In a followup, it would be neat to try using MessageChannels to replace the crypto-prefixing.
[1] https://developer.mozilla.org/en-US/docs/Web/API/MessageChannel
167be68
to
5df6b41
Compare
Is there any doc about this design? It seems like plugin authors will need instructions to use it effectively (docstrings on public methods, at least). |
There isn't a design doc for a plugin library just yet. When there is some idea on what to expose publicly, there should definitely be a design doc! That said, this PR seems like a good building block that allows us to experiment. The HostIPC and GuestIPC are not intended to be exposed to plugin authors at all, these just make up a communication layer for passing messages between the main frame & iframes. With this PR, we can locally prototype how a "getRunsList()" command might be sent from a dynamic plugin's iframe. |
Since nothing is really hooked up to this code, I will merge it. We can discuss in future PRs and possibly revert this change if necessary. |
TensorBoard, in the near future, will load plugin modules in respective
iframe as was discussed in RFC. This change adds a library that
facilitates communication between main TensorBoard and plugin frames.