chore(eslint): make react-hooks/exhaustive-deps rule as an error

.eslintrc.js

@@ -23,6 +23,7 @@ module.exports = {
   rules: {
     'prettier/prettier': 'error',
     'react-hooks/rules-of-hooks': 'error',
+    'react-hooks/exhaustive-deps': 'error',
     'react/prop-types': 'off'
   },
   settings: {
@@ -51,7 +52,12 @@ module.exports = {
         'plugin:@typescript-eslint/recommended',
         'plugin:react-hooks/recommended',
         'prettier'
-      ]
+      ],
+      rules: {
+        'prettier/prettier': 'error',
+        'react-hooks/rules-of-hooks': 'error',
+        'react-hooks/exhaustive-deps': 'error'
+      }
     }
   ]
 };

app/javascript/components/ComboMultiple.jsx

@@ -26,6 +26,19 @@ import { useDeferredSubmit, useHiddenField } from './shared/hooks';
 
 const Context = createContext();
 
+const optionValueByLabel = (values, options, label) => {
+  const maybeOption = values.includes(label)
+    ? [label, label]
+    : options.find(([optionLabel]) => optionLabel == label);
+  return maybeOption ? maybeOption[1] : undefined;
+};
+const optionLabelByValue = (values, options, value) => {
+  const maybeOption = values.includes(value)
+    ? [value, value]
+    : options.find(([, optionValue]) => optionValue == value);
+  return maybeOption ? maybeOption[0] : undefined;
+};
+
 function ComboMultiple({
   options,
   id,
@@ -40,9 +53,6 @@ function ComboMultiple({
   invariant(id || label, 'ComboMultiple: `id` or a `label` are required');
   invariant(group, 'ComboMultiple: `group` is required');
 
-  if (!Array.isArray(options[0])) {
-    options = options.filter((o) => o).map((o) => [o, o]);
-  }
   const inputRef = useRef();
   const [term, setTerm] = useState('');
   const [selections, setSelections] = useState(selected);
@@ -51,25 +61,22 @@ function ComboMultiple({
   const removedLabelledby = `${inputId}-remove`;
   const selectedLabelledby = `${inputId}-selected`;
 
-  const optionValueByLabel = (label) => {
-    const maybeOption = newValues.includes(label)
-      ? [label, label]
-      : options.find(([optionLabel]) => optionLabel == label);
-    return maybeOption ? maybeOption[1] : undefined;
-  };
-  const optionLabelByValue = (value) => {
-    const maybeOption = newValues.includes(value)
-      ? [value, value]
-      : options.find(([, optionValue]) => optionValue == value);
-    return maybeOption ? maybeOption[0] : undefined;
-  };
-
+  const optionsWithLabels = useMemo(
+    () =>
+      Array.isArray(options[0])
+        ? options
+        : options.filter((o) => o).map((o) => [o, o]),
+    [options]
+  );
   const extraOptions = useMemo(
     () =>
-      acceptNewValues && term && term.length > 2 && !optionLabelByValue(term)
+      acceptNewValues &&
+      term &&
+      term.length > 2 &&
+      !optionLabelByValue(newValues, optionsWithLabels, term)
         ? [[term, term]]
         : [],
-    [acceptNewValues, term, newValues.join(',')]
+    [acceptNewValues, term, optionsWithLabels, newValues]
   );
   const results = useMemo(
     () =>
@@ -77,12 +84,12 @@ function ComboMultiple({
         ...extraOptions,
         ...(term
           ? matchSorter(
-              options.filter(([label]) => !label.startsWith('--')),
+              optionsWithLabels.filter(([label]) => !label.startsWith('--')),
               term
             )
-          : options)
+          : optionsWithLabels)
       ].filter(([, value]) => !selections.includes(value)),
-    [term, selections.join(','), newValues.join(',')]
+    [term, selections, extraOptions, optionsWithLabels]
   );
   const [, setHiddenFieldValue, hiddenField] = useHiddenField(group, name);
   const awaitFormSubmit = useDeferredSubmit(hiddenField);
@@ -100,7 +107,7 @@ function ComboMultiple({
   };
 
   const onSelect = (value) => {
-    const maybeValue = [...extraOptions, ...options].find(
+    const maybeValue = [...extraOptions, ...optionsWithLabels].find(
       ([val]) => val == value
     );
     const selectedValue = maybeValue && maybeValue[1];
@@ -128,7 +135,7 @@ function ComboMultiple({
   };
 
   const onRemove = (label) => {
-    const optionValue = optionValueByLabel(label);
+    const optionValue = optionValueByLabel(newValues, options, label);
     if (optionValue) {
       saveSelection((selections) =>
         selections.filter((value) => value != optionValue)
@@ -149,7 +156,9 @@ function ComboMultiple({
     ) {
       if (
         term &&
-        [...extraOptions, ...options].map(([label]) => label).includes(term)
+        [...extraOptions, ...optionsWithLabels]
+          .map(([label]) => label)
+          .includes(term)
       ) {
         event.preventDefault();
         onSelect(term);
@@ -172,7 +181,9 @@ function ComboMultiple({
   const onBlur = () => {
     const shouldSelect =
       term &&
-      [...extraOptions, ...options].map(([label]) => label).includes(term);
+      [...extraOptions, ...optionsWithLabels]
+        .map(([label]) => label)
+        .includes(term);
 
     awaitFormSubmit(() => {
       if (shouldSelect) {
@@ -199,7 +210,7 @@ function ComboMultiple({
             <ComboboxToken
               key={selection}
               describedby={removedLabelledby}
-              value={optionLabelByValue(selection)}
+              value={optionLabelByValue(newValues, options, selection)}
             />
           ))}
         </ul>

app/javascript/components/ComboSearch.tsx

@@ -1,4 +1,4 @@
-import React, { useState, useCallback, useRef } from 'react';
+import React, { useState, useRef, ChangeEventHandler } from 'react';
 import { useDebounce } from 'use-debounce';
 import { useQuery } from 'react-query';
 import {
@@ -68,7 +68,7 @@ function ComboSearch<Result>({
     const [, value, label] = transformResult(result);
     return label ?? value;
   };
-  const setExternalValueAndId = useCallback((label: string) => {
+  const setExternalValueAndId = (label: string) => {
     const { key, value, result } = resultsMap.current[label];
     if (onChange) {
       onChange(value, result);
@@ -76,36 +76,35 @@ function ComboSearch<Result>({
       setExternalId(key);
       setExternalValue(value);
     }
-  }, []);
+  };
   const awaitFormSubmit = useDeferredSubmit(hiddenField);
 
-  const handleOnChange = useCallback(
-    ({ target: { value } }) => {
-      setValue(value);
-      if (!value) {
-        if (onChange) {
-          onChange(null);
-        } else {
-          setExternalId('');
-          setExternalValue('');
-        }
-      } else if (value.length >= minimumInputLength) {
-        setSearchTerm(value.trim());
-        if (allowInputValues) {
-          setExternalId('');
-          setExternalValue(value);
-        }
+  const handleOnChange: ChangeEventHandler<HTMLInputElement> = ({
+    target: { value }
+  }) => {
+    setValue(value);
+    if (!value) {
+      if (onChange) {
+        onChange(null);
+      } else {
+        setExternalId('');
+        setExternalValue('');
       }
-    },
-    [minimumInputLength]
-  );
+    } else if (value.length >= minimumInputLength) {
+      setSearchTerm(value.trim());
+      if (allowInputValues) {
+        setExternalId('');
+        setExternalValue(value);
+      }
+    }
+  };
 
-  const handleOnSelect = useCallback((value: string) => {
+  const handleOnSelect = (value: string) => {
     setExternalValueAndId(value);
     setValue(value);
     setSearchTerm('');
     awaitFormSubmit.done();
-  }, []);
+  };
 
   const { isSuccess, data } = useQuery<void, void, unknown, QueryKey>(
     [scope, debouncedSearchTerm, scopeExtra],
@@ -117,14 +116,14 @@ function ComboSearch<Result>({
   const results =
     isSuccess && data ? transformResults(debouncedSearchTerm, data) : [];
 
-  const onBlur = useCallback(() => {
+  const onBlur = () => {
     if (!allowInputValues && isSuccess && results[0]) {
       const label = getLabel(results[0]);
       awaitFormSubmit(() => {
         handleOnSelect(label);
       });
     }
-  }, [data]);
+  };
 
   return (
     <Combobox onSelect={handleOnSelect}>

app/javascript/components/MapEditor/components/CadastreLayer.tsx

@@ -28,35 +28,41 @@ export function CadastreLayer({
   const map = useMapLibre();
   const selectedCadastresRef = useRef(new Set<string>());
 
-  const highlightFeature = useCallback((cid: string, highlight: boolean) => {
-    if (highlight) {
-      selectedCadastresRef.current.add(cid);
-    } else {
-      selectedCadastresRef.current.delete(cid);
-    }
-    if (selectedCadastresRef.current.size == 0) {
-      map.setFilter('parcelle-highlighted', ['in', 'id', '']);
-    } else {
-      map.setFilter('parcelle-highlighted', [
-        'in',
-        'id',
-        ...selectedCadastresRef.current
-      ]);
-    }
-  }, []);
+  const highlightFeature = useCallback(
+    (cid: string, highlight: boolean) => {
+      if (highlight) {
+        selectedCadastresRef.current.add(cid);
+      } else {
+        selectedCadastresRef.current.delete(cid);
+      }
+      if (selectedCadastresRef.current.size == 0) {
+        map.setFilter('parcelle-highlighted', ['in', 'id', '']);
+      } else {
+        map.setFilter('parcelle-highlighted', [
+          'in',
+          'id',
+          ...selectedCadastresRef.current
+        ]);
+      }
+    },
+    [map]
+  );
 
-  const hoverFeature = useCallback((feature: Feature, hover: boolean) => {
-    if (!selectedCadastresRef.current.has(feature.properties?.id)) {
-      map.setFeatureState(
-        {
-          source: 'cadastre',
-          sourceLayer: 'parcelles',
-          id: String(feature.id)
-        },
-        { hover }
-      );
-    }
-  }, []);
+  const hoverFeature = useCallback(
+    (feature: Feature, hover: boolean) => {
+      if (!selectedCadastresRef.current.has(feature.properties?.id)) {
+        map.setFeatureState(
+          {
+            source: 'cadastre',
+            sourceLayer: 'parcelles',
+            id: String(feature.id)
+          },
+          { hover }
+        );
+      }
+    },
+    [map]
+  );
 
   useCadastres(featureCollection, {
     hoverFeature,

app/javascript/components/MapEditor/components/DrawLayer.tsx

@@ -68,7 +68,9 @@ export function DrawLayer({
         drawRef.current = null;
       }
     };
-  }, [enabled]);
+    // We only want to rerender draw layer on component mount or when the layer is toggled.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [map, enabled]);
 
   const onSetId = useCallback(({ detail }) => {
     drawRef.current?.setFeatureProperty(detail.lid, 'id', detail.id);
@@ -167,7 +169,9 @@ function useExternalEvents(
 
   useEffect(() => {
     fitBounds(featureCollection.bbox as LngLatBoundsLike);
-  }, []);
+    // We only want to zoom on bbox on component mount.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [fitBounds]);
 
   useEvent('map:feature:focus', onFeatureFocus);
   useEvent('map:feature:create', onFeatureCreate);

app/javascript/components/MapReader/components/GeoJSONLayer.tsx

@@ -44,13 +44,13 @@ export function GeoJSONLayer({
         popup.remove();
       }
     },
-    [popup]
+    [map, popup]
   );
 
   const onMouseLeave = useCallback(() => {
     map.getCanvas().style.cursor = '';
     popup.remove();
-  }, [popup]);
+  }, [map, popup]);
 
   useExternalEvents(featureCollection);
 
@@ -99,17 +99,22 @@ export function GeoJSONLayer({
 
 function useExternalEvents(featureCollection: FeatureCollection) {
   const fitBounds = useFitBounds();
-  const onFeatureFocus = useCallback(({ detail }) => {
-    const { id } = detail;
-    const feature = findFeature(featureCollection, id);
-    if (feature) {
-      fitBounds(getBounds(feature.geometry));
-    }
-  }, []);
+  const onFeatureFocus = useCallback(
+    ({ detail }) => {
+      const { id } = detail;
+      const feature = findFeature(featureCollection, id);
+      if (feature) {
+        fitBounds(getBounds(feature.geometry));
+      }
+    },
+    [featureCollection, fitBounds]
+  );
 
   useEffect(() => {
     fitBounds(featureCollection.bbox as LngLatBoundsLike);
-  }, []);
+    // We only want to zoom on bbox on component mount.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [fitBounds]);
 
   useEvent('map:feature:focus', onFeatureFocus);
 }
@@ -139,7 +144,7 @@ function LineStringLayer({
         type: 'line',
         paint: lineStringSelectionLine
       });
-  }, []);
+  }, [map, layerId, sourceId, feature]);
 
   useMapEvent('mouseenter', onMouseEnter, layerId);
   useMapEvent('mouseleave', onMouseLeave, layerId);
@@ -172,7 +177,7 @@ function PointLayer({
         type: 'circle',
         paint: pointSelectionCircle
       });
-  }, []);
+  }, [map, layerId, sourceId, feature]);
 
   useMapEvent('mouseenter', onMouseEnter, layerId);
   useMapEvent('mouseleave', onMouseLeave, layerId);
@@ -212,7 +217,7 @@ function PolygonLayer({
         type: 'fill',
         paint: polygonSelectionFill
       });
-  }, []);
+  }, [map, layerId, lineLayerId, sourceId, feature]);
 
   useMapEvent('mouseenter', onMouseEnter, layerId);
   useMapEvent('mouseleave', onMouseLeave, layerId);

app/javascript/components/shared/maplibre/MapLibre.tsx

@@ -5,7 +5,8 @@ import React, {
   useEffect,
   useMemo,
   ReactNode,
-  createContext
+  createContext,
+  useCallback
 } from 'react';
 import maplibre, { Map, Style, NavigationControl } from 'maplibre-gl';
 
@@ -37,11 +38,14 @@ export function MapLibre({ children, header, footer, layers }: MapLibreProps) {
   const containerRef = useRef<HTMLDivElement>(null);
   const [map, setMap] = useState<Map | null>();
 
-  const onStyleChange = (style: Style) => {
-    if (map) {
-      map.setStyle(style);
-    }
-  };
+  const onStyleChange = useCallback(
+    (style: Style) => {
+      if (map) {
+        map.setStyle(style);
+      }
+    },
+    [map]
+  );
   const { style, ...mapStyleProps } = useStyle(layers, onStyleChange);
 
   useEffect(() => {
@@ -56,7 +60,7 @@ export function MapLibre({ children, header, footer, layers }: MapLibreProps) {
         setMap(map);
       });
     }
-  }, []);
+  }, [map, style, isSupported]);
 
   if (!isSupported) {
     return (

app/javascript/components/shared/maplibre/hooks.ts

@@ -12,16 +12,22 @@ import { useMapLibre } from './MapLibre';
 
 export function useFitBounds() {
   const map = useMapLibre();
-  return useCallback((bbox: LngLatBoundsLike) => {
-    map.fitBounds(bbox, { padding: 100 });
-  }, []);
+  return useCallback(
+    (bbox: LngLatBoundsLike) => {
+      map.fitBounds(bbox, { padding: 100 });
+    },
+    [map]
+  );
 }
 
 export function useFlyTo() {
   const map = useMapLibre();
-  return useCallback((zoom: number, center: [number, number]) => {
-    map.flyTo({ zoom, center });
-  }, []);
+  return useCallback(
+    (zoom: number, center: [number, number]) => {
+      map.flyTo({ zoom, center });
+    },
+    [map]
+  );
 }
 
 export function useEvent(eventName: string, callback: EventListener) {
@@ -104,7 +110,7 @@ export function useStyle(
     [styleId, enabledLayers]
   );
 
-  useEffect(() => onStyleChange(style), [style]);
+  useEffect(() => onStyleChange(style), [onStyleChange, style]);
 
   return { style, layers, setStyle, setLayerEnabled, setLayerOpacity };
 }