Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(velo)!: use the @betagouv/aides-velo package #246

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

EmileRolley
Copy link
Collaborator

@EmileRolley EmileRolley commented Nov 1, 2024

Hello @mquandalle, je me permet de faire une PR afin d'utiliser le paquet @betagouv/aides-velo que j'ai créé à partir des modifications qui ont été faites par l'équipe de J'agis sur ton modèle des aides vélo. En effet, nous avons fait récemment une relecture et mise à jour de toutes les aides modélisées ainsi que rajouter certains paramètres et type de vélos (voir le CHANGELOG).

Afin de faciliter la maintenance et la réutilisation du modèle par les autres services j'ai souhaité créer un paquet indépendant. Il me semblerait donc pertinent d'avoir une seule source de vérité pour le modèle et que mesaidesvelo.fr les utilises également.

Je commence dans cette PR le refacto afin que l'on puisse échanger à ce sujet, n'hésite pas à me ping si tu souhaites le faire de vive voix !

A faire (dans cette PR ou une autre)

  • Gestion par aide de l'occasion/neuf et non par type de vélo
  • Gestion intelligente de l'âge

    Un peu comme pour le revenu, il faudrait éviter d'avoir à remplir un champ numérique alors que la plus part du temps la question devrait être Etes-vous majeur:e ?

@mquandalle
Copy link
Owner

Salut Émile, effectivement je n'ai pas mis à jour les aides ces derniers mois, notamment parce que ma mission avec aides-jeune s'est interrompue et que je n'étais donc plus payé pour fournir un paquet à jour avec les aides. Peut être que l'approche la plus simple est que je te donne accès au dépôt ?

@EmileRolley EmileRolley marked this pull request as ready for review November 6, 2024 16:06
@@ -1,18 +1,22 @@
import aidesVelo from '$lib/../aides.yaml';
import Engine, { type Situation } from 'publicodes';
import { rules as aidesVelo } from '@betagouv/aides-velo';
Copy link
Owner

@mquandalle mquandalle Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Le fichier engine.js envoyé au navigateur passe de 31.2kb à 62.3kb (compressés) avec cet PR. Ce n'est pas bien gênant mais je remarque que tout le code du paquet est envoyé au navigateur, y compris le manifeste des images et la class AideVeloEngine qui ne sont pourtant pas utilisés par le client https://mesaidesvelo-eyorlr75e-novaways.vercel.app/_app/immutable/chunks/engine.3UuStsKr.js

Je pense que le code splitting ne fonctionne pas à cause du "barrel file" index.js qui importe toute la librairie. Vu que l'import est fait côté client, il serait mieux de pouvoir importer uniquement les règles sans attraper tout le reste du code du paquet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, je vais regarder comment optimiser tout ça

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

betagouv/publicodes-aides-velo#12 permet d'accéder uniquement aux submodules /data ou /rules si besoin. Cependant, ici vu que AidesVeloEngine est utilisé pour résoudre les options d'une règle de type une possibilité tout est nécessairement importer. Pour avoir uniquement les règles d'importer, il faudrait refaire cette fonction à la main. (A noter qu'il serait tout simplement plus judicieux à terme de l'avoir directement depuis publicodes voir publicodes/publicodes#589).

@@ -1,6 +1,11 @@
{
"extends": "./.svelte-kit/mesaidesvelo.fr/tsconfig.json",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai l'erreur suivante qui n'était pas là avant :

error during build:
TSConfckParseError: failed to resolve "extends":"./.svelte-kit/mesaidesvelo.fr/tsconfig.json" in /vercel/path0/tsconfig.json
    at resolveExtends (file:///vercel/path0/node_modules/vite/dist/node/chunks/dep-R0I0XnyH.js:14139:8)
    at parseExtends (file:///vercel/path0/node_modules/vite/dist/node/chunks/dep-R0I0XnyH.js:14076:24)
    at parse$f (file:///vercel/path0/node_modules/vite/dist/node/chunks/dep-R0I0XnyH.js:13965:23)
    at async loadTsconfigJsonForFile (file:///vercel/path0/node_modules/vite/dist/node/chunks/dep-R0I0XnyH.js:14788:24)
    at async transformWithEsbuild (file:///vercel/path0/node_modules/vite/dist/node/chunks/dep-R0I0XnyH.js:14487:36)
    at async Object.transform (file:///vercel/path0/node_modules/vite/dist/node/chunks/dep-R0I0XnyH.js:14612:32)
    at async transform (file:///vercel/path0/node_modules/rollup/dist/es/shared/node-entry.js:17540:16)
    at async ModuleLoader.addModuleSource (file:///vercel/path0/node_modules/rollup/dist/es/shared/node-entry.js:17757:36)
Error: Command "npm run build:retrofit" exited with 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est étrange, je n'ai pas cette erreur en local.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je sais pas comment debugger, ni comment fix cette erreur malheureusement vu que je n'ai pas accès à Vercel.

@EmileRolley EmileRolley changed the title refactor(velo)!: use the @betagouv/aides-velo package refactor(velo)!: use the @betagouv/aides-velo package Dec 6, 2024
<RevenuSelector {goals} />
{:else}
<Question rule={question} />
{:else if question !== 'revenu fiscal de référence par part'}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai eu pas mal de problème avec l'affichage dynamique des questions. Notamment avec une désynchronisation de question et du composant. Cela semble fix avec l'ajout du {#key question} mais n'étant pas expert de Svelte ce n'est peut-être pas la meilleure façon de faire ou ça entraine peut-être des effets de bords indésirables que je n'ai pas anticipés..

Est-ce que ça te semble correct @mquandalle ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Petit point de vigilance ici aussi où j'ai du faire pas mal de modification sur la "réactivité" du composant mais n'étant pas très familier de Svelte 4, je suis curieux d'avoir ton avis là dessus aussi @mquandalle

@EmileRolley
Copy link
Collaborator Author

Hello @mquandalle, j'ai refait une dernière passe pour régler les différents bugs et prendre en compte les nouvelles questions / règles comme par exemple la prise en compte des personnes en situation de handicap.

Il ne restera plus qu'à mettre à jours la doc/à propos pour rediriger vers le nouveau dépôt des règles, mais je pense que l'on pourra le faire dans une seconde PR 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants