plugin: message channels for dynamic plugins #2703

psybuzz · 2019-09-27T16:32:42Z

Motivation for features / changes

This iterates on the experimental code in plugin_util/ for communication with IFrames. This PR does NOT address the public API.

Technical description of changes

Using window.postMessage and window.addEventListener('message', ...) works, but
Having multiple concurrent IPCs (e.g. a frame cannot be a host and a guest, a host cannot have multiple guests) required today's idPrefix system. Random-number-generated idPrefixes are prepended to message ids to distinguish which frame it comes from, to resolve the appropriate responseWait.

This PR uses MessageChannels, instead of random-numbers, to establish a private channel between frames.

The IPC now requires an isReply: true to be present on reply messages. This allows a frame to distinguish a case where (it receives a foreign message with id=X) vs (it receives a reply for its own previous message with id=X).

The flow looks like

Guest frame sends a normal hostWindow.postMessage to bootstrap, transferring ownership of one of it's MessagePorts
Host frame establishes an IPC on the received port (no need to worry about stray a postMessage from an extension!)
Guest can sendMessage(), listen(), unlisten() as usual. Host can broadcast() on all established ports.

WANT_LGTM=all

tensorboard/components/plugin_util/message.ts

tensorboard/components/plugin_util/plugin-guest.ts

tensorboard/components/plugin_util/plugin-host.ts

psybuzz · 2019-09-27T23:29:35Z

Added tests, cr address. Ready for another look!

stephanwlee

Thanks for making this module better! Also thanks for adding test cases I missed!

tensorboard/components/plugin_util/message.ts

stephanwlee · 2019-09-27T23:35:14Z

tensorboard/components/plugin_util/message.ts

Is this for readability of the module? Or does it have functional differences?

Functional difference, and the test I added covers it!

Basically, if a frame receives message {id: 0, ...}, it doesn't know whether it's the reply-message to something it previously sent, or whether it's a brand new incoming message from another frame.

In both cases, the origin is a foreign frame, but we need to distinguish that the former case should resolve the responseWait, while the latter should not.

tensorboard/components/plugin_util/plugin-guest.ts

tensorboard/components/plugin_util/plugin-host.ts

tensorboard/components/plugin_util/test/plugin-test.ts

stephanwlee · 2019-09-27T23:50:02Z

tensorboard/components/plugin_util/test/plugin-test.ts

consider using the fluent alternative.
expect(longTaskCb).to.have.been.calledOnce;

consider adding assertion on the payload. expect(longTaskCb).to.have.been.calledWith(...)

Done x2
Yes, asserting the payload is a good idea

tensorboard/components/plugin_util/test/plugin-test.ts

psybuzz · 2019-09-28T00:40:54Z

Comments addressed, ready for another look.

davidsoergel · 2019-09-30T15:19:02Z

I really think we need more consensus on what plugin architecture we're aiming for before checking in code. The two open docs (Stephan's and Erik's) are a good start, as is the new weekly meeting. Let's use those venues, as well as perhaps the TB design review, to solidify our plans--and then return to this.

stephanwlee · 2019-09-30T15:43:53Z

@davidsoergel would this draft PR (#2705) be what you are seeking? Just do note that this work is time sensitive.

psybuzz · 2019-09-30T19:07:14Z

@davidsoergel as per offline discussion, I've moved plugin_util/ under tensorboard/components/experiments/, to indicate that this is current work is unstable.

I opted to not put it at the root level (tensorboard/experiments/) because there happens to be another build target "plugin_util" at the root level BUILD that has nothing todo with components.

Would this be fine with you?

davidsoergel

components/experimental is good, thanks.

I don't fully follow the logic re id collisions and isReply. It's fine given the experimental status, but I'm looking forward to whiteboarding/etc. to understand this once other dust settles. Thanks!

davidsoergel · 2019-09-30T19:35:59Z

tensorboard/components/experimental/plugin_util/plugin-host.ts

+  }
+}
+
+export const broadcast = _broadcast;


Is the purpose of this pattern (as opposed to just export function broadcast(...) {...}) to prevent monkey-patching? Just curious if I should adopt this generally.

The underscored version is just there to allow us to export it via es6 (which is used/imported by the test) and export it under a namespace, at the same time.

davidsoergel · 2019-09-30T19:42:06Z

tensorboard/components/experimental/plugin_util/plugin-host.ts

+export const listen = _listen;
+export const unlisten = _unlisten;
+
+namespace tf_plugin {


Namespaces are disallowed by go/tsstyle (publicly: https://github.com/google/gts/blob/83cb107010f7a956bb008291066304a9f26a0f34/tslint-rules.json#L43). TensorBoard contains a fair number of them anyway, but let's not add more if possible.

Good point. @stephanwlee, could you weigh in on this?

There does seem to be alot of existing namespaces, ever since this PR, but I think the Angular ng-tensorboard/ folder avoids this.

plugin-host (at least until the tf_ng_tensorboard work is finished) will be loaded inside Polymer context which uses the namespace

(in anticipation of inevitable question) in ideal world, we want to share base code between host and guest impl except it is super hard to make ts_library build rule work with tf_web_library. The latter is used in mainly for polymer based TensorBoard. For now, since the experimental API is only used in context of Polymer anyways, I decided to just deprecate the [es]module style and use namespace everywhere as v0.

Thanks for the explanation. It's unfortunate that Polymer code depends on these files. I'll land as-is for now, but it seems we all agree that 'namespaces' are not ideal, and should be avoided if possible.

googlebot added the cla: yes label Sep 27, 2019

psybuzz requested review from davidsoergel and stephanwlee and removed request for davidsoergel and stephanwlee September 27, 2019 16:33

psybuzz changed the title ~~plugin: message channels for dynamic plugins~~ [DRAFT] plugin: message channels for dynamic plugins Sep 27, 2019

stephanwlee reviewed Sep 27, 2019

View reviewed changes

psybuzz changed the title ~~[DRAFT] plugin: message channels for dynamic plugins~~ plugin: message channels for dynamic plugins Sep 27, 2019

stephanwlee reviewed Sep 27, 2019

View reviewed changes

psybuzz force-pushed the ste3 branch from 01e221d to a24c8b7 Compare September 28, 2019 00:36

psybuzz requested a review from davidsoergel September 28, 2019 00:40

stephanwlee approved these changes Sep 28, 2019

View reviewed changes

psybuzz added 3 commits September 30, 2019 10:36

plugin: message channels for dynamic plugins

cbbb754

rename

9623bb5

manual merge

ab4f004

psybuzz force-pushed the ste3 branch from 217d1ab to ab4f004 Compare September 30, 2019 19:04

davidsoergel approved these changes Sep 30, 2019

View reviewed changes

psybuzz merged commit 842b881 into tensorflow:master Oct 1, 2019

plugin: message channels for dynamic plugins #2703

plugin: message channels for dynamic plugins #2703

Uh oh!

Conversation

psybuzz commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

psybuzz commented Sep 27, 2019

Uh oh!

stephanwlee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psybuzz Sep 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

psybuzz commented Sep 28, 2019

Uh oh!

davidsoergel commented Sep 30, 2019

Uh oh!

stephanwlee commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psybuzz commented Sep 30, 2019

Uh oh!

davidsoergel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

psybuzz commented Sep 27, 2019 •

edited

Loading

psybuzz Sep 28, 2019 •

edited

Loading

stephanwlee commented Sep 30, 2019 •

edited

Loading