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

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

Merged
merged 5 commits into from
May 6, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,21 @@
"lodash": "^4.17.4",
"mixpanel-browser": "^2.22.1",
"moment": "^2.19.1",
"path-to-regexp": "^3.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"react": "^16.6.3",
"react-dom": "^16.6.3",
"react-hot-loader": "^3.0.0-beta.7",
"react-redux": "^5.0.6",
"react-redux": "^5.1.1",
"react-router": "^4.2.0",
"react-router-dom": "^4.2.2",
"react-router-redux": "^5.0.0-alpha.6",
"redux": "^3.6.0",
"redux-thunk": "^2.2.0",
"redux": "^4.0.1",
"redux-observable": "^1.1.0",
"redux-thunk": "^2.3.0",
"remark": "^9.0.0",
"remark-react": "^4.0.3",
"reselect": "^3.0.1",
"rxjs": "^6.5.1",
"semver": "^5.5.0",
"winston": "^2.2.0"
}
Expand Down
36 changes: 14 additions & 22 deletions app/src/components/ConnectModulesModal/ReviewModuleItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as React from 'react'
import { connect } from 'react-redux'
import countBy from 'lodash/countBy'

import { makeGetRobotModules } from '../../http-api-client'
import { getModulesState } from '../../robot-api'
import { getConnectedRobot } from '../../discovery'
import { selectors as robotSelectors } from '../../robot'
import { Module as ModuleItem } from '@opentrons/components'
Expand All @@ -18,7 +18,7 @@ type SP = {| module: ?SessionModule, present: boolean |}

type Props = { ...OP, ...SP }

export default connect<Props, OP, SP, _, _, _>(makeMapStateToProps)(
export default connect<Props, OP, SP, _, _, _>(mapStateToProps)(
ReviewModuleItem
)

Expand All @@ -33,26 +33,18 @@ function ReviewModuleItem(props: Props) {
)
}

function makeMapStateToProps(): (state: State, ownProps: OP) => SP {
function mapStateToProps(state: State, ownProps: OP): SP {
// TODO(mc, 2018-07-23): this logic is duplicated because can only get props
// into Deck.props.LabwareComponent via redux
const getRobotModules = makeGetRobotModules()

return (state, ownProps) => {
const robot = getConnectedRobot(state)
const module = robotSelectors.getModulesBySlot(state)[ownProps.slot]
const sessionModules = robotSelectors.getModules(state)
const actualModulesCall = robot && getRobotModules(state, robot)
const actualModules =
actualModulesCall &&
actualModulesCall.response &&
actualModulesCall.response.modules

const requiredNames = countBy(sessionModules, 'name')
const actualNames = countBy(actualModules || [], 'name')
const present =
!module || requiredNames[module.name] === actualNames[module.name]

return { present, module }
}
const robot = getConnectedRobot(state)
const module = robotSelectors.getModulesBySlot(state)[ownProps.slot]
const sessionModules = robotSelectors.getModules(state)
const actualModules = robot ? getModulesState(state, robot.name) : []

const requiredNames = countBy(sessionModules, 'name')
const actualNames = countBy(actualModules, 'name')
const present =
!module || requiredNames[module.name] === actualNames[module.name]

return { present, module }
}
24 changes: 9 additions & 15 deletions app/src/components/ConnectModulesModal/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as React from 'react'
import { connect } from 'react-redux'
import countBy from 'lodash/countBy'

import { makeGetRobotModules, fetchModules } from '../../http-api-client'
import { getModulesState, fetchModules } from '../../robot-api'
import {
selectors as robotSelectors,
actions as robotActions,
Expand All @@ -17,7 +17,7 @@ import ReviewModuleItem from './ReviewModuleItem'

import type { State, Dispatch } from '../../types'
import type { RobotService, SessionModule } from '../../robot'
import type { Module } from '../../http-api-client'
import type { Module } from '../../robot-api'

type OP = {| robot: RobotService |}

Expand All @@ -28,7 +28,7 @@ type DP = {| setReviewed: () => mixed, fetchModules: () => mixed |}
type Props = { ...OP, ...SP, ...DP }

export default connect<Props, OP, SP, DP, State, Dispatch>(
makeMapStateToProps,
mapStateToProps,
mapDispatchToProps
)(ConnectModulesModal)

Expand All @@ -48,19 +48,13 @@ function ConnectModulesModal(props: Props) {
)
}

function makeMapStateToProps(): (state: State, ownProps: OP) => SP {
const getRobotModules = makeGetRobotModules()
function mapStateToProps(state: State, ownProps: OP): SP {
const sessionModules = robotSelectors.getModules(state)
const actualModules = getModulesState(state, ownProps.robot.name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


return (state, ownProps) => {
const sessionModules = robotSelectors.getModules(state)
const actualModulesCall = getRobotModules(state, ownProps.robot)
const actualModules =
actualModulesCall.response && actualModulesCall.response.modules

return {
modulesRequired: sessionModules.length !== 0,
modulesMissing: checkModulesMissing(sessionModules, actualModules),
}
return {
modulesRequired: sessionModules.length !== 0,
modulesMissing: checkModulesMissing(sessionModules, actualModules),
}
}

Expand Down
43 changes: 14 additions & 29 deletions app/src/components/FileInfo/ProtocolModulesCard.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,7 @@ import { connect } from 'react-redux'

import { getModuleDisplayName } from '@opentrons/shared-data'
import { selectors as robotSelectors } from '../../robot'
import {
makeGetRobotModules,
fetchModules,
type FetchModulesResponse,
} from '../../http-api-client'
import { getModulesState, fetchModules } from '../../robot-api'

import { RefreshWrapper } from '../Page'
import InfoSection from './InfoSection'
Expand All @@ -20,12 +16,13 @@ import InstrumentWarning from './InstrumentWarning'
import type { State, Dispatch } from '../../types'
import type { SessionModule } from '../../robot'
import type { Robot } from '../../discovery'
import type { Module } from '../../robot-api'

type OP = {| robot: Robot |}

type SP = {|
modules: Array<SessionModule>,
actualModules: ?FetchModulesResponse,
actualModules: Array<Module>,
attachModulesUrl: string,
|}

Expand All @@ -36,7 +33,7 @@ type Props = { ...OP, ...SP, ...DP }
const TITLE = 'Required Modules'

export default connect<Props, OP, SP, DP, _, _>(
makeMapStateToProps,
mapStateToProps,
mapDispatchToProps
)(ProtocolModulesCard)

Expand All @@ -47,17 +44,9 @@ function ProtocolModulesCard(props: Props) {

const moduleInfo = modules.map(module => {
const displayName = getModuleDisplayName(module.name)
const modulesMatch = actualModules.some(m => m.name === module.name)

const actualModel =
actualModules && actualModules.modules.find(m => m.name === module.name)

const modulesMatch = actualModel != null && actualModel.name === module.name

return {
...module,
displayName,
modulesMatch,
}
return { ...module, displayName, modulesMatch }
})

const modulesMatch = moduleInfo.every(m => m.modulesMatch)
Expand All @@ -80,19 +69,15 @@ function ProtocolModulesCard(props: Props) {
)
}

function makeMapStateToProps(): (state: State, ownProps: OP) => SP {
const getActualModules = makeGetRobotModules()
function mapStateToProps(state: State, ownProps: OP): SP {
const { robot } = ownProps
const actualModules = getModulesState(state, robot.name)

return (state, ownProps) => {
const { robot } = ownProps
const modulesCall = getActualModules(state, robot)

return {
modules: robotSelectors.getModules(state),
actualModules: modulesCall && modulesCall.response,
// TODO(mc, 2018-10-10): pass this prop down from page
attachModulesUrl: `/robots/${robot.name}/instruments`,
}
return {
actualModules,
modules: robotSelectors.getModules(state),
// TODO(mc, 2018-10-10): pass this prop down from page
attachModulesUrl: `/robots/${robot.name}/instruments`,
}
}

Expand Down
42 changes: 19 additions & 23 deletions app/src/components/InstrumentSettings/AttachedModulesCard.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,65 +4,61 @@ import * as React from 'react'
import { connect } from 'react-redux'

import { Card, IntervalWrapper } from '@opentrons/components'
import { fetchModules, makeGetRobotModules } from '../../http-api-client'
import { fetchModules, getModulesState } from '../../robot-api'
import ModulesCardContents from './ModulesCardContents'
import { getConfig } from '../../config'

import type { State, Dispatch } from '../../types'
import type { Module } from '../../http-api-client'
import type { Module } from '../../robot-api'
import type { Robot } from '../../discovery'

type OP = {| robot: Robot |}

type SP = {|
modules: ?Array<Module>,
refreshing: boolean,
__featureEnabled: boolean,
modules: Array<Module>,
__tempControlsEnabled: boolean,
|}

type DP = {| refresh: () => mixed |}
type DP = {| fetchModules: () => mixed |}

type Props = { ...OP, ...SP, ...DP }

const TITLE = 'Modules'
const POLL_MODULE_INTERVAL_MS = 5000

export default connect<Props, OP, SP, DP, State, Dispatch>(
makeMapStateToProps,
mapStateToProps,
mapDispatchToProps
)(AttachedModulesCard)

function AttachedModulesCard(props: Props) {
return (
<IntervalWrapper interval={2000} refresh={props.refresh}>
<IntervalWrapper
interval={POLL_MODULE_INTERVAL_MS}
refresh={props.fetchModules}
>
<Card title={TITLE} column>
<ModulesCardContents
modules={props.modules}
robot={props.robot}
showControls={props.__featureEnabled}
showControls={props.__tempControlsEnabled}
/>
</Card>
</IntervalWrapper>
)
}

function makeMapStateToProps(): (state: State, ownProps: OP) => SP {
const getRobotModules = makeGetRobotModules()

return (state, ownProps) => {
const modulesCall = getRobotModules(state, ownProps.robot)
const modulesResponse = modulesCall.response
const modules = modulesResponse && modulesResponse.modules
const devInternal = getConfig(state).devInternal
return {
modules: modules,
refreshing: modulesCall.inProgress,
__featureEnabled: !!devInternal && !!devInternal.tempdeckControls,
}
function mapStateToProps(state: State, ownProps: OP): SP {
return {
modules: getModulesState(state, ownProps.robot.name),
__tempControlsEnabled: Boolean(
getConfig(state).devInternal?.tempdeckControls
),
}
}

function mapDispatchToProps(dispatch: Dispatch, ownProps: OP): DP {
return {
refresh: () => dispatch(fetchModules(ownProps.robot)),
fetchModules: () => dispatch(fetchModules(ownProps.robot)),
}
}
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
// @flow
import * as React from 'react'
import type { Module } from '../../http-api-client'
import type { Module } from '../../robot-api'
import ModuleItem, { NoModulesMessage } from '../ModuleItem'

import type { Robot } from '../../discovery'

type Props = {
robot: Robot,
modules: ?Array<Module>,
modules: Array<Module>,
showControls: boolean,
}

export default function ModulesCardContents(props: Props) {
const { modules, robot, showControls } = props
if (!modules || !modules[0]) return <NoModulesMessage />
if (modules.length === 0) return <NoModulesMessage />

return (
<React.Fragment>
Expand Down
9 changes: 4 additions & 5 deletions app/src/components/ModuleControls/TemperatureControls.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,24 @@ import TempField from './TempField'

import styles from './styles.css'

import type { SetTemperatureRequest } from '../../http-api-client'
import type { SetTemperatureRequest } from '../../robot-api'

type Props = {
setTemp: (request: SetTemperatureRequest) => mixed,
}

export default class TemperatureControls extends React.Component<Props> {
inputRef: { current: null | HTMLInputElement }

constructor(props: Props) {
super(props)
this.inputRef = React.createRef()
}

deactivateModule = () => {
const request = {
command_type: 'deactivate',
}
this.props.setTemp(request)
this.props.setTemp({ command_type: 'deactivate' })
}

render() {
return (
<Formik
Expand Down
Loading