Add TypeScript #218
Add TypeScript #218
Conversation
…d rename files to .ts and .tsx
|
Bueno @FdelMazo , pude añadir bastantes types, hay mil cosas para mejorar con los types, la aplicacion funciona pero:
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:
|
|
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 |
|
buenasm @lopezac, me gustaria sumarme si te sirve una mano. Cualquier cosa avisame! |
|
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 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!! |
|
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. |
|
Lo reviso en unas semanas! Gracias por el laburazo!! |
There was a problem hiding this comment.
Buenas! Ahí estuve viendo el PR. Es tremendo!
Deje bocha de comentarios pero en gran parte son preguntas de cosas que no estoy seguro de porque hacian falta o si son adecuadas o si son errores. Como yo no desarrolle esto se que seguramente hay mil piezas del rompecabezas necesarias para el PR entero, así que confio más en su criterio de porque las cosas estan como estan!
En particular me gustaría asegurarme de dos cosas
- que quede la menor cantidad de cambios de código/lógica posible, y el PR sea en su mayor medida solamente de agregar tipos al proyecto.
- que si se cambia algo en el futuro, y el tipado se rompa, entonces el CI lo ataje y nos evite deployarlo. Se podrá agregar un CI a los PRs para lograr eso (solo tenemos CI sobre master)? El linter actual, ya lo ataja?
Veo que hay otros cambios de código (chiquitos, nada importante) de cosas que no estoy del todo seguro de como impactan en el tipado ni si son necesarios. Se que hay mucho carreo de código legacy acá y hay varias cosas que ya no sirven y se podrían sacar o limpiar o arreglar, pero me gustaría que ese tipo de cosas queden para un nuevo PR, para evitar que en el mergeo de este pueda cambiar algo de la funcionalidad sin darnos cuenta
(no voy a poder re-revisarlo mucho estos dias, perdon!)
| // Esta mal que un useMemo no sea puro... | ||
| // pero cuando se actualiza el grafo, por las dudas, limpiamos el nodo que teniamos elegido | ||
| // test: entrar y clickear un nodo rapido, y esperar a que el usuario se loguee solo | ||
| setDisplayedNode(""); | ||
| const nodes = user.carrera.graph.map((n) => new Node(n)); | ||
| const graphNodes: Node[] = user.carrera.graph.map((n) => new Node(n)); |
There was a problem hiding this comment.
Este typing explicito es necesario? TS no se da cuenta que si clavas new Node(n) entonces el array es de nodes?
| }, [user.carrera.graph, user.carrera.id]); | ||
|
|
||
| // Cuando cambia la carrera, poblamos el nuevo grafo con lo que hay en la DB | ||
| // (o, simplemente aprobamos el CBC si el usuario no tenia nada) | ||
| React.useEffect(() => { | ||
| if (!network || graph.key !== network.key) return; | ||
| if (!network || graph.key !== network.key || !user.maps) return; |
|
|
||
| if (node && (m.nota >= -1 || m.cuatrimestre)) { |
There was a problem hiding this comment.
Mantener el if (!node) return rompia?
| node = node!.cursando(m.cuatrimestre); | ||
| } | ||
| toUpdate.push(node); | ||
| toUpdate.push(node!); |
There was a problem hiding this comment.
Me gustaría evitar lo mayor posible los usos de ! tanto acá como en todo el resto del código, ¿que pensás? Así tenemos un tipado un poco mas correcto, en vez de decirle al compilador que sabemos más que el.
Lo veo como una supresion de errores que estaría bueno que salten si algo funciona mal.
Si se pueden atajar y reducir de alguna otra forma, y mantener solo los absolutamente necesarios, genial!
| const value = !!user.carrera.creditos.checkbox.find((ch) => ch.nombre === c) | ||
| .check; | ||
| const toggleCheckbox = (c: string, forceTrue = false) => { | ||
| if (user.carrera.creditos.checkbox) { |
There was a problem hiding this comment.
Me parece mejor evitar tener toda la funcion en un if, y arrancar con un if (!user.carrera.creditos.checkbox) return, a tener todo el cuerpo condicionado
| const activeVariant = useColorModeValue("solid", "subtle"); | ||
| const commonProps = { | ||
| mx: 1, | ||
| variant: active ? activeVariant : "outline", |
| <MateriaDisplay /> | ||
| </Flex> | ||
| <ScaleFade in={!!displayedNode} transition={{ enter: { delay: 0.0015 } }}> | ||
| { |
| NodesTo(id: string): string[]; | ||
| NeighborNodes(id: string): string[]; | ||
| NeighborEdges(id: string): string[]; | ||
| // TODO: en realidad los getters no retornan un NodeType sino varios especificamente un field |
There was a problem hiding this comment.
Buen TODO! En un futuro PR se puede hacer para que todos tengan la misma interfaz
There was a problem hiding this comment.
Creo que este archivo debería ir en dos partes. Por un lado, nuestros tipado sobre la lib de react-graph-vis (Nodes, Edges, etc), por el otro, nuestros tipos especificos (Getters, GraphInfo, etc). Así podemos diferenciar lo que es del vendor y lo que es propio
| orientacion?: { [key: string]: OrientacionCredito }; | ||
| } | ||
|
|
||
| export interface Orientacion { |
There was a problem hiding this comment.
Creo que algunos de estos tipos estaría bueno no exportarlos, y que vivan solamente dentro de UserInfo (y si es necesario, accederlos con UserInfo[orientacion]. Así mantenemos baja la exposicion de tipos. A los archivos del "back" solo le deberian importar algunas pocas entidades (Nodo, Grafo, Usuario... no se si mucho mas!)
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.