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

chore: resolve react-hooks/exhaustive-deps warnings #1991

Merged
merged 14 commits into from
Mar 30, 2021
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
7 changes: 4 additions & 3 deletions cypress/component/Autocomplete.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState, useCallback } from 'react'
import React, { useCallback, useState } from 'react'
import { mount } from '@cypress/react'
import debounce from 'debounce'
import {
Expand Down Expand Up @@ -67,9 +67,10 @@ export const DynamicOptionsAutocompleteExample = () => {
const [options, setOptions] = useState<typeof OPTIONS>([])
const [loading, setLoading] = useState(false)

// eslint-disable-next-line react-hooks/exhaustive-deps
const handleChangeDebounced = useCallback(
debounce(async (inputValue: string) => {
const newOptions = await loadOptions(inputValue.trim().toLowerCase())
debounce(async (newValue: string) => {
const newOptions = await loadOptions(newValue.trim().toLowerCase())

setLoading(false)
setOptions(newOptions)
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"@svgr/cli": "^5.5.0",
"@testing-library/react": "^11.2.5",
"@testing-library/react-hooks": "^5.0.3",
"@toptal/browserslist-config": "^0.1.0-alpha.2",
"@toptal/browserslist-config": "^1.1.0",
"@toptal/davinci": "^1.0.63",
"@types/debounce": "^1.2.0",
"@types/esprima": "^4.0.2",
Expand Down
2 changes: 1 addition & 1 deletion packages/picasso-forms/src/FieldWrapper/FieldWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ const FieldWrapper = <
clearValidators(name)
}
}
}, [])
}, [clearValidators, formConfig.validateOnSubmit, name])

const { meta, input } = useField<TInputValue>(name, {
validate: formConfig.validateOnSubmit ? undefined : validators,
Expand Down
75 changes: 34 additions & 41 deletions packages/picasso-forms/src/Form/Form.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import React, { useCallback, useMemo, ReactNode, useRef } from 'react'
import React, { useMemo, ReactNode, useRef } from 'react'
import {
Form as FinalForm,
FormProps as FinalFormProps
} from 'react-final-form'
import { FormApi, SubmissionErrors, getIn, setIn } from 'final-form'
import { FormApi, SubmissionErrors, getIn, setIn, AnyObject } from 'final-form'
import { Form as PicassoForm, Container } from '@toptal/picasso'
import { useNotifications } from '@toptal/picasso/utils'

Expand Down Expand Up @@ -31,8 +31,6 @@ import {
createFormContext
} from './FormContext'

type AnyObject = Record<string, any>

export type Props<T = AnyObject> = FinalFormProps<T> & {
autoComplete?: HTMLFormElement['autocomplete']
successSubmitMessage?: ReactNode
Expand All @@ -42,10 +40,10 @@ export type Props<T = AnyObject> = FinalFormProps<T> & {

const getValidationErrors = (
validators: Validators,
formValues: Record<string, any>,
form: FormApi<Record<string, any>>
) => {
let errors: Record<string, any> | undefined
formValues: any,
form: FormApi<any>
): SubmissionErrors | void => {
let errors: SubmissionErrors

Object.entries(validators).forEach(([key, validator]) => {
const fieldValue = getIn(formValues, key)
Expand All @@ -57,15 +55,15 @@ const getValidationErrors = (

const error = validator(fieldValue, formValues, fieldMetaState)

if (error) {
errors = setIn(errors || {}, key, error)
if (errors && error) {
errors = setIn(errors, key, error)
}
})

return errors
}

export const Form = <T extends any = Record<string, any>>(props: Props<T>) => {
export const Form = <T extends any = AnyObject>(props: Props<T>) => {
const {
children,
autoComplete,
Expand Down Expand Up @@ -105,36 +103,31 @@ export const Form = <T extends any = Record<string, any>>(props: Props<T>) => {
showError(failedSubmitMessage, undefined, { persist: true })
}

const handleSubmit = useCallback(
async (values, form, callback) => {
const validationErrors = getValidationErrors(
validationObject.current.getValidators(),
values,
form
)

if (validationErrors) {
return validationErrors
}

const submissionErrors = await onSubmit(values, form, callback)

if (!submissionErrors) {
showSuccessNotification()
} else {
showErrorNotification(submissionErrors)
}

return submissionErrors
},
[
failedSubmitMessage,
onSubmit,
showError,
showSuccess,
successSubmitMessage
]
)
const handleSubmit = async (
values: T,
form: FormApi<T>,
callback?: (errors?: SubmissionErrors) => void
) => {
const validationErrors = getValidationErrors(
validationObject.current.getValidators(),
values,
form
)

if (validationErrors) {
return validationErrors
}

const submissionErrors = await onSubmit(values, form, callback)

if (!submissionErrors) {
showSuccessNotification()
} else {
showErrorNotification(submissionErrors)
}

return submissionErrors
}

return (
<FormContext.Provider value={validationObject}>
Expand Down
4 changes: 2 additions & 2 deletions packages/picasso-lab/src/TreeView/PointNode/PointNode.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const PointNode = forwardRef<any, Props>(
const xPosition = node.x - nodeWidth / 2

return `translate(${xPosition},${node.y})`
}, [node.x, node.y])
}, [node.x, node.y, nodeWidth])

useLayoutEffect(() => {
if (nodeRef.current) {
Expand All @@ -41,7 +41,7 @@ export const PointNode = forwardRef<any, Props>(
})
}
}
}, [nodeRef.current, dimensions])
}, [dimensions, node.ref])

return (
<g id={node.id} transform={transform} ref={ref}>
Expand Down
13 changes: 8 additions & 5 deletions packages/picasso-lab/src/TreeView/TreeView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,20 @@ export const TreeView = (props: Props) => {
const classes = useStyles()
const rootRef = createRef<SVGSVGElement>()
const { nodes, links, selectedNode } = useTree({ data })

const center = useMemo<{ x: number; y: number } | undefined>(() => {
if (!selectedNode) {
return undefined
}
const { x: xPosition, y: yPosition, data } = selectedNode

const { x: xPosition, y: yPosition, data: nodeData } = selectedNode

return {
x: xPosition + (data.selectedOffset?.x || 0),
y: yPosition + (data.selectedOffset?.y || 0)
x: xPosition + (nodeData.selectedOffset?.x || 0),
y: yPosition + (nodeData.selectedOffset?.y || 0)
}
}, [selectedNode, selectedNode?.data])
Copy link
Collaborator

Choose a reason for hiding this comment

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

this was interesting 😁

}, [selectedNode])

const { handleZoom, zoom } = useZoom<SVGSVGElement>({
rootRef,
scaleExtent,
Expand All @@ -81,7 +84,7 @@ export const TreeView = (props: Props) => {
zoom
})
setInitialized(true)
}, [rootRef, initialized, zoom])
}, [rootRef, initialized, zoom, updateState])
denieler marked this conversation as resolved.
Show resolved Hide resolved

return (
<div className={classes.root}>
Expand Down
5 changes: 3 additions & 2 deletions packages/picasso-lab/src/TreeView/useNodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export const useNodes = (
const [initialized, setInitializedState] = useState<boolean>(false)
const initialNodes = useMemo(() => {
return getDynamicNodes(rootNode.descendants())
}, [])
}, [rootNode])
denieler marked this conversation as resolved.
Show resolved Hide resolved
const dynamicNodes = useMemo(() => {
const latestNodes = rootNode.descendants()

Expand All @@ -85,9 +85,10 @@ export const useNodes = (
})
}, [rootNode, initialNodes])

// we have to render nodes twice: first for the initial showing data, and the second one — with the correct positions.
const nodes = useMemo<DynamicPointNode[]>(() => {
return updateNodesYPosition(dynamicNodes)
// we have to render nodes twice: first for the initial showing data, and the second one — with the correct positions.
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the comment above: // we have to render nodes twice: first for the initial showing data, and the second one — with the correct positions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe fix it properly by doing a hook that would run a function on the second render?

}, [dynamicNodes, initialized])

useLayoutEffect(() => {
Expand Down
2 changes: 1 addition & 1 deletion packages/picasso-lab/src/TreeView/useTree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export const useTree = ({
leaves,
nodeWidth: fullNodeWidth
})
}, [data])
}, [data, fullNodeWidth])

const nodes = useNodes(rootNode)

Expand Down
2 changes: 1 addition & 1 deletion packages/picasso-lab/src/TreeView/useZoom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export const useZoom = <ZoomRefElement extends ZoomedElementBaseType>({
.duration(750)
.call(zoom.translateTo, center.x, center.y)
}
}, [rootRef.current, zoom, initialized, center])
}, [zoom, initialized, center, rootRef, initialScale])
denieler marked this conversation as resolved.
Show resolved Hide resolved

return {
zoom,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState, useCallback } from 'react'
import React, { useCallback, useState } from 'react'
import debounce from 'debounce'
import { Autocomplete, AutocompleteItem } from '@toptal/picasso'
import { isSubstring } from '@toptal/picasso/utils'
Expand Down Expand Up @@ -38,6 +38,7 @@ const Example = () => {
const [options, setOptions] = useState<AutocompleteItem[] | null>()
const [loading, setLoading] = useState(false)

// eslint-disable-next-line react-hooks/exhaustive-deps
const handleChangeDebounced = useCallback(
debounce(async (inputValue: string) => {
const newOptions = await loadOptions(inputValue.trim().toLowerCase())
Expand Down
12 changes: 5 additions & 7 deletions packages/picasso/src/Modal/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,7 @@ const generateKey = (() => {
})()

// eslint-disable-next-line react/display-name
export const Modal = forwardRef<HTMLElement, Props>(function Modal(
props,
ref
) {
export const Modal = forwardRef<HTMLElement, Props>(function Modal(props, ref) {
const {
children,
open,
Expand Down Expand Up @@ -175,23 +172,24 @@ export const Modal = forwardRef<HTMLElement, Props>(function Modal(
const resetBodyOverflow = () => {
document.body.style.overflow = bodyOverflow.current
}
const currentModal = modalId.current

if (open) {
// TODO: to be improved as part of https://toptal-core.atlassian.net/browse/FX-1069
bodyOverflow.current = document.body.style.overflow
document.body.style.overflow = 'hidden'

defaultManager.add(modalId.current)
defaultManager.add(currentModal)
} else {
resetBodyOverflow()

defaultManager.remove(modalId.current)
defaultManager.remove(currentModal)
}

return () => {
resetBodyOverflow()

defaultManager.remove(modalId.current)
defaultManager.remove(currentModal)
}
}, [open])

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ const useEnterOrSpaceKeyDownHandler = <
},
[
canOpen,
open,
filteredOptions,
highlightedIndex,
closeOnEnter,
handleSelect
handleSelect,
open,
close
]
)

Expand Down
4 changes: 2 additions & 2 deletions packages/picasso/src/SidebarItem/SidebarItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const useStyles = makeStyles<Theme>(styles, {
})

export const SidebarItem: OverridableComponent<Props> = memo(
forwardRef<HTMLElement, Props>(function SidebarItem(props, ref) {
forwardRef<HTMLElement, Props>(function SidebarItem (props, ref) {
const {
as,
children,
Expand Down Expand Up @@ -91,7 +91,7 @@ export const SidebarItem: OverridableComponent<Props> = memo(
{menu}
</SubMenuContext.Provider>
),
[menu]
[index, menu]
)

const titleCase = useTitleCase(propsTitleCase)
Expand Down
4 changes: 3 additions & 1 deletion packages/picasso/src/utils/use-width-of.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,16 @@ export interface ReferenceObject {
const useWidthOf = <T extends ReferenceObject>(element: T | null) => {
const [menuWidth, setMenuWidth] = useState<string | undefined>()

const offsetParent = element?.offsetParent

useLayoutEffect(() => {
if (!element) {
return
}
const { width } = element.getBoundingClientRect()

setMenuWidth(`${width}px`)
}, [element, element?.offsetParent])
}, [element, offsetParent])

return menuWidth
}
Expand Down
2 changes: 1 addition & 1 deletion packages/shared/src/Favicon/Favicon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const Favicon = ({ environment }: Props) => {
}

loadIcons()
}, [resolvedEnvironment])
}, [resolvedEnvironment, setIcons])

if (resolvedEnvironment === 'test') {
// do not load favicons in tests (e.g. in e2e)
Expand Down
1 change: 1 addition & 0 deletions packages/shared/src/Picasso/config/breakpoints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ export const useScreens = <T = unknown>() => {

return defaultValue
},
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because otherwise, eslint adds T as a dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a bug with eslint. I updated the dependency and it seems ok now
facebook/react#19751 (comment)

[screenKey]
)
}
Expand Down
Loading