search

cozy / cozy-emails

Email Client for Cozy
GNU Affero General Public License v3.0
66 stars 41 forks source link

React migration discussion #783

Closed m4dz closed 8 years ago

m4dz commented 8 years ago

This PR adds a document for upcoming React migration / revamptoenable a roadmap and discussion about it.

frankrousseau commented 8 years ago

Est-ce qu'il est possible de remanier le document pour mettre tout ce qui est alternative et future dans une section annexe en bas ?

m4dz commented 8 years ago

Yep, je suis en train de le retravailler pour le rendre plus compréhensible et plus temporel suite à la discussion de début de sprint hier avec @misstick et @poupotte

frankrousseau commented 8 years ago

je suis en train de le retravailler pour le rendre plus compréhensible et plus temporel

Je sais pas si c'est du temporel. La proposition envisagée doit bien ressortir du reste. L'historique de réflexion a plus sa place en annexe également.

Utiliser des Immutable.OrderedMap (id -> object) pour toutes les listes

Pourquoi utiliser un map si on parle de liste ?

frankrousseau commented 8 years ago

Modifier panel_router : Faire app.dispatch "ROUTE_CHANGE", panels plutot que LayoutActionCreator[pattern.fluxAction], l'AppStateStore handle "ROUTE_CHANGE" et change son état comme un store normal

Je maîtrise pas bien cette partie là mais je mets quand même un warning : attention à ne pas mêler deux concepts : la demande d'action et le moyen de répondre à cette action. L'action doit être "je veux voir la composition" et non pas "affiche le panel compose". L'intention doit être découplé du moyen.

frankrousseau commented 8 years ago

Il manque une description de l'architecture cible, on a du mal à se projeter (et donc comprendre) le résultat.

frankrousseau commented 8 years ago

L'ensemble à l'air complexe, aujourd'hui je ne pourrai pas comprendre ce que vous voulez faire en me levant le matin en étant pas en forme. Quelles parties pouvez vous retirez dans les actions citées ?

frankrousseau commented 8 years ago

Préparer l'optimisation : les getters doivent permettre d'utiliser un memoizer genre lib/cached_transform ou https://optimizely.github.io/nuclear-js/docs/07-api.html#-reactor-evaluate-getter-keypath- ou https://github.com/reactjs/reselect.

J'ai un mauvais souvenir des fonctions de memoize dans Contacts. Ca complexifie beaucoup la lecture du code.

frankrousseau commented 8 years ago

Rendre les variables des store publiques, mais s'interdire de les utiliser en dehors des getters.

Contradiction?

frankrousseau commented 8 years ago

Mixin

Je trouve que cette notion embrouille pas mal dans le code. Est-ce possible de s'en passer ?

frankrousseau commented 8 years ago

Les tests unitaires ne sont pas utiles si les stores et getters sont bien testés. Tester les composants lors des tests de states doit être suffisant.

C'est ambigu: sur quoi portent les test unitaires ?

frankrousseau commented 8 years ago

Rétablir le serveur stateless (préparation partenariat hébergeurs premium)

Est-ce que vous pouvez développer cette partie ?

frankrousseau commented 8 years ago

Passage à JSX ?

Si on peut s'en passer c'est mieux. Dans mes souvenirs la version coffee était un peu le jade du JSX. Ce qui est sympa.

frankrousseau commented 8 years ago

Voilà je vous ai mis une série de questions / remarques. Le sujet a été bien traité, mais maintenant itérons rapidement pour passer à l'action ensuite. Nous avons déjà passé trois jours dessus.

m4dz commented 8 years ago

@frankrousseau merci pour le temps de relecture. Je vais tâcher d'y répondre dans la nouvelle version du doc. Concernant le temps passé, il faut qu'on soit sûr de nous pour faire évoluer ensemble les contextes de l'app tout en faisant vivre le code legacy avec le nouveau. Ça demande un poil de réflexion pour ne pas s'engagre trop rapidement dans des voies sans issue, on se laisse donc jusqu'à la fin de la semaine pour finir de bien poser les idées et les choix techniques avant de passer à l'action (mais on a bien conscience de l'urgence).

m4dz commented 8 years ago

Note : étant donné

  1. que le doc soulève encore des questions
  2. que certaines options font débat
  3. qu'il y a des choix techniques à définir
  4. que le doc à pour objectif de devenir notre source de référence pour les implémentations en étant versé au dépôt

je propose de le reprendre, en le passant notamment en anglais (ce qui me semble plus pérenne), sauf avis contraire.

aenario commented 8 years ago

Il pourrait être pas mal de faire un état des lieu du routing, pour avoir une vision : action : url.

A noter que les actions changent le state qui change l'URL. Les actions sont une action de l'utilisateur, ou un evenement reçu du serveur. C'est la responsabilité du store de déterminer le nouvel état en fonction de l'action, l'évènement "change" du store déclenche un changement d'URL en fonction du nouvel état.

Concernant l'exemple des suppressions Même principe, une fois l'application correctement organisé, toutes les manière de supprimer un message ont le même effet : déclencher une action["DELETE_REQUEST", {target: messageID: 3232}], c'est la responsabilité du store de changer l'état de l'application (par exemple si le panel était ouvert sur le message qu'on vient de supprimer, fermer le panel) et l'évènement "change" du store déclenche un changement d'URL. Notre gros problème actuel est que cet état (panel ouvert) n'est pas dans les store, mais dans l'URL et le state du panel.coffee, ce qui conduit à des redirect foireux un peu partout.

:+1: sur mettre les étapes futur (Redux / JSX / ect ... ) dans une annexe / un autre documents

concernant les keys

L'intérêt des key est de créer un nouveau composant plutot que de réutiliser l'ancien quand c'est approprié. Ca permet de ne pas avoir à considérer le cas du componentWillReceiveProps. Toutefois, si le component est pure render / que le shouldComponentUpdate est correct / qu'il n'y a pas de logique spéciale dans le componentWillMount ce n'est pas forcément nécessaire.

OrdredMap vs List

Pour pouvoir facilement ajouter / enlever un élément de la liste. Si on se dit qu'on a besoin quelque part, vu la complexité de Immutable je pense qu'il vaut mieux utiliser la même structure partout et avoir une seule API, quite à faire des exceptions pour les points ou ça impact vraiment la performance.

:+1: sur architecture cible

\ Mixins ** Je suis partagé, d'un coté c'est vrai qu'on se retrouve avec des @fonctions dont on ne sait pas d'ou elles sortent, de l'autre c'est une façon assez élégante de simplifier le code d'un composant. Peut-être se fixer une convention genre toutes les fonctions de mixin doivent commencer par $selection pour le SelectionMixin par exemple.

:+1: Sur la selection "Quand tout est fini, l'architecture sera" , on l'a un peu fait à la fin de chaque étape.

@m4dz Peut-être un peu tard, mais je suis contre le passage en anglais tant qu'on a pas finalisé le document : ca rend la discussion plus difficile AMHA

Concernant

frankrousseau commented 8 years ago

@m4dz Peut-être un peu tard, mais je suis contre le passage en anglais tant qu'on a pas finalisé le document : ca rend la discussion plus difficile AMHA

Idem, on ira plus vite en français quit à formaliser à la fin en anglais. Cette discussion sert surtout à nous pour le moment.

m4dz commented 8 years ago

Ok, je ferai une formalisation documentée EN en fin de réflexion (en fait, j'ai commencé en parallèle pour noter les choses au propre).

Concernant l'évolution de la réflexion, après un point avec @misstick et @aenario ce matin :

m4dz commented 8 years ago

@frankrousseau j'ai poussé le document (specs-v2-frontend) qui est une formalisation de nos discussions et échanges autour de la refonte, j'espère qu'il ets plus clair / compréhensible. Il est accompagné d'une annexe contenant toute la partie liée au routing. J'ai laissé la version FR draft pour l'historique, elle est vouée à disparaitre avant le merge.

Il manque les annexes que je n'ai pas eu le temps de faire pour le moment, je les livre dans la soirée.

@aenario, @misstick je suis preneur de vos retours, évidemment :smile:

frankrousseau commented 8 years ago

Début de relecture : ok pour les routes. J'ai pas vérifié le détail. Je te propose juste de mettre le legacy en annexe.

frankrousseau commented 8 years ago

Pour le doc d'archi. Il est bien : clair et finalement pas trop long. Je pense qu'on peut jeter les appendices :

NB : Je suis moyen convaincu par les getters qui vu de loin semblent ajouter un élément en plus sans servir à grand chose. Vous êtes sûrs qu'ils sont utiles ?

frankrousseau commented 8 years ago

En dehors de ces remarques, de mon côté tout est ok. Je ne serai pas là demain pour en faire de nouvelles. Je vous laisse donc finaliser ensemble. Pour la suppression des appendices, j'y tiens beaucoup (en dehors des routes que vous avez mises à part et qu'il faut garder).

misstick commented 8 years ago

@frankrousseau : les getters c'est bien pour retirer la logique de récupération de données des vues. ça fait partie des logique de framework type redux, ou nuclearJS. je trouve ça pas mal car ça centralise la récupération de donnée au lieu de faire X fois la même chose différement. Il faut juste s'assurer de ne pas faire un getter par vue, mais un getter par besoin (comme pour les stores).

frankrousseau commented 8 years ago

Ok pour les getters.

m4dz commented 8 years ago

@frankrousseau à la réflexion, je suis d'accord aussi sur le fait que les appendices ne sont là que pour mentionner des pistes éventuelles pour le "et ensuite…". Je les retire (je garde juste la mention à JSX dans les components pour qu'on l'ait en tête quand même), et je pose les routes legacy dans un fichier à part.

Content que ça te plaise en tous cas ! :smile:

m4dz commented 8 years ago

J'ai rajouté les parties sur le stateless server et la mention à JSX en progressive enhancement. Normalement tout est ok maintenant.

frankrousseau commented 8 years ago

Est-ce qu'on peut merger ? @misstick @aenario ?