Fix/use strict

[pau-sala-at-wiris]

Description

• Add strict: true to the tsconfig.json file in the viewer package to enforce stricter typing. This will lead to more robust code and help catch potential bugs at compile time.

Steps to reproduce

Dev deployment

• Check the dev deploy here, it should show the demo viewer and work as expected.

Locally

• run: nx build viewer && nx start html-viewer, the code should compile and work as expected

Warning

• Setting strict to true has revealed a number of errors. Resolving some of these is innocuous, but addressing others involves assumptions that are not clear about what the code should do in certain cases.

---

#taskid X (Waiting for the card to be created)

[jgonzalez-at-wiris]

Esto no me atrevería a hacerlo porque no sabemos qué consecuencias podría tener. En todo caso, sugiero que la linea siguiente tuviera algo como:

if(node.nodeValue !== null) {
   node.nodeValue = node.nodeValue.substring(pos, nextLatexPosition.start);
} 

de esta forma el linter se queda satisfecho y no "rompemos" nada porque el bucle seguiería haciendo sus cosas (de lo contrario no me terminaría de quedar tranquilo).

[jgonzalez-at-wiris]

Se recomienda en estos casos usar este otro operador https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing

Suggested change

	img.alt = data.alt || '';
	img.alt = data.alt ?? '';

[jgonzalez-at-wiris]

Dado que aparentemente Config solo se usa dentro de este fichero de propiedades, creo que podríamos evitarnos esto y simplemente hacer que la interfaz `Config contenga solo campos obligatorios!

[pau-sala-at-wiris]

Si esa interfaz no se exporta y no queda expuesta al usuario no le veo problema.
Además hay algo raro que no había visto, en la declaración de la clase Properties:

config: Config = defaultValues; 

Aquí ya se asignan los default values, no entiendo porque en los getters no se devuelve eso directamente.
Lo cambio, si más adelante vemos que hay que exponer eso ya lo cambiaremos.

[jgonzalez-at-wiris]

Esta comprobación no es necesaria gracias a la que hay al principio de la función.

[jgonzalez-at-wiris]

Este as string creo que está camuflando un error diferente al esperado, ya que si lo elimino me dice el linter que Object is possibly 'undefined'. Quizás aquí deberíamos ver qué pasa en esos casos donde los textContent son undefined?

[pau-sala-at-wiris]

Si, ese y algún otro de los puntos que has comentado son los que me daban miedo. Yo sólo haría un casting si es evidente que es correcto, en ese caso no es evidente que lo sea.
Por otro lado, sin el strict aquí en tiempo de ejecución habría petado con un cannot read properties of undefined al hacer el substring, con lo que para respetar lo que había antes habría que lanzar un error si el textContent no existe.
Imagino que la solución pasa por evaluar bien el código y definir si esa ausencia de textContent es un error (y lanzamos un error), o no puede darse nunca (y hacemos el casting) o es algo controlado y se define un default que tenga sentido en ese contexto.

[pau-sala-at-wiris]

De todos modos, desde mi punto de vista, ese tipo de funciones son super candidatas a tener unos test unitarios que contemplen esos casos para ahorrarnos ese tipo de cuestiones en un futuro.

[jgonzalez-at-wiris]

De acuerdo con lo de las pruebas unitarias, pero no se hasta qué punto son viables debido a que dependenden de nodos que no se hasta qué punto es fácil replicar en un entorno de pruebas o no 🤔

[jgonzalez-at-wiris]

Genial trabajo, gracias por tomarte el tiempo de mejorar estos detalles!

Doy el aprobado a nivel de código pero me gustaría que esto lo probáramos a nivel de comportamiento, es decir, ejecutar el código y verificar que todo sigue funcionando igual. Por favor @carla-at-wiris @usantos-at-wiris ¿podriais revisar y ayudar a verificar esto que comento? Gracias!

[usantos-at-wiris]

The creation of this branch was from main and not from master. Please change.

[carla-at-wiris]

Given the fact that the branch was originally created from main, there are many conflicts with the modified files, and the staging demo breaks when trying to render new formulas. This last point may be solved if the code aligns with the current master.

[pau-sala-at-wiris]

Given the fact that the branch was originally created from main, there are many conflicts with the modified files, and the staging demo breaks when trying to render new formulas. This last point may be solved if the code aligns with the current master.

Given the scope and size of the PR I've created a new one from master branch to avoid conflict related issues . We should close this and review the other one.