-
Notifications
You must be signed in to change notification settings - Fork 17
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
Feat/ Beta support for graphql #330
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No cacho mucho de graphql como para comentarte más en detalle pero se ve bien. Lo que si pensé es que quizás esta configuración es candidata a convertirse en algo como power api. Al final, power api nació porque había mucha configuración dentro de potassium. El sacar esto a una gema, te da más libertad para trabajar y además dejar limpio potassium. Por ejemplo, estaría bueno que GraphqlController o LoginMutation tuvieran tests. Seguramente en la gema, estarían testeados pero acá pasan como templates que terminan en código no testeado en la app. Otra ventaja de tenerlo como gema, es que es fácilmente actualizable en los proyectos cambiando la versión. En fin, lo veo bien, pero pensaría en sacarlo como gema en una segunda etapa.
end | ||
|
||
def create | ||
add_api if get(:api_support) | ||
if get(:api) == :graphql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pregunta inocente: tendrá sentido tenerlo como recetas aparte? Digo, existe la posibilidad de tener un proyecto usando rest para algunas cosas y graphql para otras?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
si es cierto no lo había pensado.
Es posible que pase.
igual me tinca que eligiendo uno y depués haciendo potassium install <el_otro>
debería andar bien por mientras
me hace sentido lo de hacer otra gema. |
Claro, en definitiva eso es lo que hago en power_api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Se ve bien! No me manejo mucho en como se agregan cosas a Potassium pero los componentes importantes están considerados.
Tal como mencionas, en el mediano plazo (y dependiendo de si nos interesa) es importante resolver la autorización con alguna integración custom de Pundit o alguna otra alternativa.
Lo otro importante agregar en el futuro es el manejo del problema N + 1. GraphQL tiene este problema de manera intrínseca pero está bien resuelto. Una gema que lo resuelve bien es batch-loader pero también existen otras.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bueena see ve bien! Igual tiene su resto de boilerplating pero pensé que sería más, me gusta lo que dice Leandro pero sí o sí pa una segunda etapa!
También buena idea tener en mente lo de Gabriel sobre las N+1, pero claro, no necesariamente para este PR.
Me gusta la estructura que propusiste y mucho mejor lo de sin input:
.
Lo de versión pagada quedé 😱 , pensée que era todo full opensource jaja, misma decepción que con sidekiq pro jaja. No sé bien si es tan simple hacerlo nosotros, hablo un poco de la ignorancia pero un amigo mío quedó odiando GraphQL porque era muy difícil hacer permisos simples que con api eran botados, eso sí, no era en rails, así que puede haber sido eso. No ahondé mucho en su momento jaja.
Un detalle, creo que hay que ver si se explicita la "necesidad" de la variable de entorno pa lo de JWT en algún lado.
Te dejé una duda y un comentario en el código mismo!
rescue => e | ||
raise e unless Rails.env.development? | ||
handle_error_in_development e | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
creo que esto se parece a una de las cosas que teníamos en potassium antes y que terminamos sacando porque generaba problemas de replicabilidad development/prod y hacía el debugging más oscuro, no sé si es exacto el caso, pero en general tiendo a preferir que estas condiciones sean solo pa casos muy particulares. Porque si los errores pasan de forma distinta en prod que en dev, se vuelve cacho. Aquí está lo que decía.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eso es parte del boiler plate de la gema.
Entiendo que es su forma de solucionar que solo cuando se caiga en development el endpoint de graphql te responda con el backtrace y el error.
Me hace sentido lo que dices de todas maneras, voy a revisar como se comporta sin eso
class Mutations::LoginMutation < Mutations::BaseMutation | ||
null true | ||
|
||
# argument :user_id, ID, required: true, loads: Types::UserType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no caché esto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
se me pasó jeje
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo vengo de 🐸 y 👶 en Rails jaja pero se ve bacán! Me hace sentido la estructura de archivos por lo que he visto en proyectos de GraphQL.
Ojalá se pueda hacer algo no tan complejo con Pundit. Comparto la frustración del amigo de Rai, autorizar simple en GraphQL es un cacho, generalmente lo que existe se queda corto.
Se agrega la opción de tener una API GraphQL a la receta de api. Esto implica principalmente el uso de la gema graphql y la generación de la estructura del proyecto gql en la carpeta
graphql
. Además se usa la gema graphql-playground para generar la documentación automática con el ambiente de prueba de las consultas. También se incluye la instalación y configuración de vue-apollo para el frontend. Gran parte esta basado en lo que hizo @GabrielLyonB en su repo de prueba para la presentación de hace un par de meses.En teoría este PR es lo mínimo suficiente para generar una App Vue+backend-GraphQL-rails en la que se puede empezar a desarrollar inmediatamente.
Decisiones de diseño que revisar.
Estructura de la carpeta GraphQL
La instalación por defecto de la gema
graphql
tiene hartas cosas que no me gustaron mucho y que no calzan mucho con la forma platanus de hacer las cosas lo más modularizado / desacoplado posible, por lo que la receta de potassium hace hartos cambios posteriores a correr la instalación. Suponiendo un modeloBooks
, un proyecto sería algo así:Los cambios que hice son mover los
base_types
a una carpetabase
dentro detypes
y hacer una carpetaqueries
que permite hacer un archivo por cada query mediante
resolvers
(la única otra opción es achoclonar todo el código dentro dequery_type.rb
) , me basé en este tutorial de acá .Autenticación
Se implementó la autenticación mediante un JWT en un header. Si el usuario pidió devise en la receta entonces se autogenera todo el código para hacer login mediante una mutation y para detectar el current user mediante el token.
Usé JWT porque así lo hizo @GabrielLyonB en el repo de graphql anterior que tenía y no quise complicarme la vida haciendo otra cosa. Funciona revisando el jwt en el
graphql_controller.rb
y pasando elcurrent_user
dentro delcontext
a query/mutation. Creo que sería mejor que quedara algo similar pero con una herramienta más típica de nosotros comosimple_token_authentication
o la cookie de device y sin que quede todo el código acoplado en el controller.Uso de argumentos planos
Por default la gema genera que al hacer una mutation los argumentos se tengan que pasar como un
inputObject
en vez de como argumentos planos. Es decir una mutation se haría asíEn vez de:
Sentí que solo agregaba ruido por lo que metí mano para que quedara como la segunda opción, siguiendo lo que dicen acá
Ejemplo de uso.
Dado un modelo
book.rb
con camposauthor
ytitle
, definimos un type BookY una mutation para crear un libro
Junto con la declaración en mutation_type.rb (similar a definir una ruta en routes.rb)
También definimos una query para acceder a un libro y la agregamos a
query_type.rb
Gracias a apollo podemos definir en vue un componente que se conecte directamente con el backend mediante una query que es a la vez cacheada por apollo (El estado de la app se maneja mediante apollo, sin Vuex).
Cosas pendientes
Nota: no me funcionó el linter en local y no me esforcé en arreglarlo jaja así que el mono se enojó 🙄