Code Review / Feedback de router

[unjust]

Aqui voy a poner mis comentarios en los lineas de codigo :)

[unjust]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L14

Pienso que no necesitamos hacer la función de setRoutes y podemos manejarlo con un objeto routes que tiene [PATHNAME/ROUTE]: [COMPONENTE] estructura.

Mi argumento por eso es que creo mejor presentar codigo mas sencillo que ellas puede entender y quiza replicar por su propia cuenta, y no solo copiar sin entender que esta pasando.

[unjust]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L33

Me gusta getComponentForRoute con getKeys, no se si agregamos ambos partes de ruta por defecto o el error.

[unjust]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L55

No veo la ventaja a pasar el rootElementId como parametro. Usas esto diferente de root en algun parte de codigo o es mejor testeable? Creo podemos omitir.

[unjust]

Puede ser un diseño mejor porque no tenemos pasar navigate como callback a los componentes 💪

Con los ejemplos de codigo en general, en mi opinion, opto por lo mas sencillo porque a final de dia, si ellas estan copiando eso como ejemplo - mejor que copian cosas que pueden lograr explicar.

Seria bueno si no tenemos eso solo como repo tuyo, pero convertirlo en un post que explica los partes de codigo, incluyendo que hace pushState y popState. Y usarlo en la cuenta Laboratoria o medium Laboratoria 🤓 .

[davidgranados]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L55

No veo la ventaja a pasar el rootElementId como parametro. Usas esto diferente de root en algun parte de codigo o es mejor testeable? Creo podemos omitir.

De acuerdo ese parametro se puede omitir si me haces un PR lo apruebo

[davidgranados]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L14

Pienso que no necesitamos hacer la función de setRoutes y podemos manejarlo con un objeto routes que tiene [PATHNAME/ROUTE]: [COMPONENTE] estructura.

Mi argumento por eso es que creo mejor presentar codigo mas sencillo que ellas puede entender y quiza replicar por su propia cuenta, y no solo copiar sin entender que esta pasando.

Esto se hace para evitar las dependencias circulares

[davidgranados]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L33

Me gusta getComponentForRoute con getKeys, no se si agregamos ambos partes de ruta por defecto o el error.

Esto no lo entendí

[unjust]

Por favor, puede usar ; para terminar los statements / lineas en el codigo? https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L14

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L14 Pienso que no necesitamos hacer la función de setRoutes y podemos manejarlo con un objeto routes que tiene [PATHNAME/ROUTE]: [COMPONENTE] estructura. Mi argumento por eso es que creo mejor presentar codigo mas sencillo que ellas puede entender y quiza replicar por su propia cuenta, y no solo copiar sin entender que esta pasando.

Esto se hace para evitar las dependencias circulares

Ok. Entiendo. No se si la estructura de routes puede ser en este forma mas simple [PATHNAME/ROUTE]: [COMPONENTE] y solo usamos setRoutes para copiar el objeto a otro lado (sin reduce)?

O si queremos quedar con la estructura porque es mas descriptiva y el reduce en setRoutes - diria si empezamos compartiendo eso como ejemplo a ellas que cada parte tiene explanación para ellas entiende que esta pasando en cada paso.

[unjust]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L33 Me gusta getComponentForRoute con getKeys, no se si agregamos ambos partes de ruta por defecto o el error.

Esto no lo entendí

Pensando en omitir la ruta pro defecto https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L40 o el error https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L45

[unjust]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/components/home.js#L19

Porque es async ? No veo que navigate es async.

[unjust]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/components/register.js#L15

quiza history.back() mejor?

[davidgranados]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L14 Pienso que no necesitamos hacer la función de setRoutes y podemos manejarlo con un objeto routes que tiene [PATHNAME/ROUTE]: [COMPONENTE] estructura. Mi argumento por eso es que creo mejor presentar codigo mas sencillo que ellas puede entender y quiza replicar por su propia cuenta, y no solo copiar sin entender que esta pasando.

Esto se hace para evitar las dependencias circulares

Ok. Entiendo. No se si la estructura de routes puede ser en este forma mas simple [PATHNAME/ROUTE]: [COMPONENTE] y solo usamos setRoutes para copiar el objeto a otro lado (sin reduce)?

O si queremos quedar con la estructura porque es mas descriptiva y el reduce en setRoutes - diria si empezamos compartiendo eso como ejemplo a ellas que cada parte tiene explanación para ellas entiende que esta pasando en cada paso.

Si creo que eso también podría funcionar, concuerdo en que el reduce es complejo de entender pero creo que se perdería la capacidad de setear la pagina de 404 junto con las otras rutas

[davidgranados]

Por favor, puede usar ; para terminar los statements / lineas en el codigo? https://github.com/davidgranados/vanilla-spa-router/blob/main/src/router.js#L14

Sorry soy anti ';' hahahah aprendí python antes de javascript y realmente no los veo necesarios

[davidgranados]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/components/home.js#L19

Porque es async ? No veo que navigate es async.

tienes razón ahí no hace falta, probablemente se me olvidó borrar el 'async' ya que este código lo hice en base al repo de una estudiante

[davidgranados]

https://github.com/davidgranados/vanilla-spa-router/blob/main/src/components/register.js#L15

quiza history.back() mejor?

Si sería mejor el history back en este contexto, pero igual realmente esto ya es la lógica de aplicación y es algo que las estudiantes deben hacer de manera personalizada. El código de este archivo sería más para un demo que para que lo usen como referencia