Fix merge conflict between #44 and #68

[gnestor]

Closes https://github.com/nteract/vdom/issues/69

I don't understand the subtleties of itertools.chain.from_iterable, but my understanding of the intention here was to separate style from attributes until to_dict is called, at which point merge them with attributes. This just skips the tuple part and merges self.style with attributes (resulting from self.encode_attributes which does the event handler stuff).

[mpacer]

I can look at this soon (not immediately though). But I thought based on https://github.com/nteract/vdom/issues/70 we were going to move this out of attributes altogether.

[gnestor]

I haven't been able to test the vdom events stuff because comm-related APIs have changed in lab so my front-end implementation needs to be patched. However, this front-end event handling logic needs to live in @nteract/transform-vdom anyway, so I'm going to work on that.

The biggest challenge for the front-end implementation is how transform-vdom interfaces with the kernel. JupyterLab uses @jupyterlab/services and nteract uses rxjs. I see that in nteract, all outputs/transforms are passed a channels prop, so this seems like the way to do it in nteract. To send a message, I assume one would do:

this.props.channels.next(
  createMessage("comm_msg", {
    content: { target_name: targetName, data: serializedEvent }
  })
);

transform-vdom shouldn't be coupled to nteract, so it should accept some higher-level props, such as sendCommMessage(targetName: string, data: JSON). It would be nice to have a subscribeToComm(targetName: string, onMsg: (msg: Message) => void). The classic notebook (and ipython) interface is pretty nice:

this.comm = this.props.registerCommTarget(targetName: string, comm: Comm);
comm.onMsg(msg => {
  console.log(msg.content.data);
}
comm.send(serializedEvent);

In JupyterLab, it looks similar:

this.comm = this.session.kernel.connectToComm(targetName);
this.comm.then(comm => {
  comm.open({});
  comm.onMsg = (msg: any) => {
    console.log(msg.content.data);
  };
  comm.send(serializedEvent);
});

@rgbkrk Any thoughts? This seems like it may be relevant to the jupyter widgets transform work as well since both will need to interact with a kernel.

[rgbkrk]

I'd really love to step away from handing channels over to every output. I'd love to focus the API to match exactly what we need for the VDOM transform with these events. It looks like it would be some sort of onVDOMEvent handler that would need to accept: target_name and the event object as described in https://github.com/nteract/vdom/blob/master/docs/event-spec.md

[rgbkrk]

Adding on, this does mean that the comm open has to happen in asychronous logic outside of this React component. For nteract this means doing the comm open when we see these elements get displayed and also to create an action that would send this comm message (this last part is already implemented, we'd have to wire it up in notebook-app.js)

[gnestor]

Ok, to clarify: The VDOM transform component would receive a prop along the lines of onVDOMEvent and the component would use that in its event handlers:

(event) => {
  const serializedEvent = serializeEvent(event);
  this.props.onVDOMEvent(targetName, serializedEvent);
}

And then each client can provide that onVDOMEvent prop and handle the comm stuff as they see fit?

I chatted with @rmorshea yesterday and he brought up a good point that using update_display to update the front-end forces a full re-render of the component tree. Ideally, we would like to be able to send comm messages from the kernel to the front-end to update props on the already mounted components. How we go about doing this (without going the traitlets route and overcomplicating things) I'm not sure, but it raises the question of whether onVDOMEvent will be enough or whether we need to support sending and receiving comm messages.

[rgbkrk]

update_display should only force a full re-render of the component tree if the VDOM structure doesn't match. The DOM nodes stay the same.

[rgbkrk]

I'm not sure, but it raises the question of whether onVDOMEvent will be enough or whether we need to support sending and receiving comm messages.

When we have something more complicated to handle, let's chat about it at that time. The onVDOMEvent keeps it focused so that we don't have a wide open chasm of support within comms (otherwise it will inevitably lead to scope creep). This also helps for a future where the outputs are able to be iframed.

[gnestor]

Ok 👌 I think this PR is ready to merge:

• It fixes the merge conflict and unbreaks vdom master
• It adds a test for event handlers: https://github.com/nteract/vdom/issues/69

I am working on a new branch to address moving event handlers out of attributes: https://github.com/nteract/vdom/issues/70

[rgbkrk]

Ok, I'll merge this under the assumption you'll contribute new work on moving event handlers out of attributes.

[rgbkrk]

Thank you @gnestor and @mpacer!