Runtime middleware #662
Runtime middleware #662
Conversation
bergie
force-pushed
the
runtime_middleware
branch
from
182e2bb to
8515d8e
Compare
| else | ||
| # Warn of actions that don't contain state | ||
| debug "#{groups.join(':')} was sent without state, using previous state" | ||
| state = data?.state or c.state |
There was a problem hiding this comment.
WTH is state middleware? Sounds like a god-module waiting to happen...
How does one know what is "state" and what is not supposed to be "state"?
There was a problem hiding this comment.
Explained in PR description. Basically we had problems with stale state because:
- Application state is kept in the main Polymer element
- However, certain middleware (and currently still stores) need partial state access
- For this, old stores kept their own partial state
- This state was not properly cleared in some situations, causing stores to use stale runtime connections etc
- Now latest state is sent alongside UI actions so stores/middlewares don't need to keep track of it on their own
However, in the transitional period we will have actions sent without state. This middleware amends them with the latest state.
There was a problem hiding this comment.
So as far as I understand, the entire application state is available as state on the messages - React style. Components like GetActionValues are then used to pick out the particular pieces of the state that is relevant for individual components that actually perform the logic of the "middleware".
Is this correct?
There was a problem hiding this comment.
Yes, this is correct. Middleware state access is meant to be read-only (though in plain JS that is hard to guarantee)
|
|
||
| exports.getComponent = -> | ||
| c = new noflo.Component | ||
| c.description = 'Strips state from action payloads for backwards compat' |
There was a problem hiding this comment.
Why do components care about the presence of additional attributes?
There was a problem hiding this comment.
They don't care about additional attributes, but the old stores expect to have event payload as the payload of the data packet. Now it is in data.payload (to provide support for data.state as well). This component is there so we don't need to change how the old stores operate until we migrate them to modern middleware/reducer combos
| # Send selected edges to runtime | ||
| 'runtime,graphs[0]' -> KEYS GetEdgeState(ui/GetActionValues) | ||
| Dispatch HANDLE[0] -> IN GetEdgeState | ||
| GetEdgeState VALUES[0] -> RUNTIME SendEdges(runtime/SendEdges) |
There was a problem hiding this comment.
Why do we use arrayports for this? :( How is one supposed to know what-is-what?
There was a problem hiding this comment.
Because we have variable number of connections to send data to. Just like we do with routers
There was a problem hiding this comment.
Variable meaning not-known-at-component-creation-time?
There was a problem hiding this comment.
Would prefer to use a JS array or object for such things, and then another component for picking out the particular element if/when needed. Don't like arrayports, and they are very badly supported in Flowhub.
| @@ -5,6 +5,7 @@ var exported = { | |||
| 'child_process': null, | |||
| 'uuid': require('uuid'), | |||
| 'flowhub-registry': require('flowhub-registry'), | |||
| 'fbp-protocol-client': require('fbp-protocol-client'), | |||
There was a problem hiding this comment.
Who owns the connection to the runtime? What is the lifetime? How do code which needs a reference get it?
There was a problem hiding this comment.
To be honest, that export there is just so we can load fbp-protocol-client in tests. We could do a separate webpack build for tests to remove it.
There was a problem hiding this comment.
Not asking about the module export, but about the connection handling.
There was a problem hiding this comment.
The connection is supposed to be owned by runtime/ConnectRuntime component, but right now we break encapsulation in the noflo-runtime element. That part should be refactored so that the Polymer element sends events to NoFlo-land and the correct component reacts to them
|
Breaks current Msgflo completely, nothing happens when adding nodes. No error messages shown to user. Console has |
|
@jonnor does MsgFlo send capabilities? |
|
Ok the |
|
@bergie yes of course. But Flowhub in project mode with existing components/graphs sends a bunch of messages before getting the reply of that... Not that I have any reason to believe that is new broken-ness. It also sends |
|
Looks like it re-sends the entire state also after receiving the |
There was a problem hiding this comment.
The edges payload is not following the spec.
It is using from and to instead of src and tgt.
https://flowbased.github.io/fbp-protocol/#edges
Improves runtime handling reliability following the same middleware approach as we did with users in #660.
This PR will change actions from
payloadto{ state: stateWhenActionFired, payload: payload }. Will make compatibility wrapper so that old stores don't need to be touched.