[RFC] Divorce socket state from middleware and implement subscription

[NoahBres]

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 (https://github.com/acmerobotics/ftc-dashboard/blob/29f78cb4dea96aecf5d3448ac53681f5ca9af179/FtcDashboard/dash/src/store/middleware/socketMiddleware.ts#L45).

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: https://github.com/acmerobotics/ftc-dashboard/blob/d070d2df1d49e720b353f03c071eb04d034d1fd1/FtcDashboard/dash/src/containers/FieldView.jsx#L72

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: https://github.com/acmerobotics/ftc-dashboard/blob/d070d2df1d49e720b353f03c071eb04d034d1fd1/FtcDashboard/dash/src/containers/FieldView.jsx#L30 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.

[NoahAndrews]

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.

[rbrott]

This seems problematic as component rendering is coupled to new Redux store data.

This is exactly my mental model: Redux store changes drive component re-renders.

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.

I'm hoping I can better understand your objection by making it more concrete. Are you bothered that FieldView componentDidUpdate() may find no satisfactory overlays in this.props.telemetry but re-render anyway? Maybe that logic should be pushed into the reducer then? Or perhaps the problem is the shallow props equality check at the beginning of componentDidUpdate()?

Rather than telemetry/socket data being passed down through props, components should be able to choose to subscribe to telemetry/socket data.

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.

[NoahBres]

I'm hoping I can better understand your objection by making it more concrete. Are you bothered that FieldView componentDidUpdate() may find no satisfactory overlays in this.props.telemetry but re-render anyway? Maybe that logic should be pushed into the reducer then? Or perhaps the problem is the shallow props equality check at the beginning of componentDidUpdate()?

I utilize a reducer in LoggingView

But the logic here requires a useEffect on the telemetry Redux selector and then dispatches actions to the telemetry store reducer: https://github.com/acmerobotics/ftc-dashboard/blob/29f78cb4dea96aecf5d3448ac53681f5ca9af179/FtcDashboard/dash/src/containers/LoggingView/LoggingView.tsx#L224

So the rendering flow follows the pattern of:

• Redux store pushes new telemetry states
• Component picks this up via selector
  • Causes re-render
• Component picks up telemetry state change via useEffect
  • new telemetry state dispatched to reducer
• New state derived in reducer
  • Causes re-render

It's not a huge problem. It works. It just doesn't seem to match the actual intended behavior, instead chaining multiple state changes.

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

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.

[rbrott]

Thanks, LoggingView is a clear instance of the problem. Here's my articulation of the problem: Since there's little overlap in the component views/aggregations/reductions of the message stream, each component should maintain its own internal view instead of duplicating the component tree structure in the Redux store and splitting the component-specific logic between the component and the store reducers.

Rather than telemetry/socket data being passed down through props, components should be able to choose to subscribe to telemetry/socket data.

Sounds good. I even started down a similar path in anger when trying to improve the graphing code.