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

borisyordanov · 2021-03-18T08:19:06Z

Description

Describe the changes and motivations for the pull request.

How to test

run yarn lint --rule "{react-hooks/exhaustive-deps: error}" --quiet, it should return 0 errors

Screenshots

Before.	After.
Insert screenshots or screen recordings	Insert screenshots or screen recordings

Review

Read CONTRIBUTING.md and Component API principles
Annotate all props in component with documentation
Create examples for component
Ensure that deployed demo has expected results and good examples
Ensure that tests pass by running yarn test
Ensure that visuals tests pass by running yarn test:visual. If not - check the documentation how to fix visual tests
Ensure the changed/created components have not caused accessibility issues. How to use a11y plugin in storybook.

PR commands

List of available commands:

@toptal-bot run all - Run whole pipeline
@toptal-bot run build - Check build
@toptal-bot run visual - Run visual tests
@toptal-bot run deploy:documentation - Deploy documentation
@toptal-bot run package:alpha-release - Release alpha version

borisyordanov · 2021-03-18T12:07:09Z

@toptal-bot run visual

OleksandrNechai · 2021-03-18T13:01:58Z

cypress/component/Autocomplete.spec.tsx

-  const handleChangeDebounced = useCallback(
-    debounce(async (inputValue: string) => {
-      const newOptions = await loadOptions(inputValue.trim().toLowerCase())
+  const handleChangeDebounced = debounce(async (inputValue: string) => {


This does not make sense to me. Debounce will be run on every render and hence the effect will be lost. If this is just to delay the execution, maybe we should rather use delay function here?

OleksandrNechai · 2021-03-18T13:08:22Z

packages/picasso-forms/src/Form/Form.tsx

-  const showErrorNotification = (errors: SubmissionErrors) => {
-    if (typeof errors === 'string') {
-      showError(errors, undefined, { persist: true })
+  const showErrorNotification = useCallback(


Why do we struggle here for the reference identity but later do this

onSubmit={handleSubmit} decorators={[...decorators, scrollToErrorDecorator]}

decorators always get a new array and render happens all the time.

I think we should remove all useCallback if there are no performance problems reports

I probably agree, it will hide the issue

OleksandrNechai · 2021-03-18T13:22:37Z

packages/picasso-lab/src/TreeView/useNodes.ts

  // 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)
+    // eslint-disable-next-line react-hooks/exhaustive-deps


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.

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

OleksandrNechai · 2021-03-18T13:23:04Z

packages/picasso-lab/src/TreeView/useZoom.ts

  const [initialized, setInitialized] = useState(false)
  const zoom = useMemo(
    () => d3.zoom<ZoomRefElement, unknown>().scaleExtent(scaleExtent),
+    // eslint-disable-next-line react-hooks/exhaustive-deps


Because eslint adds ZoomRefElement as a dependency

Is not there a way to fix it properly? It's just type, right?

OleksandrNechai · 2021-03-18T13:23:58Z

packages/picasso/src/Autocomplete/story/DynamicOptions.example.tsx

-  )
+    setLoading(false)
+    setOptions(newOptions)
+  }, 500)


Same as above. This is not debounced any more

OleksandrNechai · 2021-03-18T13:28:20Z

packages/picasso/src/utils/use-width-of.ts

 const useWidthOf = <T extends ReferenceObject>(element: T | null) => {
  const [menuWidth, setMenuWidth] = useState<string | undefined>()

+  const parent = element?.offsetParent


is parent accurate name?

I would rename it

is parentElement okay?

OleksandrNechai · 2021-03-18T13:29:05Z

packages/shared/src/Picasso/config/breakpoints.ts


      return defaultValue
    },
+    // eslint-disable-next-line react-hooks/exhaustive-deps


Because otherwise, eslint adds T as a dependency

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

packages/picasso-lab/src/TreeView/useZoom.ts

denieler

A bit dangerous change. It might break some behaviour in some hooks 😃 But probably better to fix this rule, break and fix later 👍

denieler · 2021-03-29T15:47:52Z

packages/picasso-forms/src/Form/Form.tsx

-  const showErrorNotification = (errors: SubmissionErrors) => {
-    if (typeof errors === 'string') {
-      showError(errors, undefined, { persist: true })
+  const showErrorNotification = useCallback(


I probably agree, it will hide the issue

denieler · 2021-03-29T15:48:40Z

packages/picasso-lab/src/TreeView/TreeView.tsx

+      x: xPosition + (nodeData.selectedOffset?.x || 0),
+      y: yPosition + (nodeData.selectedOffset?.y || 0)
    }
-  }, [selectedNode, selectedNode?.data])


this was interesting 😁

packages/picasso-lab/src/TreeView/TreeView.tsx

packages/picasso-lab/src/TreeView/useNodes.ts

denieler · 2021-03-29T15:51:18Z

packages/picasso-lab/src/TreeView/useNodes.ts

+    if (!initialized) {
+      return updateNodesYPosition(dynamicNodes)
+    }
+
    return updateNodesYPosition(dynamicNodes)


are those 2 different? look the same

denieler · 2021-03-29T15:52:25Z

packages/picasso/src/utils/use-width-of.ts

 const useWidthOf = <T extends ReferenceObject>(element: T | null) => {
  const [menuWidth, setMenuWidth] = useState<string | undefined>()

+  const parent = element?.offsetParent


I would rename it

havenchyk

I would fix what Oleksandr found on Form and we're good to go 👍

packages/picasso-lab/src/TreeView/useNodes.ts

toptal-devbot · 2021-03-30T13:13:01Z

🎉 Last commit is successfully deployed 🎉

Demo is available on:

https://picasso.toptal.net/fix-deps/

Your davinci-bot 🚀

borisyordanov requested a review from a team as a code owner March 18, 2021 08:19

borisyordanov changed the title ~~fix: react-hooks/exhaustive-deps~~ chore: resolve react-hooks/exhaustive-deps warnings Mar 18, 2021

borisyordanov marked this pull request as draft March 18, 2021 08:41

borisyordanov marked this pull request as ready for review March 18, 2021 09:24

borisyordanov force-pushed the fix-deps branch from 1f953aa to 34d7808 Compare March 18, 2021 12:07

piotrd approved these changes Mar 18, 2021

View reviewed changes

OleksandrNechai reviewed Mar 18, 2021

View reviewed changes

denieler reviewed Mar 19, 2021

View reviewed changes

packages/picasso-lab/src/TreeView/useZoom.ts Show resolved Hide resolved

denieler reviewed Mar 19, 2021

View reviewed changes

borisyordanov requested review from OleksandrNechai and denieler March 22, 2021 11:26

borisyordanov force-pushed the fix-deps branch 2 times, most recently from 190ba99 to e1613f6 Compare March 24, 2021 19:21

denieler reviewed Mar 29, 2021

View reviewed changes

havenchyk approved these changes Mar 29, 2021

View reviewed changes

denieler reviewed Mar 30, 2021

View reviewed changes

packages/picasso-lab/src/TreeView/useNodes.ts Outdated Show resolved Hide resolved

Boris Yordanov added 5 commits March 30, 2021 12:15

fix: react-hooks/exhaustive-deps

67c0cd3

fix: breakpoints

38ebfea

fix: type error

0f273b4

fix: tree nodes

1d32df2

fix: usecallback

7a550ce

denieler approved these changes Mar 30, 2021

View reviewed changes

chore: update davinci and browserslsit

898ef6d

Boris Yordanov added 6 commits March 30, 2021 12:44

chore: remove eslint comment

b564a1c

chore: yarn dedup

0a511ef

chore: remove lint comment

dad2618

chore: yarn lock update

2ea8ce8

chore: rename var

a9b1c91

chore: improve code

5be13ae

borisyordanov force-pushed the fix-deps branch from 2f3008f to 5be13ae Compare March 30, 2021 11:44

Boris Yordanov added 2 commits March 30, 2021 13:04

chore: remove usecallbacks

6065471

chore: remove usecallbacks

a250984

borisyordanov merged commit bc0f242 into master Mar 30, 2021

borisyordanov deleted the fix-deps branch March 30, 2021 13:34

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

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

Uh oh!

Conversation

borisyordanov commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to test

Screenshots

Review

Uh oh!

borisyordanov commented Mar 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

denieler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

havenchyk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toptal-devbot commented Mar 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

borisyordanov commented Mar 18, 2021 •

edited

Loading