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

[Tech] Migre le Frontend de CRA vers Vite #1150

Merged
merged 18 commits into from
Feb 9, 2024

Conversation

ivangabriele
Copy link
Member

@ivangabriele ivangabriele commented Jan 25, 2024

Taches effectuées

  • Migration des imports SVG en tant que composants pour les adapter à Vite.
  • Séparation des envs Frontend des autres via un fichier /frontend/.env.example
    (⚠️ qu'il faut maintenant copier localement dans un /frontend/.env).
  • Injection des variables d'environnement Frontend au runtime (et non au buildtime) via import-meta-env, comme sur MonitorFish. Voir l'ADR correspondant.
  • Création d'un utilitaire isPuppeteer() pour différencier les conditions que l'on applique pour les tests Puppeteer de celles qu'on applique aux tests Cypress. Même principe qu'avec isCypress().
  • Traduction à la volée des instructions de contribution legacy et déplacement vers le fichier CONTRIBUTING.md.
  • Début d'écriture des instructions de contribution mises à jour dans ce même fichier, en anglais, pour faciliter le partage éventuel avec d'autres équipes de l'UE.
  • Remplacement de la commande npm run cypress:open par npm run test:e2e:open.
  • Petite refacto qui déplace les constantes de frontend/src/features/missions/MissionForm/sse.ts dans un fichier séparé pour éviter un import cycle.

Je me suis aussi permis de désactiver un des tests des points d'întérêt parce qu'on est à près de 10% de failure, il y a au moins 4 bugs notables connus sur cette feature et Adeline a prévu de la revoir entièrement.
image

Notes

  • GitGuardian râle pour une clé de dév qui était déjà présente mais qui a été déplacée dans le nouveau ficher fontend/.env.example.
  • Il y aura un soucis avec monitor-ui concernant cy.fill() lorsqu'on passera à une der versions Rsuite >5.45.0 qui casse la structure DOM des fields en assignant l'attribut id au wrapper (div) au lieu de le passer à l'input. Cet attribut est ciblé par attributs for des labels. Je ferai le fix.
  • Les tests e2e des missions sont localement instables à cause de clickOutside() manquants ou qui ne ferment pas correctement certains dropdowns. À investiguer ?

Review

  • Ca serait idéal si vous pouviez tester que tout fonctionne bien localement chez vous. Env est assez léger pour tourner sur Chrome en mode dev chez moi contrairement à Fish.
  • Je vous conseille de cloner cette branche sous un autre dossier car le switch entre CRA et Vite n'est pas fun d'une branche à l'autre.

⚠️ Derniers points à voir ensemble

  • Faire le tour des impications staging / prod concernant les variables d'environnement @louptheron @thoomasbro.
  • Il n'y a pas de FRONTEND_MISSION_FORM_AUTO_SAVE_ENABLED dans le docker compose de prod, c'est normal @louptheron ? Rien de problématique mais juste pour être sûr.

Related Pull Requests & Issues


  • Tests E2E (Cypress)

@ivangabriele ivangabriele force-pushed the ivan/migrate-from-CRA-to-Vite branch 7 times, most recently from ce994a0 to 94da0d9 Compare January 26, 2024 10:36
@ivangabriele ivangabriele force-pushed the ivan/migrate-from-CRA-to-Vite branch 16 times, most recently from 1416295 to 8fd8a02 Compare February 3, 2024 11:36
Copy link

gitguardian bot commented Feb 3, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
9429425 Triggered Generic Password 07c4d6b infra/.env.example View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@ivangabriele ivangabriele force-pushed the ivan/migrate-from-CRA-to-Vite branch 6 times, most recently from 62c611c to 518d26d Compare February 4, 2024 08:28
ivangabriele added a commit to MTES-MCT/monitorfish that referenced this pull request Feb 5, 2024
Copy link
Collaborator

@thoomasbro thoomasbro left a comment

Choose a reason for hiding this comment

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

Well done !!! 👏

frontend/config/jest.config.js Outdated Show resolved Hide resolved
infra/docker/app/env.sh Show resolved Hide resolved
Comment on lines +34 to +37
- FRONTEND_GEOSERVER_NAMESPACE=monitorenv
- FRONTEND_GEOSERVER_REMOTE_URL=http://0.0.0.0:8081
- FRONTEND_MISSION_FORM_AUTO_SAVE_ENABLED=true
- FRONTEND_MISSION_FORM_AUTO_UPDATE=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Est ce qu'on a encore besoin de ça, vu que c'est dans le .env.example? plutot d'avis de retirer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Bien vu ! Pour le Docker de test je suis d'accord que ça ne sert pas à grand chose lorsque ce sont les mêmes valeurs.

COPY --from=buildFront /tmp/infra/docker/app/env.sh /home/monitorenv/
COPY --from=buildFront /tmp/frontend/.env.example /home/monitorenv/
RUN cp /home/monitorenv/.env.example /home/monitorenv/.env
Copy link
Collaborator

Choose a reason for hiding this comment

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

à quoi sert ce .env dans l'image du coup ? à passer des valeurs par défault ? ou est ce qu'il n'est jamais appelé?

Copy link
Member Author

@ivangabriele ivangabriele Feb 8, 2024

Choose a reason for hiding this comment

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

Bien vu aussi ! Mais alors il faut q'uon fasse un choix : soit on passe toutes les envs par le docker-compose, soit on les passe en copiant le .env. Le plus propre et conventionnel étant le docker-compose à priori.

Copy link
Collaborator

Choose a reason for hiding this comment

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

plutot partant pour docker compose aussi. Comme ça c'est très clair que le .envdu front sert uniquement en dev

@ivangabriele ivangabriele force-pushed the ivan/migrate-from-CRA-to-Vite branch 4 times, most recently from 3aae79a to 0ef64ad Compare February 9, 2024 09:08
@ivangabriele ivangabriele merged commit 48a63e5 into main Feb 9, 2024
18 of 19 checks passed
@ivangabriele ivangabriele deleted the ivan/migrate-from-CRA-to-Vite branch February 9, 2024 14:00
@ivangabriele ivangabriele added the tech. enhancement technical enhancement label Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech. enhancement technical enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tech - Migration vers Vite
4 participants