From c8deaeacce6ee34c887a9971dfe6417c5a4126d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Bedi?= Date: Wed, 24 Feb 2021 17:21:47 +0100 Subject: [PATCH] Chore: Fix eslint react hook warnings in grafana-ui (#31092) * Chore: Fix eslint react hook warnings in grafana-ui * Remove unneeded exlcudes * Address Andrej's review --- .eslintrc | 3 +-- .../CustomScrollbar/CustomScrollbar.tsx | 22 ++++++++-------- .../DataSourceSettings/SigV4AuthSettings.tsx | 1 + .../MatchersUI/FieldNamesMatcherEditor.tsx | 12 ++++----- .../grafana-ui/src/components/Modal/Modal.tsx | 13 +++++----- .../src/components/NodeGraph/NodeGraph.tsx | 9 ++++--- .../src/components/NodeGraph/useNodeLimit.ts | 2 +- .../src/components/NodeGraph/usePanning.ts | 17 +++++++------ .../src/components/NodeGraph/useZoom.ts | 25 +++++++++++-------- .../src/components/Slider/Slider.tsx | 8 +++--- .../src/components/VizLayout/VizLayout.tsx | 5 ++-- 11 files changed, 63 insertions(+), 54 deletions(-) diff --git a/.eslintrc b/.eslintrc index 518210d0f62..01ec2d6a0d1 100644 --- a/.eslintrc +++ b/.eslintrc @@ -11,8 +11,7 @@ "overrides": [ { "files": [ - "packages/grafana-ui/**/*/!(*.story).{ts,tsx}", - "packages/jaeger-ui-components/**/*.{ts,tsx,js}", + "packages/grafana-ui/src/components/uPlot/**/*.{ts,tsx}", "public/app/**/*.{ts,tsx}" ], "rules": { diff --git a/packages/grafana-ui/src/components/CustomScrollbar/CustomScrollbar.tsx b/packages/grafana-ui/src/components/CustomScrollbar/CustomScrollbar.tsx index 509a3681a4f..fd0071128e0 100644 --- a/packages/grafana-ui/src/components/CustomScrollbar/CustomScrollbar.tsx +++ b/packages/grafana-ui/src/components/CustomScrollbar/CustomScrollbar.tsx @@ -54,16 +54,18 @@ export const CustomScrollbar: FC = ({ * Special logic for doing a update a few milliseconds after mount to check for * updated height due to dynamic content */ - if (updateAfterMountMs) { - useEffect(() => { - setTimeout(() => { - const scrollbar = ref.current as any; - if (scrollbar?.update) { - scrollbar.update(); - } - }, updateAfterMountMs); - }, []); - } + + useEffect(() => { + if (!updateAfterMountMs) { + return; + } + setTimeout(() => { + const scrollbar = ref.current as any; + if (scrollbar?.update) { + scrollbar.update(); + } + }, updateAfterMountMs); + }, [updateAfterMountMs]); function renderTrack(className: string, hideTrack: boolean | undefined, passedProps: any) { if (passedProps.style && hideTrack) { diff --git a/packages/grafana-ui/src/components/DataSourceSettings/SigV4AuthSettings.tsx b/packages/grafana-ui/src/components/DataSourceSettings/SigV4AuthSettings.tsx index 47abee16fa9..9edd7f7d62a 100644 --- a/packages/grafana-ui/src/components/DataSourceSettings/SigV4AuthSettings.tsx +++ b/packages/grafana-ui/src/components/DataSourceSettings/SigV4AuthSettings.tsx @@ -46,6 +46,7 @@ export const SigV4AuthSettings: React.FC = (props) => { useEffect(() => { const sigV4AuthType = dataSourceConfig.jsonData.sigV4AuthType || 'default'; onJsonDataChange('sigV4AuthType', sigV4AuthType); + // eslint-disable-next-line react-hooks/exhaustive-deps }, []); const onSecureJsonDataReset = (fieldName: string) => { diff --git a/packages/grafana-ui/src/components/MatchersUI/FieldNamesMatcherEditor.tsx b/packages/grafana-ui/src/components/MatchersUI/FieldNamesMatcherEditor.tsx index d0eca8d4c2d..5deb5132ef9 100644 --- a/packages/grafana-ui/src/components/MatchersUI/FieldNamesMatcherEditor.tsx +++ b/packages/grafana-ui/src/components/MatchersUI/FieldNamesMatcherEditor.tsx @@ -17,11 +17,6 @@ export const FieldNamesMatcherEditor = memo; - } - const onChange = useCallback( (selections: Array>) => { if (!Array.isArray(selections)) { @@ -39,9 +34,14 @@ export const FieldNamesMatcherEditor = memo; + } + return ; }); FieldNamesMatcherEditor.displayName = 'FieldNameMatcherEditor'; diff --git a/packages/grafana-ui/src/components/Modal/Modal.tsx b/packages/grafana-ui/src/components/Modal/Modal.tsx index e90a9202357..9fb0d171fe5 100644 --- a/packages/grafana-ui/src/components/Modal/Modal.tsx +++ b/packages/grafana-ui/src/components/Modal/Modal.tsx @@ -42,13 +42,12 @@ export function Modal(props: PropsWithChildren): ReturnType> { } }, [propsOnDismiss]); - const onEscKey = (ev: KeyboardEvent) => { - if (ev.key === 'Esc' || ev.key === 'Escape') { - onDismiss(); - } - }; - useEffect(() => { + const onEscKey = (ev: KeyboardEvent) => { + if (ev.key === 'Esc' || ev.key === 'Escape') { + onDismiss(); + } + }; if (isOpen && closeOnEscape) { document.addEventListener('keydown', onEscKey, false); } else { @@ -57,7 +56,7 @@ export function Modal(props: PropsWithChildren): ReturnType> { return () => { document.removeEventListener('keydown', onEscKey, false); }; - }, [closeOnEscape, isOpen]); + }, [closeOnEscape, isOpen, onDismiss]); if (!isOpen) { return null; diff --git a/packages/grafana-ui/src/components/NodeGraph/NodeGraph.tsx b/packages/grafana-ui/src/components/NodeGraph/NodeGraph.tsx index 400470236b8..9adc9cef720 100644 --- a/packages/grafana-ui/src/components/NodeGraph/NodeGraph.tsx +++ b/packages/grafana-ui/src/components/NodeGraph/NodeGraph.tsx @@ -84,11 +84,14 @@ export function NodeGraph({ getLinks, dataFrames, nodeLimit }: Props) { const [edgeHover, setEdgeHover] = useState(undefined); const clearEdgeHover = useCallback(() => setEdgeHover(undefined), [setEdgeHover]); + const firstNodesDataFrame = nodesDataFrames[0]; + const firstEdgesDataFrame = edgesDataFrames[0]; + // TODO we should be able to allow multiple dataframes for both edges and nodes, could be issue with node ids which in // that case should be unique or figure a way to link edges and nodes dataframes together. - const processed = useMemo(() => processNodes(nodesDataFrames[0], edgesDataFrames[0]), [ - nodesDataFrames[0], - edgesDataFrames[0], + const processed = useMemo(() => processNodes(firstNodesDataFrame, firstEdgesDataFrame), [ + firstEdgesDataFrame, + firstNodesDataFrame, ]); const { nodes: rawNodes, edges: rawEdges } = useNodeLimit(processed.nodes, processed.edges, nodeCountLimit); diff --git a/packages/grafana-ui/src/components/NodeGraph/useNodeLimit.ts b/packages/grafana-ui/src/components/NodeGraph/useNodeLimit.ts index 17f1adbd535..3acbc7e3d0b 100644 --- a/packages/grafana-ui/src/components/NodeGraph/useNodeLimit.ts +++ b/packages/grafana-ui/src/components/NodeGraph/useNodeLimit.ts @@ -46,5 +46,5 @@ export function useNodeLimit( const newEdges = edges.filter((e) => newNodes[e.source as string] && newNodes[e.target as string]); return { nodes: Object.values(newNodes), edges: newEdges }; - }, [edges, nodes]); + }, [edges, limit, nodes]); } diff --git a/packages/grafana-ui/src/components/NodeGraph/usePanning.ts b/packages/grafana-ui/src/components/NodeGraph/usePanning.ts index 8622febb6a9..e0b03476556 100644 --- a/packages/grafana-ui/src/components/NodeGraph/usePanning.ts +++ b/packages/grafana-ui/src/components/NodeGraph/usePanning.ts @@ -18,7 +18,7 @@ interface Options { * Based on https://github.com/streamich/react-use/blob/master/src/useSlider.ts * Returns position x/y coordinates which can be directly used in transform: translate(). * @param scale Can be used when we want to scale the movement if we are moving a scaled element. We need to do it - * here because we don't wont to change the pos when scale changes. + * here because we don't want to change the pos when scale changes. * @param bounds If set the panning cannot go outside of those bounds. */ export function usePanning( @@ -105,17 +105,18 @@ export function usePanning( }); }; - if (panRef.current) { - panRef.current.addEventListener('mousedown', onPanStart); - panRef.current.addEventListener('touchstart', onPanStart); + const ref = panRef.current; + if (ref) { + ref.addEventListener('mousedown', onPanStart); + ref.addEventListener('touchstart', onPanStart); } return () => { - if (panRef.current) { - panRef.current.removeEventListener('mousedown', onPanStart); - panRef.current.removeEventListener('touchstart', onPanStart); + if (ref) { + ref.removeEventListener('mousedown', onPanStart); + ref.removeEventListener('touchstart', onPanStart); } }; - }, [scale, bounds?.left, bounds?.right, bounds?.top, bounds?.bottom]); + }, [scale, bounds?.left, bounds?.right, bounds?.top, bounds?.bottom, isMounted]); return { state, ref: panRef }; } diff --git a/packages/grafana-ui/src/components/NodeGraph/useZoom.ts b/packages/grafana-ui/src/components/NodeGraph/useZoom.ts index 52bce9e016c..0c157f41943 100644 --- a/packages/grafana-ui/src/components/NodeGraph/useZoom.ts +++ b/packages/grafana-ui/src/components/NodeGraph/useZoom.ts @@ -64,18 +64,21 @@ export function useZoom({ stepUp, stepDown, min, max } = defaultOptions) { ); useEffect(() => { - if (ref.current) { - // Adds listener for wheel event, we need the passive: false to be able to prevent default otherwise that - // cannot be used with passive listeners. - ref.current.addEventListener('wheel', onWheel, { passive: false }); - return () => { - if (ref.current) { - ref.current.removeEventListener('wheel', onWheel); - } - }; + if (!ref.current) { + return; } - return undefined; - }, [ref.current, onWheel]); + + const zoomRef = ref.current; + + // Adds listener for wheel event, we need the passive: false to be able to prevent default otherwise that + // cannot be used with passive listeners. + zoomRef.addEventListener('wheel', onWheel, { passive: false }); + return () => { + if (zoomRef) { + zoomRef.removeEventListener('wheel', onWheel); + } + }; + }, [onWheel]); return { onStepUp, diff --git a/packages/grafana-ui/src/components/Slider/Slider.tsx b/packages/grafana-ui/src/components/Slider/Slider.tsx index ddd23005a62..8cae961db4d 100644 --- a/packages/grafana-ui/src/components/Slider/Slider.tsx +++ b/packages/grafana-ui/src/components/Slider/Slider.tsx @@ -24,7 +24,7 @@ export const Slider: FunctionComponent = ({ const theme = useTheme(); const styles = getStyles(theme, isHorizontal); const SliderWithTooltip = SliderComponent; - const [slidervalue, setSliderValue] = useState(value || min); + const [sliderValue, setSliderValue] = useState(value || min); const onSliderChange = useCallback( (v: number) => { @@ -58,7 +58,7 @@ export const Slider: FunctionComponent = ({ onAfterChange(v); } }, - [setSliderValue, onAfterChange] + [max, min, onChange, onAfterChange] ); const sliderInputClassNames = !isHorizontal ? [styles.sliderInputVertical] : []; @@ -74,7 +74,7 @@ export const Slider: FunctionComponent = ({ max={max} step={step} defaultValue={value} - value={slidervalue} + value={sliderValue} onChange={onSliderChange} onAfterChange={onAfterChange} vertical={!isHorizontal} @@ -84,7 +84,7 @@ export const Slider: FunctionComponent = ({ {children(width, height)}; } const { placement, maxHeight, maxWidth } = legend.props; - const [legendRef, legendMeasure] = useMeasure(); + let size: VizSize | null = null; const vizStyle: CSSProperties = { @@ -64,7 +65,7 @@ export const VizLayout: VizLayoutComponentType = ({ width, height, legend, child } // This happens when position is switched from bottom to right - // Then we preserve old with for one render cycle until lenged is measured in it's new position + // Then we preserve old with for one render cycle until legend is measured in it's new position if (size?.width === 0) { size.width = width; }