FIUBA-Map icon indicating copy to clipboard operation
FIUBA-Map copied to clipboard

Add TypeScript

Open lopezac opened this issue 1 year ago • 2 comments

Closes #187

La idea es agregar TypeScript a la app, se que esta el otro PR #190, pero esta basado en una branch en el repo de Lazcano Luca, y preferi trabajarlo en una branch directamente en este repo.

Agregue la version de TypeScript 4.9.5 que es la mas moderna compatible (me parece) con la version de react-scripts 5.0.1 de la app. Los types van a estar en src/Types, nose si preferis otra manera de organizarlos.

lopezac avatar Aug 19 '24 20:08 lopezac

Bueno @FdelMazo , pude añadir bastantes types, hay mil cosas para mejorar con los types, la aplicacion funciona pero:

  • Introduje un bug que cuando se pone una materia como cursando se corre muchisimo a la izquierda

Lo principal seria arreglar ese bug.

Aparte de eso lo que le falta a este PR es resolver varios problemas que deje en comentarios TODO y FIXME, por ejemplo:

  • En constants.ts el objeto GRUPOS , falta agregar, o idear una manera de que el editor reconozca cuando una categoria admite la propiedad color, actualmente en 3 partes (estan comentados con linea // FIXME: reparar ts-ignore ) ignoro el error que salta,
  • O el modal de LoadingGraph chakra pide que se le agregue on onClose
  • etc

lopezac avatar Aug 22 '24 15:08 lopezac

Buenísimo @lopezac! Re va tomando forma. Chiflame si necesitas una mano con el bug, o si queres una review en algun momento en particular

FdelMazo avatar Aug 22 '24 16:08 FdelMazo

buenasm @lopezac, me gustaria sumarme si te sirve una mano. Cualquier cosa avisame!

salluzziluca avatar Nov 24 '24 15:11 salluzziluca

Hola @salluzziluca, como andas, si me encantaria ayuda, esto lo deje hace unos meses tirado, y ahora acabo de leer masomenos todo para ponerme al dia de vuelta.

Para tipear FIUBA-Map, lo que hice fue convertir los archivos a TS, y empezar a escribir sus tipos, y aprender como funcionan, que parametros, toman etc, y ver como bajaban la cantidad de errores de 200+, a ahora seran menos de 10 errores maso, los podes encontrar buscando la palabra ts-ignore

Tambien en su momento tuve muchas dudas que deje en TODOs y FIXMEs esparcidos a lo largo de la app

Modele manualmente los tipos por ejemplo de Graph de react-graph-vis, ya que es una libreria vieja, sin soporte de TS.

Tienen un tipo , pero no lo agregaron a su paquete npm, y intente usarlo agregandolo manualmente a la app, pero no es de mucha ayuda creo.

Y acabo de descubrir, mientras releia todo lo que habia hecho hace 3 meses. El poder de Copilot ya que me ayudo a resolver problemas oscuros como unos de ChakraUI, que los tipos de por ejemplo de los props de un Badge, tiene textAlign (que es el text-align de css), pero ellos te pedian que fuera de tipo ResponsiveValue<TextAlign> y lo chistoso es que ellos tenian definian el tipo ResponsiveValue, pero no decian ni pio (en su documentacion) de donde sacaban el TextAlign (o almenos mi yo de hace 3 meses no supo encontrarlo), pero ahora gracias a Copilot en un momento descubri que TextAlign lo definia csstype con Property.

Lo esencial que falta reparar son los @ ts-ignore esparcidos, y incluso ver los TODO y FIXME, e ir descartandolos, aparte de eso hay un bug importante que comente arriba, que cuando uno marca una materia como cursando aparece muy a la izquierda.

Bienvenida sera tu ayuda @salluzziluca!!

lopezac avatar Nov 24 '24 20:11 lopezac

Listo @FdelMazo, chequea como quedo, fijate si anda todo bien, y si falta algo decime

El bug era en la clase Node donde definia originalLevel luego de level, y originalmente habia que definir originalLevel y luego level.

lopezac avatar Dec 21 '24 23:12 lopezac

Lo reviso en unas semanas! Gracias por el laburazo!!

FdelMazo avatar Dec 22 '24 00:12 FdelMazo

Hola Fede

  • Ahi veo todas las sugerencias, pero tenes total razon en lo que decis, hoy lo veo igual que vos, este PR solo tendria que ser tipados, nada de cambio de logica, pero el Axel de agosto pensaba diferente jajaja, asi que intentare deshacer todos los cambios de logica que hice.
  • Ahi me fijo si el CI se ataja las roturas de tipos, y si no, le intento agregar mayor proteccion

lopezac avatar Dec 28 '24 11:12 lopezac

Hola Fede!

  • Elimine la mayor cantidad de cambios de logica
  • Quedaron 3 usos del ! non-null assertion en el proyecto, al eliminar todos los otros, fue una forma de realmente tipear las cosas y no esconderlas bajo la alfombra digamos, en MapContext.tsx:
export const UserContext = React.createContext<UserType.Context>(null!);
export const GraphContext = React.createContext<GraphType.Context>(null!);

Es una practica "conocida", safa para mi

Luego en Graph.ts const [network, setNetwork] = React.useState<ReactGraphVisType.Network>(null!); Si no usasemos un ! aca, tendriamos que en cada funcion que utilice network, nodes, o edges chequear que no sean nulos, habria que agregarlo?, o esta bien asi?, que opinas?, aca capaz seria mas correcto no usar el !, pero seria mas verboso el codigo

lopezac avatar Jan 08 '25 23:01 lopezac

Me parece bien usar los ! en esos casos especificos! @lopezac esta quedando buenísimo. En unos días lo reviso de punta a punta.

FdelMazo avatar Jan 09 '25 12:01 FdelMazo

Buenas

  • Añadirle "noEmitOnError": true, en tsconfig.json creo que ayuda, a tirar error si hay un error buildeando el proyecto en main
  • No estoy seguro si el verify-ts-pr.yml workflow es suficiente para chequear si cada vez que se hace un commit en un pr, tire error

lopezac avatar Jan 09 '25 23:01 lopezac

Hola @FdelMazo , ahi agregue todas las sugerencias, me parecieron buenas, nose si sera mas prolijo, pero estara bien supongo mergearlo asi sin squash, ahi lo mergeo

lopezac avatar Jan 27 '25 23:01 lopezac