Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernize code #40

Merged
merged 21 commits into from
Sep 30, 2019
Merged

Modernize code #40

merged 21 commits into from
Sep 30, 2019

Conversation

fdemian
Copy link
Contributor

@fdemian fdemian commented Jul 3, 2019

Cambio es largo pero necesario.
Entre otras cosas el cambio:

  • Resuelve el issue .js o .jsx ? #34 .
  • Migra componentes a hooks.
  • Upgradea un monton de paquetes.
  • Sube la cobertura de código del 60% a 80%.

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #40 into master will increase coverage by 20.02%.
The diff coverage is 78.87%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #40       +/-   ##
===========================================
+ Coverage   60.88%   80.91%   +20.02%     
===========================================
  Files          46       38        -8     
  Lines         317      241       -76     
  Branches       31       29        -2     
===========================================
+ Hits          193      195        +2     
+ Misses        114       40       -74     
+ Partials       10        6        -4
Impacted Files Coverage Δ
src/Seguidor/TreeView/SubjectYears.js 100% <ø> (ø) ⬆️
src/Seguidor/CarouselView/SubjectYears.js 100% <ø> (ø) ⬆️
src/Errors/NotFound.js 100% <ø> (ø)
src/App/Home.js 100% <ø> (ø)
src/store/configureStore.js 0% <0%> (ø)
src/Routes/AppRoute.js 0% <0%> (ø)
src/App/App.js 100% <100%> (ø)
src/Seguidor/YearOfStudy/YearOfStudy.js 100% <100%> (ø)
src/Fetching/FetchingIndicator.js 100% <100%> (ø)
src/Seguidor/Pendientes/Pendientes.js 100% <100%> (+75%) ⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6efd4e1...d5919b5. Read the comment docs.

@gndelia
Copy link
Contributor

gndelia commented Jul 3, 2019

porque migramos a hooks? Justamente lo que recomienda el team de react es no ir como loco a migrar todo a hooks.

@fdemian
Copy link
Contributor Author

fdemian commented Jul 3, 2019

¿Eh, donde dicen eso, Gonza? Hasta donde entiendo dicen que no es necesario migrar inmediatamente, pero que conviene.
No me parece que cambiar el par (casi literalmente, son 2 o 3) componente cuente como "apurarnos a migrar". Nosotros, justamente tenemos pocos componentes y nos podemos permitir hacer el cambio.
El futuro llego hace rato y son hooks (todo un palo, ya lo se).

El cambio a hooks es, justamente porque son funciones en lugar de clases (las clases en JS son un quilombo internamente). Además:

  • Las funciones son más fáciles de sestear.
  • Podés reusar logica entre componentes.
  • Son componentes más simples que usar clases.
  • Tienen ventajas para el code splitting.
  • Hacen que sea más fácil el trabajo para los minificadores de código.

¿Cual era tu preocupación mas allá de eso respecto a hooks?

@gndelia
Copy link
Contributor

gndelia commented Jul 3, 2019

Crucially, Hooks work side-by-side with existing code so you can adopt them gradually. There is no rush to migrate to Hooks. We recommend avoiding any “big rewrites”, especially for existing, complex class components. It takes a bit of a mindshift to start “thinking in Hooks”. In our experience, it’s best to practice using Hooks in new and non-critical components first, and ensure that everybody on your team feels comfortable with them. After you give Hooks a try, please feel free to send us feedback, positive or negative.

emphasis mine

no te lo voy a borrar si ya lo hiciste jajaj pero creo que capaz se podian hacer otras cosa (?)

@fdemian
Copy link
Contributor Author

fdemian commented Jul 3, 2019

"Big rewrites" no creo que aplique 😄 .

Por eso dije que en una codebase más grande sería un issue. Acá no me parece realmente que sea el caso. Si tenés alguna incomodidad con usar hooks lo vamos viendo, pero lo otro, con solo 3 componentes "serios" que realmente usan clases (y que fueron los únicos refactoreados para hooks), no me parece muy aplicable.

PD: por las dudas, hice testing y la aplicación no tiene nuevos errores con esto.

@fdemian
Copy link
Contributor Author

fdemian commented Jul 8, 2019

Hice un par de commits mas donde le meti un poco de amor a la UI de mobile en el visor de materias (no necesitamos una "vista compacta" al estar en mobile, ya que es la única que hay ahi). Hacen falta más retoques igual.

@gndelia , después abrimos thread para esto si queréis, pero...tenías objeciones al hecho de usar hooks? O simplemente te parece que el cambio fue muy rápido?

@Javier-Rotelli
Copy link
Member

yo si tengo objeciones a usar hooks. en lineas generales se puede lograr lo mismo con recompose. y no te quedan los componentes super acoplados e intesteables.
en fin. move on. hay mil cosas que objetaria aca. pero me parece mejor si le damos para adelante

@Javier-Rotelli Javier-Rotelli merged commit 5dc94f6 into master Sep 30, 2019
@gndelia
Copy link
Contributor

gndelia commented Sep 30, 2019

ojo que el de recompose ya empezó a avisar que quiere discontinuarlo y recomendar usar hooks

parece que nadie se quiere perder ese tren 🤷‍♂

@Javier-Rotelli
Copy link
Member

No es tan asi:
acdlite/recompose#756 (comment)
en fin. en mi experiencia hooks te acopla todo de manera grosera. pero 🤷‍♂

@Javier-Rotelli Javier-Rotelli deleted the modernize-code branch October 1, 2019 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants