code_review.md

Code Review
===========

Note : les points redondants entre différents fichiers ne sont pas forcément ré-expliqués.

## App.jsx

Redirection redondante dans la configuration du routeur :

```jsx
<Redirect from="/" to="/home" />
<Redirect from="*" to="/home" />
```

## CardsContainer.tsx

### Usage non recommandé de `any` :

```tsx
  const [elecCardProps, setElecCardProps] = useState<any>({
    consumptionValue: null,
    weeklyCompareValue: null,
  })
```

Il vaut mieux définir un type commun dans `fluidService.ts` par exemple :

```tsx 
// fluidService.ts
export type ProcessedData = {
  consumptionValue: number;
  weeklyCompareValue: number;
  euclideanComparator: string;
}

// Card.tsx
export interface CardProps extends ProcessedData {
  type: FluidType
  t: any
}

// CardsContainer.tsx
const [elecCardProps, setElecCardProps] = useState<ProcessedData|null>(null)

// Dans le render :
{elecCardProps && <Card
  type={FluidType.ELECTRICITY}
  {...elecCardProps}
/>}
```

### Instanciation des services

Il vaut mieux instancier les services dans les effets où on les utilise, pour éviter d'en instancier un de nouveau à chaque render, alors que l'effet l'utilisant n'est exécuté qu'une seule fois, au premier render, via le tableau de dépendances vide `[]` :

```tsx
useEffect(() => {
  const fluidService = new FluidService(client)
  // ...
}, [])
```

### `async/await` dans un effet

Un effet `async` n'est pas évident, car pendant qu'on attend un retour, il se peut que le composant soit re-render entre-temps, voire détruit.

Du coup on aurait une `Promise` résolue, et les `set...CardProps` appelés alors que l'élément n'existe plus, généreront une erreur.

Pour éviter cela il faut rajouter un flag mis à jour lors du unsubscribe de l'effet :

```tsx
useEffect(() => {
  const fluidService = new FluidService(client)
  let subscribed = true

  async function getData() {
    const elecData = await fluidService.getElectricityLastTwoWeeks()
    const gasData = await fluidService.getGasLastTwoWeeks()
    const waterData = await fluidService.getWaterLastTwoWeeks()

    if (subscribed) {
      setElecCardProps(fluidService.processElectricityData(elecData.data))
      setGasCardProps(fluidService.processGasData(gasData.data))
      setWaterCardProps(fluidService.processWaterData(waterData.data))
      setIsLoading(false)
    }
  }

  getData()

  // Fonction unsubscribe, appelée avant le prochain render ou avant la destruction.
  return () => {
    subscribed = false
  }
}, [])
```

### Multiples `await`

Dernière chose pour optimiser les appels : quand on chaine des await, ils sont exécutés en séquence.

Pour les appeler en parallèle, on peut utiliser `Promise.all()` couplé à de la deconstruction.

Avant :

```tsx
const elecData = await fluidService.getElectricityLastTwoWeeks()
const gasData = await fluidService.getGasLastTwoWeeks()
const waterData = await fluidService.getWaterLastTwoWeeks()
```

Après :

```tsx
const [elecData, gasData, waterData] = await Promise.all([
  fluidService.getElectricityLastTwoWeeks(),
  fluidService.getGasLastTwoWeeks(),
  fluidService.getWaterLastTwoWeeks(),
])
```

## KonnectorViewerList.tsx

### Déclaration des fonctions utilisées par un effect

De la même façon que les services doivent être instanciés dans l'effect, il vaut mieux déclarer les fonctions utilisées par un effect dans celui-ci, pour éviter de définir une nouvelle référence à chaque render. C'est le cas ici des fonctions `setAllKonnectors` et `setAllTriggers` :

*Noter l'utilisation d'un flag subscribed comme expliqué précédemment.*

```tsx
  useEffect(() => {
    let subscribed = true

    const setAllKonnectors = async () => {
      const kss = new KonnectorStatusService(client)
      const allKonnectors = await kss.getAllKonnectors()

      if (subscribed) {
        setKonnectors(allKonnectors)
      }
    }

    const setAllTriggers = async () => {
      const kss = new KonnectorStatusService(client)
      const allTriggers = await kss.getAllTriggers()

      if (subscribed) {
        setTriggers(allTriggers)
      }
    }

    setAllKonnectors()
    setAllTriggers()

    return () => {
      subscribed = false
    }
  }, [])
```

De même, la fonction `fetchKonnectorFromTrigger` pourrait être déplacée en dehors composant pour éviter de créer une nouvelle référence à chaque render.

### Valeur utilisée pour la `key` lors d'un `.map()``

Idéalement, la prop `key` doit avoir une valeur associée à l'item courant, plutôt que son index dans le tableau. En effet le tableau pourrait changer, et un index pourrait pointer vers un autre item que celui du render précédent.

La conséquence serait que le même élément serait utilisé pour render le nouvel item, sans qu'il soit détruit, générant potentiellement des effets de bords non souhaités (certains effect pourraient ne pas être exécutés par exemple).

Pour éviter ça, on peut utiliser en `key` une propriété unique de l'item s'il y en a une :

```tsx
<KonnectorViewer
  key={item.id}
  konnectorConfig={item}
  trigger={getLastTrigger(item)}
/>
```

## KonnectorConfigForm.tsx

### Ref

Le hook `useRef` peut être utilisé pour stocker n'importe quelle valeur, pour s'en servir pour un node HTML, on peut le déclarer comme suit :

Avant :

```tsx
const content: React.MutableRefObject<null> = useRef<null>(null)
```

Après :

```tsx
const content = useRef<HTMLDivElement>(null)
```

### Callbacks

Dans le formulaire, on peut directement passer au `onSubmit` la callback comme la signature est identique :

Avant :

```tsx
onSubmit={(e: React.FormEvent<HTMLFormElement>) => handleSubmit(e)}
```

Après :

```tsx
onSubmit={handleSubmit}
```

De plus, il est préférable quand on fourni des fonctions à des composants React (mais pas nécessaire lors des simples balises HTML) de mémoizer cette fonction pour fournir toujours la même fonction entre les renders.

On peut faire ceci via le hook `useCallback`, et en refactorisant le stockage du couple login/password, on peut faire ceci :

*Noter le tableau de dépendances, comme pour useEffect et useMemo, qui permet de préciser quand la référence de la fonction peut changer*

```tsx
const [{login, password}, setCredentials] = useState({
  login: '',
  password: '',
})

const handleChange = useCallback((e: React.ChangeEvent<HTMLInputElement>) => {
  // Il nous faut utiliser cette syntaxe, avec une fonction, pour récupérer la valeur précédente.
  // On ne peut utiliser les valeurs déconstruites plus haut car elles pourraient ne pas être à jour, React entassant une pile de callbacks avant de toutes les exécuter)
  // Il faut aussi extraire les valeurs avant car la fonction est asynchrone, et l'event peut avoir été détruit entretemps
  const {name, value} = e.target

  setCredentials(credentials => {
    return {
      ...credentials,
      [name]: value,
    }
  })
}, [])

<Field
  onChange={handleChange}
  name="login"
  value={login}
/>
```