FdelMazo / FIUBA-Plan

Organizador de horarios de la Facultad de Ingenieria
https://fede.dm/FIUBA-Plan/
MIT License
46 stars 6 forks source link

Fix react state #26

Closed FdelMazo closed 3 years ago

FdelMazo commented 3 years ago

Como la app está estable, esto hay que hacerlo recién despues de que termine la inscripción, mid-cuatri

Hay que admitirlo... el estado esta muy manejado.

Hay un context global (code smell... variable super global) que es un objeto entero, tocando la data de verdad (esta mal que entrelacemos la data con el estado, deberian ser dos entidades distintas), y conteniendo como 3 arrays a la vez, y se pasa por todo el repo con un llamado a useData horrendo y super fragil. Introdujo bocha de código repetido, y bocha de deep copys de objetos que solo terminaron funcionando después de mucha prueba y error. Casi todos los bugs que nos surgieron, salen de como manejamos el estado de la app.

Y a todo esto, no es código reactoso.

La manera correcta de hacerlo sería:

Esto impacta en... todo. Haría mucho más fácil de arreglar #18, mucho más facil de implementar una lista de eventos modificables (para remover en #24 y para agregar en un futuro), y hace que saquemos mucho código repetido y todos los deep copies. Si hace falta, es conveniente poner en pausa el resto de los issues hasta hacer esto, para evitar trabajar de más y reescribir de nuevo cosas.

Busco opiniones de @komod0 @nicomatex @JDSanto

(Muchas gracias @colltoaction por todas estas ideas)

FdelMazo commented 3 years ago

Un gran problema de esto es que, desde que se introdujo el cache, los end-users no van a recibir los cambios del json que hay en master, porque como no hay un buen manejo de estado, se guarda literalmente todo el objeto de horarios en el cache del usuario.

O sea, si mañana vemos que hay un error, y Algo2 no es el codigo 7541, pero 7542, y cambiamos el horario.js para reflejar ese cambio, como el usuario ya tiene en cache los horarios viejos, nunca va a ver reflejado esto.

Cuando se fixee este issue, y se maneje mejor el estado, se va a pasar a solo guardar en cache los cursos que tiene cada usuario, entonces, siempre tendria los horarios al dia.

Si o si hay que primero addressear este issue, antes que seguir con cualquier otra cosa del repo

colltoaction commented 3 years ago

Quedó en algún lado lo que hicimos juntos?

FdelMazo commented 3 years ago

La branch coll-rewrite, pero desde entonces cambio la estructura del json, y en si era un dirty draft de todo lo que peloteamos (tiene mucho codigo comentado, por ejemplo). Habría que tomarla solo como idea, no como codigo

FdelMazo commented 3 years ago

En este commit gigante termine desacoplando la data de los eventos, y granularizando un toque más lo seleccionado por el usuario. No es clavado lo que habiamos hablado en su momento, y sigue siendo hacer abuso de useContext, pero al menos ya no tenemos el behemoth que era un objeto enorme de react states

89f5376d76c6f5b79757b4faaade4d73005d0aad