-
Notifications
You must be signed in to change notification settings - Fork 125
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
[RFC] Divorce socket state from middleware and implement subscription #93
Comments
While refactoring this, I'd encourage you to consider building on top of the SDK's WebSocket library. I've found NanoHTTPD's WebSocket server to be very buggy, and reusing the already-running server is more efficient. The SDK's WebSocket libraries are designed to be easy to make use of (even as a third-party library), and provide support for subscribing to particular message namespaces and registering handlers for individual message types. |
This is exactly my mental model: Redux store changes drive component re-renders.
I'm hoping I can better understand your objection by making it more concrete. Are you bothered that
Is the central store the culprit? This approach is tantamount to storing a message log in the store with simple reducers and pushing that logic down to individual components. I can see the centralized/distributed trade-off here, but I'm not clear what material benefits flow from dispatching with Redux. The current dash dataflow peculiarities seem to stem mostly from not putting enough logic in the reducers. |
I utilize a reducer in But the logic here requires a useEffect on the telemetry Redux selector and then dispatches actions to the telemetry store reducer:
So the rendering flow follows the pattern of:
It's not a huge problem. It works. It just doesn't seem to match the actual intended behavior, instead chaining multiple state changes.
The thought in this change was that Redux store changes force re-renders and any component that wishes to store or transform their own telemetry state has to cascade state changes/re-renders rather than directly deriving it. |
Thanks,
Sounds good. I even started down a similar path in anger when trying to improve the graphing code. |
In its current state, Dash handles websocket communication and the passing of data in a very unconventional way.
The socket and how it dispatches data is handled entirely through redux middleware (
ftc-dashboard/FtcDashboard/dash/src/store/middleware/socketMiddleware.ts
Line 45 in 29f78cb
The way that this is communicated down to components that do need it, is through the Redux connect method, binding the Redux store to the component props:
ftc-dashboard/FtcDashboard/dash/src/containers/FieldView.jsx
Line 72 in d070d2d
This seems problematic as component rendering is coupled to new Redux store data. The component is no longer in charge of rendering. This was worked around via React's old
componentDidUpdate
method:ftc-dashboard/FtcDashboard/dash/src/containers/FieldView.jsx
Line 30 in d070d2d
This works but the component is never in charge of its own rendering here—rather, it spends its business logic deciding when not to render. The component renders at the whim of the telemetry redux state, or whichever state it's props are mapped to. This seems like it would be a violation of the "UI as a function of state" as the state (render props) are actively being triaged against. Obviously, React + the nature of JavaScript makes this tenet difficult to follow but it seems like it could be improved with the following underlying change:
Rather than telemetry/socket data being passed down through props, components should be able to choose to subscribe to telemetry/socket data. This should divorce the telemetry/socket data from component rendering. Components can attach a listener on mount and choose to act on the information stream internally—transforming data directly and then setting state, rather than preventing renders and triaging state updates.
This can be done incrementally with the current prop/telemetry architecture in place.
The text was updated successfully, but these errors were encountered: