feat: Better user information #48
Conversation
Faster checks & prevent too many retry on same center
|
@dunglas Je continue ? La revue va être un poil lourde... M'enfin je progresse, le code back m'a l'air plus lisible :) |
|
Perso je ne suis pas super fan de l'utilisation des classes dans le code front sans TypeScript (je trouve que ça alourdit un peu, et que c'est un peu étrange dans un langage à héritage par prototype), après je n'ai pas d'avis tranché non plus. |
Bah ca sépare bien les responsabilités, l'index est beaucoup plus lisible quand tu débarques je pense. Clairement si on avait pu faire en TS, j'aurais préféré, je pensais pas que c'était possible. Mais avec les classes que j'ai rédigé là, la migration serait très rapide ! |
|
La j'étais volontairement parti sur un style inspiré de la programmation fonctionnelle (avec quelques grosses divergences je l'accorde) et un seul fichier par page pour la simplicité. Je ne pense pas que la programmation fonctionnelle nuise plus à la séparation des responsabilités que la POO (on pourrait aussi utiliser des modules pour séparer les différents jeux de fonctions). Le fait de ne pas séparer c'était plutôt dans l'idée de garder le code le plus simple et concis possible (un peu à la manière d'un test e2e vu le sujet de l'extension). |
|
Ha ben, faut qu'on statue rapidement sur le sujet, clairement je m'y perdais dans le code en un seul fichier, j'ai d'ailleurs corrigé plusieurs choses en passant aux classes. Dans tous les cas, si tu t'y oppose, je préfère le savoir assez tot, il y a déjà pas mal de taff là, que je continue pas pour rien ;) |
|
Comme je te disais ce n'est pas une opposition forte. C'est plus que ce n'est pas le style que j'avais choisi (consciemment), mais après si ça te simplifie la tâche je m'en accommoderai bien ! T'as un avis là-dessus @julienw ?
(Ce ne sont que des background scripts don, ca devrait le faire) |
J'avais pas vu ca comme un risque, je pensais que tout était reload ou rien...
L'objectif final est bien d'utiliser les classes utilitaires des deux cotés. C'est ce que je teste en ce moment. |
|
Pour tester facilement : tu lances la page de la liste des centres, et tu recharges l'extension. Tu verras que le script est exécuté à nouveau alors que la page n'a pas été rechargée (au début ça m'ajoutait deux fois les boutons "ajouter à ma liste", c'est comme ça que j'ai découvert cette particularité, et d'où le test de la propriété sur Aussi pour les content scripts, je pense qu'il faut éviter de polluer le namespace global, ça pourrait entrer en conflit avec les futurs développements faits par Doctolib (on est quand même dans le contexte d'exécution de leurs pages). C'est pour ça que j'ai wrappé tout le code dans une fonction anonyme, comme ça on ne pollue pas le contexte global de la page. |
|
Ca marche, je prend en compte tout ca, thx :) |
|
Ca avance bien :) Et c'est beaucoup plus rapide ! |
…r-user-information
* Clean destroy or resources in popup => LOT OF ERROR are gone form console \o/
browser_action/index.js
Outdated
| window.addEventListener("unload", function (e) { | ||
| vCLStorage.destroy(); | ||
| browser.storage.onChanged.removeListener(onStorageChange); | ||
| }); |
There was a problem hiding this comment.
On détruit proprement les resources (les listeners), ca nous évite des zombies et plein d'erreur dans la console 👍
| if ( | ||
| !confirm( | ||
| "Êtes-vous sur de vouloir supprimer toutes les données de l'extension ?" | ||
| ) | ||
| ) | ||
| return; | ||
|
|
||
| await browser.storage.sync.clear(); | ||
| browser.storage.sync.clear(); | ||
| vCLStorage.clear(); |
There was a problem hiding this comment.
On clear bien les données locales aussi
* Clean handle of reset data
|
Done, fini le refacto, on utilise bien les memes modèles coté back & front :) |
|
Une console, qu'elle est belle :) |
…r-user-information
…r-user-information
|
Quelqu'un aura le temps de faire une passe sur cette PR ? Je vois les autres s'accumuler, tant que je peux merger ça va, quand il va y avoir des conflits ça sera moins rigolo 😭. il y a aussi des fix qui arrivent par d'autres PR, c'est dmg de faire les choses 2 fois 😢 |
There was a problem hiding this comment.
Bon, j'ai fait une passe.
Ça a le mérite de fonctionner, ce qui est déjà pas mal vu le niveau de réécriture ! Alors bravo pour ça !
J'ai fait quelques remarques, certaines optionnelles, d'autres moins (utiliser un paramètre "options" dans un constructeur un peu compliqué, ajout de commentaires). Ça me va aussi très bien si tu préfères traiter ces remarques dans un ou des patchs séparés, pour pouvoir sortir celui-là, vu que tout marche correctement.
Bon boulot !
| */ | ||
| constructor(delayBetweenJobs, minDelayRetryJob, onJobStart) { | ||
| /** @type {number} en millisecondes */ | ||
| this.delayBetweenJobs = delayBetweenJobs * 1000; |
There was a problem hiding this comment.
Avis perso: je préfèrerais aussi qu'on garde tout en ms (je trouve ça plus parlant notamment si on veut des demi-secondes, et puis en JS on a l'habitude des ms)
mais ne change pas maintenant alors que ça marche, il y a un risque de péter des trucs.
| * listenChanges? = false; | ||
| * onLocationsChanged?: (locations: LocationStatus[]) => void; | ||
| * onLogsChanged?: (logs: string[]) => void; |
There was a problem hiding this comment.
En typescript, tu peux utiliser Partial<> pour faire ce genre d'objets dont toutes les propriétés sont optionnelles.
(remarque optionnelle pour cette PR)
There was a problem hiding this comment.
Ca marche en JS doc ca ?
There was a problem hiding this comment.
Pas en JSdoc pur bien sûr, mais en typescript-en-JSDoc oui pas de souci
There was a problem hiding this comment.
typescript-en-JSDoc je vois pas bien ce que tu veux dire
Je voudrais améliorer le retour utilisateur une nouvelle fois...
Fait:
A faire:
Donner un historique pour chaque lieu(je ne sais pas trop comment le présenter finalement)