Refactor(pulicodes-state): code review + refactoring #93

EmileRolley · 2023-10-09T13:22:46Z

Closes incubateur-ademe/nosgestesclimat-site#1315

Code review - publicodes-state

J’avais en tête quelque chose du style https://publi.codes/docs/api/react-ui qui permettrait d’avoir un composant React PublicodesForm au quel on fournit un moteur et/ou un ensemble de règles et qui créer un formulaire à partir de celles-ci, mettant à jour une situation en fonction des réponses de l’utilisateur-ice. La gestion du state global de l’app serait alors laissée à l’utilisateur.ice — qui comprendrait dans le cas de NGC toute la gestion des users, actions, etc…

En l’état actuel, c’est un ensemble de hooks autour d’un state React. Ce qui est très bien pour isoler la logique métier, mais qui je pense est trop complexe et spécifique à NGC pour en faire une lib standalone.

Remarques

Contenu

🔴 La gestion de l’utilisateur est spécifique à NGC et ne devrait pas faire partie de publicodes-state.

🔴 A plusieurs endroits sont utilisés des noms de règle hardcodés du modèle NGC (ex. bilan et services sociétaux).

🟠 ❓ Entre useDisposableEngine et useEngine qui retourne deux moteurs différents, il est un peu compliqué de si retrouver et de savoir lequel utiliser. En particulier, pour le pristineEngine qui est utilisé à un seul endroit. Pourquoi ne pas utiliser useDisposableEngine avec une situation vide ?

🟠 Les actions sont pour l’instant assez spécifique à NGC donc je m’interroge sur la pertinence de garder useActions dans publicodes-state.

🟠 Un peu de doc pour dans chaque fichiers index ne serait pas de trop. De préférence au format tsdoc, ce qui permettrait de générer automatiquement une documentation de l’API facilement utilisable par d’autre dev.

Tech/TS

🔴 Le fichier types.d.ts mélange tous les types, là où il serait préférable de séparer ce qui concerne la gestion des simulations/users des types relatifs au modèle Publicodes.

🔴 ❓ De plus il y a une perte d’information par rapport au fichier publicodesUtils, dans lequel j’avais fais l’effort de rédiger un minimum de documentation et pourquoi les types ne sont pas réutilisés ?

🔴 any aliases (ex. Formule et Rules dans types.d.ts)

🟠 Chaque function possède un type Props pour ses arguments ce qui est vraiment bien 👍. En revanche, très peu possède un type de retour, ce qui est dommage 😕

🟠 ❓ Pourquoi ne pas utiliser la format d’import @/...comme pour le reste de l’app ?

🟠 Plusieurs fonctions possèdent le même nom ce qui rend complique la manipulation du code, comme par exemple useRules.

🟡 Utiliser root: string n’est pas forcément très parlant, rootRule: string ou root: RuleName serait déjà plus clair je trouve.

Général

🔴 Plusieurs casts passant par unknown

🔴 Problème de l'utilisation de useRule dans le composant ChoicesValue. Par exemple, on obtient l'erreur suivante à la page /simulateur/bilan:

UnknownRule: 
[ Règle inconnue ]
➡️  Dans la règle "logement . chauffage collectif . false"
⚠️  La règle 'logement . chauffage collectif . false' n'existe pas
    NextJS 40
2027-ea3f8dac202ef1ed.js:4:1828

Le problème vient d'ici :

nosgestesclimat-site-nextjs/src/components/misc/ChoicesValue.tsx

Line 9 in c676e90

const { title, icons } = useRule(question + ' . ' + value)

🟠 L’utilisation de yarn 3 est forcée, cependant, le yarn.lock ne fonctionne plus une fois regénéré

🟠 ❓ Pourquoi utilise-t-on à la fois axios et fetch ?

🟠 Ce pattern apparait plusieurs fois dans le code, il pourrait être factorisé dans un fonction avec un nom explicite :

nosgestesclimat-site-nextjs/src/app/(layout-with-navigation)/stats/_components/content/IframeFigures.js

Lines 60 to 62 in c676e90

    
           {line.entry_nb_visits 
        
             .toString() 
        
             .replace(/\B(?=(\d{3})+(?!\d))/g, '\u00A0')}

🟡 34 erreurs du type error TS2786: 'XXXX' cannot be used as a JSX component

🟡 ❓ Il y a-t-il une raison particulière pour forcer l’utilisation de Node 18 ?

🟡 Ce serait top de garder quelques console.debug pour pouvoir savoir combien de règles sont chargées, quand, le temps que ça a pris etc...

Questions

Tech/TS

Par rapport aux types relatifs au modèle Publicodes, il faudrait arriver à séparer ceux qui sont propres à NGC (et qui sont préfixé par NGC) de ceux génériques, utilisable par n’importe quel modèle Publicodes. Quel serait la manière de faire la plus appropriée ?
Est-ce que l’on a envie d’utiliser des composant Trans dans publicodes-state ? Je ne pense pas, et en tel cas, le useForm/index.ts n’a peut-être pas sa place dans publicodes-state.
Est-ce qu’il ne faudrait pas setup eslint et prettier dans la CI pour s’assurer de l’uniformité de la codebase ?

Changelog

New type: NGCQuestionType (+ use switch statement instead of if-then-else)

vercel · 2023-10-09T13:22:51Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nosgestesclimat	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 6, 2023 11:11am

Clemog · 2023-10-12T12:01:04Z

src/publicodes-state/simulationProvider/useCategories.ts

+  const categories = useMemo<string[]>(() => {
+    const sum = safeGetRule(root)?.rawNode?.formule?.somme
+    return order
+      ? // NOTE(@EmileRolley): what should be the wanted behavior if the order


Je me suis aussi posé la question ?

On ne compare rien ici. On ordonne juste la liste des catégories selon un paramètre order

cypress · 2023-10-13T16:54:44Z

2 failed tests on run #518 ↗︎

2	34	0	0	0

Details:

Merge `cf64546` into `c676e90`...
Project: Nos Gestes Climat (Next.js)	Commit: `592a070618 ℹ️`
Status: Failed	Duration: 03:57 💡
Started: Nov 6, 2023 11:11 AM	Ended: Nov 6, 2023 11:15 AM

cypress/e2e/integration/pages/groupes.cy.js • 2 failed tests • Test - e2e

View Output Video

Test		Artifacts
The Group creation page /amis/creer > allows to create a new group and displays it afterwards		`Test Replay` `Output` `Screenshots`
The Group creation page /amis/creer > allows to join a group with the invitation link and display		`Test Replay` `Output` `Screenshots`

Review all test suite changes for PR #93 ↗︎

…statement instead of if-then-else

github-actions · 2023-11-06T11:01:00Z

Report for the pull request #93

🌐 Translation status

UI's texts

Language	Nb. missing translations	Status
en	Missing 18 UI texts ⬇️ 📅 Répondre sur les 3 dernières années,📅 Répondre sur les 3 dernières années Comparez vos résultats <2>avec vos proches</2>,Comparez vos résultats <2>avec vos proches</2> Découvre nos pistes pour agir dès aujourd’hui pour le climat, ou passe le test pour obtenir des recommandations personnalisées.,Découvre nos pistes pour agir dès aujourd’hui pour le climat, ou passe le test pour obtenir des recommandations personnalisées. Découvrez vos résultats, et nos idées d'actions pour vous améliorer.,Découvrez vos résultats, et nos idées d'actions pour vous améliorer. Entrer mes trajets,Entrer mes trajets La catégorie {{title}} représente {{formattedValue}} tonnes de CO2 equivalent.,La catégorie {{title}} représente {{formattedValue}} tonnes de CO2 equivalent. northstar.thankyou,Merci pour votre retour ! Passagers,Passagers Qu'est-ce que ça veut dire ? Cette page s'ouvrira dans un nouvel onglet.,Qu'est-ce que ça veut dire ? Cette page s'ouvrira dans un nouvel onglet. Recevez vos résultats <2>par email</2>,Recevez vos résultats <2>par email</2> Télécharger l'image,Télécharger l'image Total :,Total : Total par an :,Total par an : Une capture du mode Groupe Nos Gestes Climat.,Une capture du mode Groupe Nos Gestes Climat. Votre bilan,Votre bilan Votre simulation est sauvegardée !,Votre simulation est sauvegardée ! Vous allez recevoir un email de notre part sous peu, qui vous permettra de la retrouver <1>à tout moment</1>.,Vous allez recevoir un email de notre part sous peu, qui vous permettra de la retrouver <1>à tout moment</1>. Vous avez terminé le test !,Vous avez terminé le test !	❌

FAQ's questions

Language	Nb. missing translations	Status
en	Ø	✔️

You will find more information about the translation in the dedicated file.

notion-workspace · 2023-11-07T14:55:19Z

Refonte / Code review

bjlaa

Plusieurs questions àgauche et à droite :) Le rebase preprod risque de faire MAL on a changé l'archi de PS !

src/components/form/Question.tsx

bjlaa · 2023-11-09T10:46:03Z

src/publicodes-state/helpers/getType.ts

-  rule: NGCRuleNode | null | any // Model shenanigans: question alimentation . local . consommation is missing "formule"
+  dottedName: RuleName
+  // Model shenanigans: question alimentation . local . consommation is missing "formule"
+  // NOTE(@EmileRolley): I don't get why it's a problem?


Ici est-ce que ça vaudrait pas le coup de mettre un commentaire sur la PR plutôt que dans le code pour en discuter ?

src/publicodes-state/helpers/getType.ts

src/publicodes-state/helpers/safeGetSituation.ts

bjlaa · 2023-11-09T10:48:56Z

src/publicodes-state/hooks/useEngine/index.ts

@@ -22,7 +26,7 @@ export default function useEngine() {
  const getCategory = (dottedName: string): string => dottedName.split(' . ')[0]

  const checkIfValid = (dottedName: string): boolean =>
-    safeGetRule(dottedName) ? true : false


Pour le coup, ici je trouve que le code préexistant incluait plus de cas de figure : false undefined null 🤔

safeGetRule a comme signature :

nosgestesclimat-site-nextjs/src/publicodes-state/providers/simulationProvider/context.ts

Line 15 in cf64546

safeGetRule: (rule: string) => NGCRuleNode | null

Donc on a uniquement besoin de vérifier si la valeur de retour est différente de null.

bjlaa · 2023-11-09T10:54:02Z

src/publicodes-state/providers/simulationProvider/useRules.ts

@@ -1,65 +1,77 @@
+/* eslint @typescript-eslint/no-unused-vars: "warn" */


Pour ne pas avoir de warnings avec l'utilisation de _. Je ne sais pas si c'est recommandé mais j'ai trop l'habitude d'utiliser _ pour ignore des variables.

Tu n'as pas d'erreur si la variable est obligatoire il me semble (exemple : map(_, index)-

bjlaa · 2023-11-09T10:56:06Z

src/publicodes-state/providers/simulationProvider/useRules.ts

    [engine]
  )

-  const everyMosaicChildWhoIsReallyInMosaic = useMemo<string[]>(
+  // FIXME(@EmileRolley): refactoring not tested yet


Il faudrait peut-être tester ça avant de demander une relecture non ?

Hmm il me semble que c'est un oublie 🤔

bjlaa · 2023-11-09T10:58:25Z

src/publicodes-state/types.d.ts

@@ -21,6 +21,8 @@ export type User = {
  hasSavedSimulation?: boolean
 }

+export type RuleName = string


Je vois pas trop l'intérêt d'ajouter un type comme ça, autant utiliser string directement non ?

Cf. #156 (comment)

Ca permet de réduire l'ensemble des valeurs possible. Par exemple, si tu sais que t'as fonction attend un argument de type RuleName tu ne vas pas avoir envie de l'appeler avec n'importe qu'elle string. Dans le contexte de TS ce n'est pas très efficace parce que rien ne t'interdit de passer n'importe quelle string en argument, mais dans les language avec un vrai système de type ça permet de forcer l'utilisation de certaines fonctions.

Je trouve ça assez lourd pour un apport qui me semble minime

bjlaa · 2023-11-09T10:58:35Z

src/publicodes-state/types.d.ts

@@ -21,6 +21,8 @@ export type User = {
  hasSavedSimulation?: boolean
 }

+export type RuleName = string
+
 export type Rules = any


Pareil ici @florianpanchout 🙄

J'espérais faire quelque chose de plus détaillé à terme

bjlaa · 2023-11-09T10:59:20Z

src/publicodes-state/simulationProvider/useCategories.ts

+  order: RuleName[] | null
+}
+
+export default function useCategories({


Pourquoi est-ce qu'il s'agit d'un nouveau fichier ? On n'avait pas déjà ce hook ?

Ca doit être un legacy de rebase

florianpanchout · 2023-11-13T15:01:22Z

Je réponds aux retours qui appellent une réponse (le reste n'est soit plus d'actualité, soit je suis d'accord et n'ai rien à y ajouter)

🔴 La gestion de l’utilisateur est spécifique à NGC et ne devrait pas faire partie de publicodes-state.

🔴 A plusieurs endroits sont utilisés des noms de règle hardcodés du modèle NGC (ex. bilan et services sociétaux).

🟠 Les actions sont pour l’instant assez spécifiques à NGC donc je m’interroge sur la pertinence de garder useActions dans publicodes-state.

Je pense qu’on peut abandonner pour l’instant l’ambition d’avoir une librairie réutilisable telle quelle par d’autres projets. J’avais sous-estimé la spécificité des besoins de NGC ; j’ai préféré réduire les possibilités de réutilisation plutôt que d’ajouter de multiples niveaux d’abstractions.

🟠 ❓ Entre useDisposableEngine et useEngine qui retourne deux moteurs différents, il est un peu compliqué de si retrouver et de savoir lequel utiliser. En particulier, pour le pristineEngine qui est utilisé à un seul endroit. Pourquoi ne pas utiliser useDisposableEngine avec une situation vide ?

pristineEngine est une shallow copie de engine, donc plus efficient pour le besoin.

🟠 ❓ Pourquoi ne pas utiliser la format d’import @/...comme pour le reste de l’app ?

À cause de l’ambition du début d’avoir une librairie réutilisable. Ça n’a plus forcément beaucoup de sens aujourd’hui.

🟠 ❓ Pourquoi utilise-t-on à la fois axios et fetch ?

Axios est utilisé côté client (conjugué à React Query). Fetch côté serveur

🟡 ❓ Il y a-t-il une raison particulière pour forcer l’utilisation de Node 18 ?

Pour avoir la même config que Vercel

EmileRolley · 2023-11-14T11:21:00Z

Je réponds aux retours qui appellent une réponse (le reste n'est soit plus d'actualité, soit je suis d'accord et n'ai rien à y ajouter)

On s'organise comment pour les points sur lesquels ont est d'accord ?

🟠 ❓ Entre useDisposableEngine et useEngine qui retourne deux moteurs différents, il est un peu compliqué de si retrouver et de savoir lequel utiliser. En particulier, pour le pristineEngine qui est utilisé à un seul endroit. Pourquoi ne pas utiliser useDisposableEngine avec une situation vide ?

pristineEngine est une shallow copie de engine, donc plus efficient pour le besoin.

On parle de quoi en terme de gains de perfs ? Afin de comparer avec la complexité que ça ajoute.

🟠 ❓ Pourquoi utilise-t-on à la fois axios et fetch ?

Axios est utilisé côté client (conjugué à React Query). Fetch côté serveur

Ce n'est pas possible d'utiliser l'un ou l'autre à la fois côté serveur et client ?

EmileRolley added the ✨ enhancement New feature or request label Oct 9, 2023

vercel bot deployed to Preview October 9, 2023 13:24 View deployment

EmileRolley force-pushed the code-review branch from 80dd4de to e6d570a Compare October 9, 2023 15:06

vercel bot deployed to Preview October 9, 2023 15:08 View deployment

EmileRolley force-pushed the code-review branch from e6d570a to 3ba9b65 Compare October 9, 2023 15:56

vercel bot had a problem deploying to Preview October 9, 2023 15:56 Failure

EmileRolley force-pushed the code-review branch 2 times, most recently from bc7ce1c to ad09960 Compare October 10, 2023 11:45

vercel bot deployed to Preview October 10, 2023 11:55 View deployment

EmileRolley force-pushed the code-review branch from 18e7675 to 4ad34ed Compare October 10, 2023 12:27

vercel bot deployed to Preview October 10, 2023 12:28 View deployment

Clemog reviewed Oct 12, 2023

View reviewed changes

vercel bot deployed to Preview October 12, 2023 12:37 View deployment

vercel bot deployed to Preview October 12, 2023 13:30 View deployment

EmileRolley mentioned this pull request Oct 13, 2023

Refonte / Code review incubateur-ademe/nosgestesclimat-site#1315

Open

EmileRolley force-pushed the code-review branch from 9e84210 to 3b5b39e Compare October 13, 2023 14:32

vercel bot deployed to Preview October 13, 2023 14:34 View deployment

vercel bot deployed to Preview October 13, 2023 15:15 View deployment

vercel bot deployed to Preview October 13, 2023 15:55 View deployment

vercel bot deployed to Preview October 13, 2023 16:03 View deployment

vercel bot had a problem deploying to Preview October 13, 2023 16:26 Failure

vercel bot had a problem deploying to Preview October 13, 2023 16:29 Failure

EmileRolley force-pushed the code-review branch from 680dfa0 to 872c7f3 Compare October 13, 2023 16:34

vercel bot had a problem deploying to Preview October 13, 2023 16:35 Failure

EmileRolley force-pushed the code-review branch from 872c7f3 to c5a5ad4 Compare October 13, 2023 16:35

vercel bot deployed to Preview October 13, 2023 16:37 View deployment

EmileRolley requested a review from florianpanchout October 13, 2023 16:39

EmileRolley marked this pull request as ready for review October 13, 2023 16:42

EmileRolley changed the title ~~Refacto(pulicodes-state): code review + refactoring~~ Refactor(pulicodes-state): code review + refactoring Oct 13, 2023

EmileRolley mentioned this pull request Nov 4, 2023

Refactor: ues the new data.nosgestesclimat.fr (master branch of the model) - [NGC-184] #172

Merged

EmileRolley added 10 commits November 6, 2023 11:29

refactor(publicodes-state): introduce NGCQuestionType and use switch …

b835d6d

…statement instead of if-then-else

typo: format comment to 80 width

62778e3

refactor(publicodes-states): nitpicks

bb3aa41

refactor(publicodes-state): useEngine/index.ts

9131525

refactor(publicodes-state): reviewed useActions

1d5030b

refactor(publicodes-state): reviewed useDisposableEngine

a77b5b1

refactor(publicodes-state): reviewed simulationProvider/useRules

a1f858d

refactor(publicodes-state): reviewed simulationProvider

4f687f1

refactor(publicodes-state): reviewed useRule

1db207a

refactor(publicodes-state): reviewed helpers

d6b27cb

EmileRolley requested a review from bjlaa November 6, 2023 10:56

EmileRolley force-pushed the code-review branch from c5a5ad4 to d0ed2e1 Compare November 6, 2023 10:57

vercel bot had a problem deploying to Preview November 6, 2023 10:59 Failure

refactor(publicodes-state): sync with preprod

cf64546

EmileRolley force-pushed the code-review branch from d0ed2e1 to cf64546 Compare November 6, 2023 11:08

vercel bot deployed to Preview November 6, 2023 11:11 View deployment

bjlaa reviewed Nov 9, 2023

View reviewed changes

EmileRolley closed this Nov 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor(pulicodes-state): code review + refactoring #93

Refactor(pulicodes-state): code review + refactoring #93

EmileRolley commented Oct 9, 2023 •

edited

Loading

vercel bot commented Oct 9, 2023 •

edited

Loading

Clemog Oct 12, 2023

florianpanchout Nov 13, 2023

cypress bot commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023 •

edited

Loading

notion-workspace bot commented Nov 7, 2023

bjlaa left a comment

bjlaa Nov 9, 2023

bjlaa Nov 9, 2023

EmileRolley Nov 9, 2023

bjlaa Nov 9, 2023

EmileRolley Nov 9, 2023

florianpanchout Nov 13, 2023

bjlaa Nov 9, 2023

EmileRolley Nov 9, 2023

bjlaa Nov 9, 2023

EmileRolley Nov 9, 2023

florianpanchout Nov 13, 2023

bjlaa Nov 9, 2023

florianpanchout Nov 13, 2023

bjlaa Nov 9, 2023

EmileRolley Nov 9, 2023

florianpanchout commented Nov 13, 2023

EmileRolley commented Nov 14, 2023

	{line.entry_nb_visits
	.toString()
	.replace(/\B(?=(\d{3})+(?!\d))/g, '\u00A0')}

		@@ -1,65 +1,77 @@
		/* eslint @typescript-eslint/no-unused-vars: "warn" */

Refactor(pulicodes-state): code review + refactoring #93

Refactor(pulicodes-state): code review + refactoring #93

Conversation

EmileRolley commented Oct 9, 2023 • edited Loading

Code review - publicodes-state

Remarques

Contenu

Tech/TS

Général

Questions

Tech/TS

Changelog

vercel bot commented Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Oct 13, 2023 • edited Loading

2 failed tests on run #518 ↗︎

Review all test suite changes for PR #93 ↗︎

github-actions bot commented Nov 6, 2023 • edited Loading

Report for the pull request #93

🌐 Translation status

UI's texts

FAQ's questions

notion-workspace bot commented Nov 7, 2023

bjlaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florianpanchout commented Nov 13, 2023

EmileRolley commented Nov 14, 2023

EmileRolley commented Oct 9, 2023 •

edited

Loading

vercel bot commented Oct 9, 2023 •

edited

Loading

cypress bot commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023 •

edited

Loading