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

feat(js): update Formik to v2 #5190

Merged
merged 7 commits into from
Mar 11, 2020
Merged

feat(js): update Formik to v2 #5190

merged 7 commits into from
Mar 11, 2020

Conversation

IanLondon
Copy link
Contributor

overview

Serves as its own ticket

We use Formik in several projects and it had a major v1 -> v2 release in October. The breaking changes are minimal: https://jaredpalmer.com/formik/docs/migrating-v2

What do we get? Some nice stuff

For the migration to v2, I found 2 changes that affected us:

  • resetForm args changed. This PR changes resetForm(stuff) to resetForm({values: stuff})
  • Avoid render prop, this is deprecated. In this PR, I used a child function instead. (Unfortunately they still don't provide a hook interface, you need to use <Formik> or withFormik(). Maybe you could do <Formik><MyFormComponent /></Formik> and in MyFormComponent you could use useFormikContext hook? See docs)

changelog

  • Update Formik to latest v2.x.x
  • Migrate forms
  • Install flow-typed defs for Formik 2.x.x and use them in our Formik forms! This led to some fixes b/c before much of the forms were implicitly any-typed.

review requests

  1. Code review
  2. Please make sure the forms I touched still work correctly!

Run App

  • app/src/components/RobotSettings/SelectNetwork/ConnectForm.js

  • app/src/components/AppSettings/AddManualIp/ManualIpForm.js

  • app/src/components/ConfigurePipette/ConfigForm.js

Labware Creator

It's just a big Formik form!

  • labware-library/src/labware-creator/index.js

Protocol Designer

  • protocol-designer/src/components/FilePage.js -- File Page's file fields
  • protocol-designer/src/components/LiquidPlacementForm/LiquidPlacementForm.js -- adding liquids to a labware
  • protocol-designer/src/components/LiquidsPage/LiquidEditForm.js -- creating/editing liquids in the Liquids tab

@@ -195,18 +196,19 @@ export class ConfigForm extends React.Component<ConfigFormProps> {
initialValues={initialValues}
validate={this.validate}
validateOnChange={false}
render={formProps => {
>
{(formProps: FormikProps<FormValues>) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just changed from render prop to child fn

setValues: FormValues => mixed
): mixed {
setValues: FormValues => mixed,
resetForm: ({| values: FormValues |}) => mixed,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the shape of the args for setValues vs resetForm Formik fns used to match, now in v2 they don't. So now I pass both fns into handleSecurityChange and decide which to call inside here (instead of choosing which of the 2 fns to pass into handleSecurityChange from outside)

@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #5190 into edge will increase coverage by 0.13%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #5190      +/-   ##
==========================================
+ Coverage   60.29%   60.43%   +0.13%     
==========================================
  Files        1025     1025              
  Lines       29029    29230     +201     
==========================================
+ Hits        17502    17664     +162     
- Misses      11527    11566      +39     
Impacted Files Coverage Δ
...components/AppSettings/AddManualIp/ManualIpForm.js 0.00% <ø> (ø)
app/src/components/ConfigurePipette/ConfigForm.js 0.00% <0.00%> (ø)
...src/components/ConfigurePipette/ConfigFormGroup.js 0.00% <ø> (ø)
...ponents/RobotSettings/SelectNetwork/ConnectForm.js 16.30% <0.00%> (-0.55%) ⬇️
...library/src/labware-creator/components/Dropdown.js 0.00% <0.00%> (ø)
...brary/src/labware-creator/components/RadioField.js 0.00% <ø> (ø)
...ibrary/src/labware-creator/components/TextField.js 0.00% <ø> (ø)
labware-library/src/labware-creator/index.js 0.00% <0.00%> (ø)
protocol-designer/src/components/FilePage.js 0.00% <ø> (ø)
...ponents/LiquidPlacementForm/LiquidPlacementForm.js 0.00% <0.00%> (ø)
... and 10 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 cdf0c8a...bb55ace. Read the comment docs.

@mcous
Copy link
Contributor

mcous commented Mar 9, 2020

This is looking good to me so far, and would be quite helpful for the rewrite of ConnectForm we're trying to land as part of the Wi-Fi disconnect push. Would be great if we could validate and get this in asap

@IanLondon IanLondon added app Affects the `app` project labware creator protocol designer Affects the `protocol-designer` project labels Mar 10, 2020
@IanLondon IanLondon marked this pull request as ready for review March 10, 2020 16:32
@IanLondon IanLondon requested review from a team as code owners March 10, 2020 16:32
const handleReset = () => {
const newValues = mapValues(values, v => {
if (typeof v === 'boolean') {
// NOTE: checkbox fields don't have defaults from the API b/c they come in from `quirks`
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 isn't a change, I just added a note b/c I got confused about what was intended here 🙃

@@ -107,13 +109,13 @@ export function ConfigCheckbox(props: ConfigCheckboxProps) {
const id = makeId(name)
return (
<ConfigFormRow label={displayName} labelFor={id}>
<Field name={name}>
<Field name={name} type="checkbox">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type="checkbox" is required for fieldProps.field.checked to work

@@ -70,7 +83,7 @@ export function SelectField(props: SelectFieldProps) {
menuPosition={menuPosition}
formatOptionLabel={formatOptionLabel}
onChange={opt => onValueChange && onValueChange(name, opt?.value || '')}
onBlur={() => onLoseFocus && onLoseFocus(name)}
onBlur={handleOnBlur}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcous do you know why we didn't do something like this earlier? Formik needs that event to handle onBlur, otherwise it will throw an error at runtime (eg in labware-library/src/labware-creator/components/Dropdown.js)

Copy link
Contributor

@mcous mcous Mar 10, 2020

Choose a reason for hiding this comment

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

SelectField is an old component, and was always designed to use setFieldValue and setFieldTouched rather than onChange and onBlur (see SelectField.md). The reasons behind this were:

  • onBlur is attached to the <input type="text"> that <ReactSelect> sometimes (maybe? It's pretty hard to tell from the react-select docs so this is a combo of code reading and docs inference) renders depending on what props you pass in
  • onChange in <ReactSelect> is not "standard", so it felt weird to have a "vanila" onBlur but a custom onChange

Accordingly, it looks lile labware-library/src/labware-creator/components/Dropdown.js was written incorrectly for how SelectField is designed to be used:

// is now
onLoseFocus={() => {
  reportFieldEdit({ value: field.value, name: field.name })
  field.onBlur()
}}

// "should" be
onLoseFocus={name => {
  reportFieldEdit({ value: field.value, name })
  form.setFieldTouched(name)
}}

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Wi-Fi connect and pipette settings forms in the app look like they're behaving

  • Responded to why SelectField doesn't have an onBlur prop. If you feel strongly about it, we can add it, but it looks like the stuff in LC that was using SelectField was actually not using its onLoseFocus prop correctly
  • asdsafsd: * is deprecated and should be avoiding in favor of simply omitting the type annotation
  • Premature useMemo

@@ -70,7 +83,7 @@ export function SelectField(props: SelectFieldProps) {
menuPosition={menuPosition}
formatOptionLabel={formatOptionLabel}
onChange={opt => onValueChange && onValueChange(name, opt?.value || '')}
onBlur={() => onLoseFocus && onLoseFocus(name)}
onBlur={handleOnBlur}
Copy link
Contributor

@mcous mcous Mar 10, 2020

Choose a reason for hiding this comment

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

SelectField is an old component, and was always designed to use setFieldValue and setFieldTouched rather than onChange and onBlur (see SelectField.md). The reasons behind this were:

  • onBlur is attached to the <input type="text"> that <ReactSelect> sometimes (maybe? It's pretty hard to tell from the react-select docs so this is a combo of code reading and docs inference) renders depending on what props you pass in
  • onChange in <ReactSelect> is not "standard", so it felt weird to have a "vanila" onBlur but a custom onChange

Accordingly, it looks lile labware-library/src/labware-creator/components/Dropdown.js was written incorrectly for how SelectField is designed to be used:

// is now
onLoseFocus={() => {
  reportFieldEdit({ value: field.value, name: field.name })
  field.onBlur()
}}

// "should" be
onLoseFocus={name => {
  reportFieldEdit({ value: field.value, name })
  form.setFieldTouched(name)
}}

app/src/components/AppSettings/AddManualIp/ManualIpForm.js Outdated Show resolved Hide resolved
components/src/forms/SelectField.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

🍴

@IanLondon IanLondon merged commit b15d360 into edge Mar 11, 2020
@IanLondon IanLondon deleted the js_update-formik-to-v2 branch March 11, 2020 14:43
@IanLondon IanLondon added this to the SPDDRS Sprint 4 milestone Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project JS Tech Debt labware creator protocol designer Affects the `protocol-designer` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants