refactor(app): Implement modules API-client with redux-observable #3395

mcous · 2019-04-29T21:21:31Z

overview

After going through the 3.8.2 release testing process and running into #3378, I started an exploration last week of finally revamping the app's API state tracking with these goals:

Handle the state of ID'd resources in a sane manner
- Especially when multiple endpoints can retrieve and/or affect the given resource
Keep easy endpoints easy, while making additional behavior easy to layer on
Improve the DX of the API state selectors

Based on @b-cooper's previous experience using it, I decided to try out redux-observable and rxjs. Observables are neat, but there's a learning curve we'll have to deal with. Luckily HTTP API calls don't go too deep into that curve.

I'm really curious to see if we think this is a viable path forward and if it looks like it solves our existing problems. Did I miss anything in the above bullets, and is anything in this PR confusing?

changelog

refactor(app): Implement modules API-client with redux-observable

The /modules endpoints hit the bullet points above really nicely:

GET /modules, GET /modules/:serial/data, and POST /modules/:serial all retrieve and change the same resources
POST /modules/:serial needs an immediate GET /modules/:serial/data afterwards to update the model in state, which involves layering some logic on top of a default request behavior
Selectors for module data are a mess because the state shape is a mess

review requests

The new new HTTP API state lives in app/src/robot-api. I'm planning on getting everything moved over from http-api-client as quickly as possible to minimize confusion, but for a minute it's gonna be weird. Sorry.

checklist

Modules card in "Pipettes and Modules"
Temperature controls with OT_APP_DEV_INTERNAL__TEMPDECK_CONTROLS=1
Module missing and module connected messages in deck setup
Temp deck card on run screen

tour of `robot-api`

resources/
- Directory for all resource categories
- index.js - Exports
- health.js - Simple module for the /health endpoint
  - /health isn't actually used explicitly by the app; it's wrapped up in the discovery client
  - This file is really just for show
- modules.js - Action creators, a reducer, a selector, and epics for the three modules endpoints
fetch.js
- Minimal wrapper of rxjs::fromFetch; calls window.fetch but wraps it in an Observable with cancellation, which will come in handy later for networking endpoints
index.js - Exports
README.md - Instructions; give this one a read and let me know what you think
reducer.js
- HTTP API state is now divided into resources and networking per robot by robot name
- The top level reducer is basically a custom combineReducers that handles the networking state and the "per robot name" part of things
utils.js
- Boilerplate minimizers
- createBaseRequestEpic - Defines an epic for a given endpoint
- getRobotApiState - Gets a robot's API state
- Action creators apiCall, apiResponse, and apiError
- Various string constants

codecov · 2019-04-29T21:36:13Z

Codecov Report

Merging #3395 into edge will increase coverage by 1.18%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##             edge    #3395      +/-   ##
==========================================
+ Coverage   52.56%   53.74%   +1.18%     
==========================================
  Files         771      782      +11     
  Lines       22821    24241    +1420     
==========================================
+ Hits        11996    13029    +1033     
- Misses      10825    11212     +387

Impacted Files	Coverage Δ
app/src/components/ModuleItem/index.js	`0% <ø> (ø)`	⬆️
app/src/http-api-client/index.js	`100% <ø> (ø)`	⬆️
app/src/components/ModuleItem/ModuleInfo.js	`0% <ø> (ø)`	⬆️
app/src/http-api-client/reducer.js	`100% <ø> (ø)`	⬆️
app/src/epic.js	`0% <0%> (ø)`
app/src/components/ModuleControls/index.js	`0% <0%> (ø)`	⬆️
app/src/robot-api/resources/modules.js	`0% <0%> (ø)`
...c/components/ModuleControls/TemperatureControls.js	`0% <0%> (ø)`	⬆️
app/src/robot-api/reducer.js	`0% <0%> (ø)`
app/src/robot-api/http.js	`0% <0%> (ø)`
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15da0f0...e31301f. Read the comment docs.

mcous · 2019-04-29T21:24:10Z

app/src/components/ModuleControls/index.js


  return (
-    <IntervalWrapper


This IntervalWrapper called GET /modules/:serial/data can be removed because:

Its parent is already calling GET /modules on an interval

Module data by serial is now properly merged into the state in the same place as the response from GET /modules goes

mcous · 2019-04-29T21:24:55Z

app/package.json

@@ -31,18 +31,21 @@
    "lodash": "^4.17.4",
    "mixpanel-browser": "^2.22.1",
    "moment": "^2.19.1",
+    "path-to-regexp": "^3.0.0",


This is the same module react-router uses to turn route paths like /modules/:serial into regular expressions. Figured we could use it for the same thing for the same reasons

mcous · 2019-04-29T21:28:00Z

app/src/components/ConnectModulesModal/index.js

-  const getRobotModules = makeGetRobotModules()
+function mapStateToProps(state: State, ownProps: OP): SP {
+  const sessionModules = robotSelectors.getModules(state)
+  const actualModules = getModulesState(state, ownProps.robot.name)


getModulesState is a non-memoized selector. It's simply pulling an object out of state by the robot name, so any sort of memoization:

Probably adds some overhead

Makes everything harder to read and more prone to errors

mcous · 2019-04-29T21:29:46Z

app/src/components/TempDeckStatusCard/index.js

-      tempdeck,
-      tempdeckData,
-    }
+function mapStateToProps(state: State): SP {


The diff on this one mapStateToProps I think is the best encapsulation on the problems with our previous state management strategy

mcous · 2019-04-29T21:31:17Z

app/src/robot-api/fetch.js

+  }
+
+  return fromFetch(url, options).pipe(
+    timeout(timeoutMs),


Default timeout of something less than 5 minutes!

mcous · 2019-04-29T21:36:47Z

app/src/robot-api/resources/modules.js

+// after POST /modules/:serial completes, call GET /modules/:serial/data
+const _setTargetTempEpic = createBaseRequestEpic('POST', MODULE_BY_SERIAL_PATH)
+
+const setTargetTempEpic: Epic = action$ => {


Because an epic is an action stream (action$: Observable<Action>) => Observable<Action>, we can pipe more stuff onto the base stream to add functionality. In this case, if the POST comes back successfully, we tack on a GET .../data request for the same serial

Because I'm new to RxJS, I don't actually know if a switchMap is the best way to do this, so any guidance is appreciated

This article does a pretty good job of comparing map vs. mergeMap vs. switchMap (https://netbasal.com/understanding-mergemap-and-switchmap-in-rxjs-13cf9c57c885). The TL;DR of it is that in this case, For each emission of the outer Observable (every time the epic is hit with it's request action), map would mint a new inner Observable that needs to be subscribed to and cleaned up etc., mergeMap manages the subscription of the inner observable and will emit once even if a past request is still in flight, without cancelling the past request, switchMap is very similar to mergeMap, except it will cancel all older inner observables as soon as a newer one emits. This makes switchMap a good enough fit for this case as we only care about the most recent call to the robots

mcous · 2019-04-29T21:38:48Z

app/src/robot-api/resources/modules.js

+  setTargetTempEpic
+)
+
+export function modulesReducer(


By having per-resource-type reducers, we avoid the problems caused by trying to be too generic with API state. For /modules, our resource state is best expressed as an array of attached modules. This get rid of a bunch of annoying modulesCall && modulesCall.response && ... chains.

In the future we should look into normalization (i.e. flattening to by-ID objects), but I didn't want to lump that in here, too

mcous · 2019-04-29T21:39:42Z

app/src/robot-api/types.js

@@ -0,0 +1,107 @@
+// @flow


Trying to keep all API types in this file for now. Maybe won't scale

mcous · 2019-04-29T21:40:46Z

app/src/robot-api/utils.js

+  payload,
+})
+
+export const createBaseRequestEpic = (method: Method, path: string): Epic => {


Base epic for any given endpoint. I'm hopeful this will be broadly applicable, with additional logic able to be pipe'd in

mcous · 2019-04-29T21:41:34Z

shared-data/robot-data/pipetteModelSpecs.json

@@ -885,7 +885,7 @@
          "aspirate": [
            [1.774444444, -0.1917910448, 1.2026],
            [2.151481481, -0.0706286837, 1.0125],
-            [2.898518519, -0.04343083788, 0.9540],
+            [2.898518519, -0.04343083788, 0.954],


This is just here because I ran make format and apparently someone had forgotten to before me

b-cooper

After looking at the code it seems like this is the right path to go down.

I added a few discussion point comments, but overall, I think the diff elucidates the positive effect of introducing this layer of async state management.

💡

app/src/index.js

b-cooper · 2019-05-01T19:26:10Z

app/src/reducer.js

@@ -3,11 +3,14 @@
 import { combineReducers } from 'redux'
 import { routerReducer } from 'react-router-redux'

-// robot state
+// oldest robot api state


app/src/robot-api/README.md

app/src/robot-api/fetch.js

app/src/robot-api/utils.js

Co-Authored-By: mcous <[email protected]>

Kadee80

📝 🚀
README was super helpful
New State makes sense
Will do further reading up on rxjs before asking any in person questions

app/src/robot-api/fetch.js

refactor(app): Implement modules API-client with redux-observable

7f270f5

mcous added app Affects the `app` project ready for review refactor labels Apr 29, 2019

mcous requested review from Kadee80, b-cooper and IanLondon April 29, 2019 21:21

mcous commented Apr 29, 2019

View reviewed changes

b-cooper approved these changes May 1, 2019

View reviewed changes

fixup: s/that/than/

c3724da

Co-Authored-By: mcous <[email protected]>

Kadee80 approved these changes May 2, 2019

View reviewed changes

fixup: Add method and path to internal action types

183b8ef

b-cooper reviewed May 2, 2019

View reviewed changes

app/src/robot-api/fetch.js Outdated Show resolved Hide resolved

b-cooper reviewed May 2, 2019

View reviewed changes

app/src/robot-api/fetch.js Outdated Show resolved Hide resolved

fixup: Make names and types consistent

f43f326

b-cooper approved these changes May 6, 2019

View reviewed changes

fixup: Fix one wrong util function return type

e31301f

mcous merged commit d015ce3 into edge May 6, 2019

mcous deleted the app-refactor-http-api branch May 6, 2019 20:36

This was referenced May 13, 2019

refactor(app): Move /settings endpoints to redux-observable #3453

Merged

refactor(app): Move /pipettes state to redux-observable #3462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(app): Implement modules API-client with redux-observable #3395

refactor(app): Implement modules API-client with redux-observable #3395

mcous commented Apr 29, 2019

codecov bot commented Apr 29, 2019 •

edited

Loading

mcous Apr 29, 2019

mcous Apr 29, 2019

mcous Apr 29, 2019

mcous Apr 29, 2019

mcous Apr 29, 2019

mcous Apr 29, 2019 •

edited

Loading

b-cooper May 2, 2019 •

edited

Loading

mcous Apr 29, 2019

mcous Apr 29, 2019

mcous Apr 29, 2019

mcous Apr 29, 2019

b-cooper left a comment

b-cooper May 1, 2019

Kadee80 left a comment

refactor(app): Implement modules API-client with redux-observable #3395

refactor(app): Implement modules API-client with redux-observable #3395

Conversation

mcous commented Apr 29, 2019

overview

changelog

review requests

checklist

tour of robot-api

codecov bot commented Apr 29, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

b-cooper May 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-cooper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kadee80 left a comment

Choose a reason for hiding this comment

tour of `robot-api`

codecov bot commented Apr 29, 2019 •

edited

Loading

mcous Apr 29, 2019 •

edited

Loading

b-cooper May 2, 2019 •

edited

Loading