refactor(js): Fix a variety of type definitions that will break in future Flow versions #4749

mcous · 2020-01-14T20:03:58Z

overview

Flow v0.111 (latest: 0.115, ours: 0.110.1) introduced drastic changes to how object spreads are typed. If we upgrade, roughly 300 individual flow errors are introduced by ~150 lines in about ~50 files.

In my examination, we can break these cases down into a few categories:

Problems solved by strict object typing
Actually incorrect code
Problems in code that is deprecated anyway
New errors caused by the fact the Flow no longer allows union types as the indexer in object types

changelog

This PR attempts to...

Make object types exact where it helps (1)
Fix the most trivial instances of (2)
- Trivial meaning 1 or 2 line fixes that were ideally already covered by tests
Remove any unnecessary existing $FlowFixMes

...without actually updating the version of Flow we use.

review requests

Point (4) above is a gnarly one. Basically, we do a lot of stuff like this:

type Mount = 'left' | 'right'

type StateByMount = {| [Mount]: boolean |}

Moving forward with Flow, this won't work anymore. (Aside: this also doesn't work with TypeScript, but TS has something else called Mapped Types which we would use instead)

This makes a whole host of things we do a lot stop working. Flow's recommendation moving forward to is explicitly define the types at the get-go...

type StateByMount = {|
  left: boolean,
  right: boolean,
|}

...which works for some stuff, but is onerous for others.

Flow v0.115, however, is quite fast. So I think organizationally, we have some decisions to make regarding:

Are we going to just consider ourselves stuck on Flow 0.110?
If yes, how hard + fast do we want to push on getting to TypeScript?
If no, how many $FlowFixMe's are we willing to tolerate in non-deprecated code?
- Interesting take that suggests "as many as you need to be on the latest version of Flow"

remaining flow errors

After this PR is applied, updating flow-bin and suppressing all errors results in:

232 new Flow errors suppressed by...
97 new $FlowFixMes in...
App - 6 files
- app/src/components/DeckMap/index.js
- app/src/components/JogControls/index.js
- app/src/components/RobotSettings/SelectNetwork/ConnectForm.js
- app/src/components/calibrate-pipettes/Pipettes.js
- app/src/pipettes/selectors.js
- app/src/robot/reducer/calibration.js
Components - 2 files
- components/src/deck/RobotCoordsForeignDiv.js
- components/src/tooltips/Tooltip.js
Protocol Designer - 22 files
- protocol-designer/src/components/SettingsPage/FeatureFlagCard/FeatureFlagCard.js
- protocol-designer/src/components/StepEditForm/fields/TipPosition/TipPositionModal.js
- protocol-designer/src/components/modals/FilePipettesModal/index.js
- protocol-designer/src/file-data/selectors/fileCreator.js
- protocol-designer/src/labware-ingred/reducers/index.js
- protocol-designer/src/labware-ingred/selectors.js
- protocol-designer/src/load-file/migration/1_1_0.js
- protocol-designer/src/persist.js
- protocol-designer/src/step-forms/reducers/index.js
- protocol-designer/src/step-forms/selectors/index.js
- protocol-designer/src/step-generation/getNextRobotStateAndWarnings/dispenseUpdateLiquidState.js
- protocol-designer/src/step-generation/test-with-flow/fixtures/robotStateFixtures.js
- protocol-designer/src/step-generation/test-with-flow/mix.test.js
- protocol-designer/src/step-generation/test-with-flow/transfer.test.js
- protocol-designer/src/step-generation/utils/misc.js
- protocol-designer/src/steplist/formLevel/generateNewForm.js
- protocol-designer/src/steplist/formLevel/handleFormChange/dependentFieldsUpdateMix.js
- protocol-designer/src/steplist/formLevel/handleFormChange/dependentFieldsUpdateMoveLiquid.js
- protocol-designer/src/steplist/generateSubsteps.js
- protocol-designer/src/steplist/substepTimeline.js
- protocol-designer/src/tutorial/reducers.js
- protocol-designer/src/ui/steps/actions/actions.js

codecov · 2020-01-14T20:04:12Z

Codecov Report

Merging #4749 into edge will increase coverage by 0.21%.
The diff coverage is 25.64%.

@@            Coverage Diff             @@
##             edge    #4749      +/-   ##
==========================================
+ Coverage   57.96%   58.18%   +0.21%     
==========================================
  Files         956      956              
  Lines       26189    26467     +278     
==========================================
+ Hits        15181    15400     +219     
- Misses      11008    11067      +59

Impacted Files	Coverage Δ
...pp/src/components/CalibrateDeck/ConfirmPosition.js	`0% <ø> (ø)`	⬆️
protocol-designer/src/analytics/fullstory.js	`0% <ø> (ø)`	⬆️
labware-library/src/components/ui/ClickableIcon.js	`0% <ø> (ø)`	⬆️
components/src/forms/InputField.js	`100% <ø> (ø)`	⬆️
components/src/forms/SelectField.js	`13.63% <ø> (ø)`	⬆️
...c/components/RobotSettings/SelectNetwork/fields.js	`0% <ø> (ø)`	⬆️
components/src/structure/PageTabs.js	`100% <ø> (ø)`	⬆️
...pp/src/components/CalibrateLabware/ProceedToRun.js	`0% <ø> (ø)`	⬆️
protocol-designer/src/components/FilePage.js	`0% <ø> (ø)`	⬆️
app/src/analytics/make-event.js	`72.13% <ø> (ø)`	⬆️
... and 35 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 1f06422...bc77e31. Read the comment docs.

mcous · 2020-01-14T20:07:44Z

app/src/components/CalibrateLabware/ConfirmPositionContents.js

@@ -49,7 +50,7 @@ function ConfirmPositionContents(props: Props) {
        {...{ labware, calibrator, calibrateToBottom, useCenteredTroughs }}
        buttonText={confirmButtonText}
      />
-      <JogControls {...props} />
+      <JogControls jog={jog} />


<JogControls> only takes the one prop, so this code change is safe

mcous · 2020-01-14T20:07:59Z

app/src/components/ConnectPanel/UnreachableRobotItem.js

-            <Icon
-              name={'alert-circle'}
-              className={styles.robot_item_icon}
-              disabled


<Icon> has no prop named disabled

mcous · 2020-01-14T20:08:48Z

app/src/components/ReviewDeck/Prompt.js

-  const labwareType = robotSelectors.labwareType(props)
+  const { onClick, ...labware } = props
+  const { name, definition, slot } = labware
+  const labwareType = robotSelectors.labwareType(labware)


This change was driven by switching Labware to an exact object type. This minor code change keeps that all behaving

mcous · 2020-01-14T20:10:12Z

app/src/logger.js

-  'debug',
-  'silly',
-]
+type Logger = {|


This change demonstrates what I was talking about in my PR description. The app logger is simple enough that I figured I'd include this change as an example. plus, this will make the inevitable TS port a little easier

mcous · 2020-01-14T21:36:28Z

app/src/components/CalibratePanel/LabwareList.js

@@ -54,7 +54,6 @@ function LabwareList(props: Props) {
            modulesBySlot[lw.slot].name
          }
          isDisabled={disabled}
-          confirmed={lw.confirmed}


lw was already being spread in, so this line was unnecessary

mcous · 2020-01-14T21:37:36Z

app/src/http-api-client/calibration.js

@@ -68,8 +69,7 @@ export type DeckCalStartState = ApiCall<DeckStartRequest, DeckStartResponse>
 export type DeckCalCommandState = ApiCall<DeckCalRequest, DeckCalResponse>

 type RobotCalState = {
-  'calibration/deck/start'?: DeckCalStartState,
-  'calibration/deck'?: DeckCalCommandState,
+  [string]: any,


The typing on this state was an illusion and this file is super deprecated, so this just makes the danger more explicit. Unit tests are already in place for this state

mcous · 2020-01-14T21:37:54Z

app/src/http-api-client/reducer.js


 export type RobotApiState = $Shape<{|
-  ...NetworkingState,
+  [path: string]: any,


Same story here: types were an illusion

mcous · 2020-01-14T21:39:07Z

protocol-designer/src/containers/ConnectedFilePage.js

@@ -48,7 +48,6 @@ function mergeProps(

  return {
    ...passThruProps,
-    ...dispatchProps,


This just spread bare dispatch into the props of the component, which was not used

protocol-designer/src/step-forms/selectors/index.js

…ow >= 0.111

IanLondon · 2020-01-15T16:04:10Z

protocol-designer/src/components/LabwareSelectionModal/LabwareSelectionModal.js

@@ -168,7 +168,7 @@ const LabwareSelectionModal = (props: Props) => {
    >(
      defs,
      (acc, def: $Values<typeof defs>) => {
-        const category = def.metadata.displayCategory
+        const category: string = def.metadata.displayCategory


I wonder why it's not inferring this one?

EDIT: Oh it's string | LabwareDisplayCategory and enum type isn't a valid key for object indexing in new flow 😠 ...what the heck, Flow?

It's inferring string | LabwareCategory where LabwareCategory is a union type that makes Flow mad at its usage in a computed properly below this line

IanLondon

Thanks for doing this! Looks good, only change I'd like is to protocol-designer/src/step-forms/selectors/index.js which is just a slightly different workaround

mcous added WIP refactor labels Jan 14, 2020

mcous requested a review from a team January 14, 2020 20:03

mcous changed the title ~~Chore type fixes~~ refactor(js): Fix a variety of type definitions that will break in future Flow versions Jan 14, 2020

mcous commented Jan 14, 2020

View reviewed changes

mcous added ready for review and removed WIP labels Jan 14, 2020

mcous marked this pull request as ready for review January 14, 2020 21:40

mcous added 2 commits January 14, 2020 17:14

build(flow): remove unecessary flow devDependencies

380ab15

refactor(js): fix a variety of type definitions that will break on Fl…

4e05a81

…ow >= 0.111

mcous force-pushed the chore_type-fixes branch from 0d519d2 to 4e05a81 Compare January 14, 2020 22:19

IanLondon reviewed Jan 15, 2020

View reviewed changes

IanLondon approved these changes Jan 15, 2020

View reviewed changes

fixup: Revert change to getPipettesForInstrumentGroup

bc77e31

mcous merged commit 11c0f76 into edge Jan 15, 2020

mcous deleted the chore_type-fixes branch January 15, 2020 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(js): Fix a variety of type definitions that will break in future Flow versions #4749

refactor(js): Fix a variety of type definitions that will break in future Flow versions #4749

mcous commented Jan 14, 2020 •

edited

Loading

codecov bot commented Jan 14, 2020 •

edited

Loading

mcous Jan 14, 2020

mcous Jan 14, 2020

mcous Jan 14, 2020

mcous Jan 14, 2020

mcous Jan 14, 2020

mcous Jan 14, 2020

mcous Jan 14, 2020

mcous Jan 14, 2020

IanLondon Jan 15, 2020 •

edited

Loading

mcous Jan 15, 2020

IanLondon left a comment

refactor(js): Fix a variety of type definitions that will break in future Flow versions #4749

refactor(js): Fix a variety of type definitions that will break in future Flow versions #4749

Conversation

mcous commented Jan 14, 2020 • edited Loading

overview

changelog

review requests

remaining flow errors

codecov bot commented Jan 14, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanLondon Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanLondon left a comment

Choose a reason for hiding this comment

mcous commented Jan 14, 2020 •

edited

Loading

codecov bot commented Jan 14, 2020 •

edited

Loading

IanLondon Jan 15, 2020 •

edited

Loading