Frontend: Amélioration diverses / Expérience développement

[pierre-gilles]

Salut les développeurs Gladys

Le projet « frontend » de Gladys 4 a été créé en 2018/2019, et certain points de la configuration deviennent vieillissant/plus adapté avec le recul.

J’aimerais qu’on discute ensemble des différentes choses qui peuvent être mise à jour pour que le code soit plus au goût du jour, et pour itérer sur ce qui a marché/pas bien marché dans le frontend.

Mettre à jour ESLint

Mettre à jour la configuration ESLint pour autoriser les dernières améliorations JS ES2023 (opérateur ?. par exemple, on en parlait avec @bertrandda sur sa dernière PR)

Ca me fait penser qu’il faudrait le faire sur le backend aussi car certaines erreurs liés à JSDoc sont impossible à comprendre, et dans une version plus récente c’est beaucoup plus compréhensibles !

Stopper l’utilisation des décorators @connect utilisé par unistore

C’était la mode à l’époque et au final unistore ne le recommande plus, c’était un « hack » autorisé par le transpiler, je ne crois même pas que ce soit dans la spec JS en 2023.

Fini :

Maintenant :

Il faut que j’ajoute une règle ESLint pour bannir ce comportement.

Restreindre grandement l’utilisation du store global unistore

Je l’avoue, c’est de ma faute, au début de la v4 je pensais qu’un store global serait une bonne idée, sûrement par manque d’expérience à l’époque en développement front React. Le résultat : on a eu de nombreux bugs liés au store global, car forcément des variables se polluaient entre elle inter-components.

Ma recommandation: Limiter l’utilisation du store aux variables « globales »: user, session, httpClient, et c’est tout.

Le reste doit être des states de classe, local à la classe. ça évite la pollution entre components.

De même, il faudrait voir si ESLint pourrait nous aider à prévenir le développeur.

Unification du comportement des styles

Le styling dans le frontend n’a pas nécessairement été enforce au niveau eslint, et c’est une erreur à mon sens.

Certains components utilisent du style inline (style={{}}), ce qui est une mauvaise pratique en terme de performance car un objet est re-créé à chaque render.

Aussi, ces styles sont difficilement modifiable pour gérer le responsive.

Ma recommandation: des fichiers de style en .css (un pour chaque feature en gros), qui sont importés dans le component.

Les media queries deviennent possible et permettent de gérer les petits/grands devices:

Plus de règles ESLint pour accélérer les reviews de PR

Je me suis rendu compte en faisant la review de la dernière PR de @bertrandda que je faisais souvent les mêmes remarques sur le code front dans les PRs (attention aux fonctions inline, style inline, etc…), et que ces feedbacks pourraient être dans ESLint pour que les reviews aillent plus vite

@contributors : Qu’en pensez-vous ? D’autres retours sur le frontend ?

[pierre-gilles]

Bon je me suis chauffé aujourd’hui, j’ai mis à jour ESLint server-side notamment côté JSDoc

Upgrade & fix eslint jsdoc server side

ça fait beaucoup de changements, mais les erreurs sont beaucoup plus claires maintenant

[pierre-gilles]

J’ai commencé une PR pour rajouter un warning sur l’utilisation du decorator @connect (et remplacer par l’appel de fonction)

113 warnings pour l’instant :

Travail de fourmi mais j’avance vite

Upgrade front Eslint, add warning on @connect decorator

[pierre-gilles]

Du coup pour l’instant j’ai rajouté 2 règles :

1. Un warning pour les styles inline
2. Une erreur pour les décorateurs @connect
3. Pour plus tard, mettre un gros warning sur l’utilisation du store.setState pour pousser à l’utilisation des setState locaux aux components

Tout est fixé dans cette PR : Upgrade front Eslint, add warning on @connect decorator by Pierre-Gilles · Pull Request #1766 · GladysAssistant/Gladys · GitHub

[pierre-gilles]

J’ai mergé ces 2 PRs !

Ceux qui travaillent sur des PRs Gladys en ce moment (@AlexTrovato, @bertrandda, @Pti_Nico, @euguuu, @Romuald_Pochet ), je vous recommande de rebase vos branches, faire un coup de npm install (server/front), et faire un coup d’Eslint pour vérifier que votre code est à jour

Si vous avez un souci pour faire les changements, n’hésitez pas !

[AlexTrovato]

Super, je soutiens ton action.
Maintenant à nous de faire en sorte de corriger les warning intégration par intégration, ou t’es chaud et tu t’en charges @pierre-gilles ?

[Romuald_Pochet]

Bonsoir,

J’ai commencé à inclure les modifications dans l’intégration zwave-js-ui et j’ai un soucis avec la méthode suivante:

/**
* @description Return array of Nodes.
* @param {string} orderDir - Ordering.
* @param {string} search - Keyword to match.
* @returns {Array} Return list of nodes.
* @example
* const nodes = zwaveManager.getNodes();
function getNodes({ orderDir = undefined, search = undefined } = {}) {

Suite à la mise à jour de SE Lint, j’ai l’erreur suivante:

  30:1  error  @param "search" does not match an existing function parameter  jsdoc/check-param-names

Une idée?

[pierre-gilles]

@Romuald_Pochet Oui, ce n’est pas des paramètres de fonction que tu décris, c’est un objet !

Il faut donc faire de cette manière :

D’ailleurs petit feedback, le = undefined n’est pas utile, car si tu ne spécifie pas de orderDir, ça sera undefined de toute façon

[pierre-gilles]

@AlexTrovato

Maintenant à nous de faire en sorte de corriger les warning intégration par intégration, ou t’es chaud et tu t’en charges @pierre-gilles ?

J’ai tout corrigé dans les 2 PRs hier

La seule chose que je n’ai pas faite, c’est d’activer la règle ESLint pour mettre un warning sur les utilisations de unistore (fichier actions.js), car ça créeait 600 warnings

A l’avenir, essayez de ne plus utiliser ces fichiers actions.js au profit des states locaux, et si vous avez un peu de temps, migrez progressivement les components existants Après, c’est pas non plus la priorité, c’est plus une guideline pour le futur

[AlexTrovato]

Je parlais d’aider à fixer les 146 warnings côté front, principalement sur l’utilisation de l’attribut « style » : « Using inline style is not recommended. Please use a .css file »
Il y a également les warning des promises pour « Prefer await to then()/catch()/finally() »

[pierre-gilles]

Ah effectivement ! Oui sur ceux là je suis preneur d’aide, après pour moi c’est pas une priorité de faire tout d’un coup, on peut faire en progressif au fil de l’eau

Pour moi il y a d’autres priorités de DEV en ce moment, typiquement le chantier Zigbee que t’as commencé pour sélectionner la marque de clé USB, c’est un truc avec plus de valeur ajouté pour l’utilisateur parce que ça bloque tous ceux qui commencent sur Gladys (vu que Domadoo ne vend plus que le Dongle Sonoff-E)