Skip to content
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

Receive graph events from runtime #564

Closed

Conversation

ifitzpatrick
Copy link
Contributor

@ifitzpatrick ifitzpatrick commented Apr 28, 2016

This pull request allows noflo-ui to respond to commands from the runtime such as addnode, addedge, etc., and update the graph exposed by the graph editor accordingly. It depends on this pull request to noflo-runtime: noflo/noflo-runtime#66

The two pull requests are together intended to address this issue: #390.


when 'removenode'
id = payload.payload.id

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check to see if the node has already been removed

@chadrik
Copy link

chadrik commented May 6, 2016

@bergie and @jonnor, I reviewed this and the other PR and it looks good to me.

I wanted to mention one thing, which maybe should make it into the comments of UpateGraph.coffee.

Updates get from the editor to the runtime like this:

  1. graph editor updates Graph instance
  2. Graph event fires
  3. event listeners send a graph message to the runtime
  4. backend acknowledges by bouncing the graph message back

With this PR, we're now listening for graph messages from the runtime, but it is not possible to distinguish between actual update messages originating from the runtime and those which are simply acknowledging an update previously requested by the UI. This PR is careful to avoid feedback loops by first checking to see if the current state is already correct. This works, but I think it would be good if ack messages were easier to distinguish. Also it might be nice if every message had a UUID so that we could easily pair up requests with their acks or errors. Anyway, it's probably more than we need for this PR, but wanted to get the gears turning on that idea.

@jonnor
Copy link
Member

jonnor commented May 23, 2016

@ifitzpatrick the required noflo-runtime PR has now been merge. Can you update this?

@@ -26,7 +26,7 @@
"noflo/noflo-indexeddb": "*",
"noflo/noflo-github": "*",
"noflo/noflo-graph": "*",
"noflo/noflo-runtime": "*",
"ifitzpatrick/noflo-runtime": "*",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be possible to swap back to upstream

@chadrik
Copy link

chadrik commented May 24, 2016

@jonnor component.json now points back to noflo

@jonnor
Copy link
Member

jonnor commented May 24, 2016

I don't see any changes here regarding 'live mode', in particular the component:getsource command which used to be emitted to get the initial graph? #390

When this change is in, what is the correct behavior for a runtime which is already running a graph?
That upon a "runtime:getruntime", it sends all of the current graph state via the normal methods? graph:clear, graph:addnode, graph:addedge etc? If so, we need to document that in github.com/flowbased/fbp-protocol

What will happen if a runtime does not do send these graph:.. messages upon UI connect, but instead responds to the component:getsource? What happens if the runtime does both (interesting due to gradual transitioning)?

@ifitzpatrick
Copy link
Contributor Author

ifitzpatrick commented May 25, 2016

Correct behavior would be that when the runtime receives network:getstatus, it would build the graph with the normal methods graph:addnode, graph:addedge etc. I will write up some documentation for fbp-protocol to reflect that.

If it sends both getsource and the new events, the graph: events should be ignored since the nodes and edges those events are trying to add will already exist in the graph. Legacy runtimes should still be able to respond to getsource instead, or along with the new events if it is transitioning gradually.

@ifitzpatrick
Copy link
Contributor Author

Maybe we should have another event besides getstatus though to request a fresh graph? There might be other reasons a UI might want to request status besides starting up originally. Maybe a new graph:getgraph command so the protocols of the request and response match? Would love to hear your thoughts. I chose getstatus because of this comment on the "live mode" thread.

@ifitzpatrick
Copy link
Contributor Author

Once we nail down how exactly the protocol should function I will work on getting fbp-test to test this functionality.

@ifitzpatrick
Copy link
Contributor Author

ifitzpatrick commented Jun 16, 2016

@jonnor After looking over the get source stuff, I think that the runtime should probably decide whether it wants to support a graph:getgraph event, to which it will respond with addnode/addedge/etc., or component:getsource. The issue with supporting gradual transition, is that a runtime may support getsource as described in the protocol to supply component source code, but not necessarily support getsource for graphs. If a runtime wants to use getsource, the UpdateGraph component will still be able to receive updates from the runtime.

@ifitzpatrick
Copy link
Contributor Author

ifitzpatrick commented Jun 16, 2016

I have a pull request ready for fbp-protocol describing the graph:getgraph message, which depends on the json-schema pull request awaiting approval. I will share that once we finalize the json-schema pr.

@ifitzpatrick
Copy link
Contributor Author

Will reopen or open a new PR once we have cleared up the protocol situation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants