-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
@toptal-bot run visual |
const handleChangeDebounced = useCallback( | ||
debounce(async (inputValue: string) => { | ||
const newOptions = await loadOptions(inputValue.trim().toLowerCase()) | ||
const handleChangeDebounced = debounce(async (inputValue: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
const showErrorNotification = (errors: SubmissionErrors) => { | ||
if (typeof errors === 'string') { | ||
showError(errors, undefined, { persist: true }) | ||
const showErrorNotification = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably agree, it will hide the issue
@@ -84,6 +84,7 @@ export const useNodes = ( | |||
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@@ -25,6 +25,7 @@ export const useZoom = <ZoomRefElement extends ZoomedElementBaseType>({ | |||
const [initialized, setInitialized] = useState(false) | |||
const zoom = useMemo( | |||
() => d3.zoom<ZoomRefElement, unknown>().scaleExtent(scaleExtent), | |||
// eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because eslint adds ZoomRefElement
as a dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not there a way to fix it properly? It's just type, right?
) | ||
setLoading(false) | ||
setOptions(newOptions) | ||
}, 500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. This is not debounced any more
@@ -8,14 +8,16 @@ export interface ReferenceObject { | |||
const useWidthOf = <T extends ReferenceObject>(element: T | null) => { | |||
const [menuWidth, setMenuWidth] = useState<string | undefined>() | |||
|
|||
const parent = element?.offsetParent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is parent
accurate name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is parentElement
okay?
@@ -186,6 +186,7 @@ export const useScreens = <T = unknown>() => { | |||
|
|||
return defaultValue | |||
}, | |||
// eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit dangerous change. It might break some behaviour in some hooks 😃 But probably better to fix this rule, break and fix later 👍
190ba99
to
e1613f6
Compare
const showErrorNotification = (errors: SubmissionErrors) => { | ||
if (typeof errors === 'string') { | ||
showError(errors, undefined, { persist: true }) | ||
const showErrorNotification = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably agree, it will hide the issue
} | ||
}, [selectedNode, selectedNode?.data]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was interesting 😁
if (!initialized) { | ||
return updateNodesYPosition(dynamicNodes) | ||
} | ||
|
||
return updateNodesYPosition(dynamicNodes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are those 2 different? look the same
@@ -8,14 +8,16 @@ export interface ReferenceObject { | |||
const useWidthOf = <T extends ReferenceObject>(element: T | null) => { | |||
const [menuWidth, setMenuWidth] = useState<string | undefined>() | |||
|
|||
const parent = element?.offsetParent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would fix what Oleksandr found on Form and we're good to go 👍
🎉 Last commit is successfully deployed 🎉 Demo is available on: Your davinci-bot 🚀 |
FX-1777
Description
Describe the changes and motivations for the pull request.
How to test
yarn lint --rule "{react-hooks/exhaustive-deps: error}" --quiet
, it should return 0 errorsScreenshots
Review
props
in component with documentationexamples
for componentyarn test
yarn test:visual
. If not - check the documentation how to fix visual testsPR 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