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

Improve Rails adoption process #511

Closed
rmosolgo opened this issue Jan 26, 2017 · 34 comments
Closed

Improve Rails adoption process #511

rmosolgo opened this issue Jan 26, 2017 · 34 comments

Comments

@rmosolgo
Copy link
Owner

Currently, the first several steps of adding GraphQL to your Rails app involve copy-pasting lots of boilerplate code:

  • controller
  • graphiql-rails gem
  • schema file
  • query type

Then, expanding the schema also requires repetitive work: opening files and typing boilerplate code.

What could be done to improve this process?

If we were to provide an opinionated setup & organization strategy, what would it be?

Could we also improve the client-side experience? (Eg, configuring apollo-client to send Rails CSRF tokens?)

Rails generators could solve a lot of problems here. It's important to keep Rails as a totally optional dependency for this library.

@junosuarez
Copy link

One of the things we do in our controller is create a standard context object, which includes some things like an error and performance profiling accumulator, which gets flushed to the respective logging services (new relic, statsd, etc) at in an after filter.

@rmosolgo
Copy link
Owner Author

Very cool, seems like at a minimum, a generator could guide the new user in the right direction with something like:

query_context  = {
  # You can access these values anywhere 
  # in GraphQL execution, so use them to 
  # scope the query or accumulate information. 
  # eg: 
  # current_user: current_user, 
}

{Generated}Schema.execute(query_string, {
  context: query_context, 
  variables: query_variables, 
})

@nettofarah
Copy link
Contributor

Here are a few ideas:

Controller/Route setup

  • Create a /graphql route in routes.rb
post '/graphql' => 'graphql#query'
  • Create a app/controllers/graphql_controller.rb controller

GraphQL Specific folder

Something like this:

app/graph
 types/
    query_type.rb
 mutations/
 fields/ # not sure about this one
 schema.rb
  • Add app/graph to config.autoload_paths

@junosuarez
Copy link

ah, yep, folder structure! ours looks something like

./app/
  graph/
    loaders/
    mutations/
    resolvers/
    types/
    schema.rb

where /loaders is objects from GraphQL::Batch::Loader and /resolvers has POROs mapping to the fields in the /types dsl

@rmosolgo
Copy link
Owner Author

rmosolgo commented Jan 27, 2017

Are mutations "just" resolvers with side effects? oh, like, GraphQL::Relay::Mutations, right

@rmosolgo
Copy link
Owner Author

Another option would be adding Sprockets-ready versions of apollo and Relay. I might be the only Sprockets user left on earth, and Relay might not even work with Sprockets, but I thought I'd mention it 😆

@dphaener
Copy link

One thing I have done is make a base resolver class that handles typical use cases. I.E. when you know a collection is going to respond in the standard Rails way: user.todos. Using the same inflector magic that AR uses. So, directory structure is similar to what everyone else has listed:

app/graphql
  types/
  mutations/
  resolvers/

Gist of the base resolver here: https://gist.github.com/dphaener/94c9ca91b1bef6e8af37441cffd2f5f8

For me, this has removed a lot of boilerplate code. For instance:

connection :todos do
  type -> { TodoType.connection_type }
  resolve TodoResolver.new.collection
end

I can also use a different caller if I have a custom method:

connection :completed_todos do
  type -> { TodoType.connection_type }
  resolve TodoResolver.new(:completed_todos).collection
end

This will call user.completed_todos to retrieve the collection.

@dphaener
Copy link

I also have a whole bunch of generators that I use: https://github.com/DNACodeStudios/react-rails-bootstrap/tree/master/lib/generators

@nettofarah
Copy link
Contributor

@rmosolgo I think supporting relay right away could be overkill.
Do we know how many people use this gem for the relay bindings vs just using it for GraphQL alone?

@rmosolgo
Copy link
Owner Author

rmosolgo commented Jan 27, 2017

how many people

🙅‍♂️ I don't know!

@junosuarez
Copy link

relay is a very common case, but supporting relay-specific things ought to be an option when scaffolding a new project.

@Willianvdv
Copy link
Contributor

Looks like Rails' webpacker could make integrating Rails + Relay / Apollo a lot easier.

@Willianvdv
Copy link
Contributor

I'm trying to setup Relay for the graphql-ruby-demo app. I'm using the webpacker gem to get all the js webpack stuff running. My idea is to write a installer that installs Relay in a Rails project, similar to the React installer in webpacker.

Here's the (huge) PR rmosolgo/graphql-ruby-demo#24 for the demo app on Relay, currently it has only React support since I've not yet started with the Relay installer.

@rmosolgo rmosolgo mentioned this issue Feb 5, 2017
3 tasks
@rmosolgo
Copy link
Owner Author

rmosolgo commented Feb 5, 2017

Sketched out a few specific generators at #521, I'd love to hear your opinions on them if you have any !

@vergenzt
Copy link

One random thing I thought I'd comment on: I would strongly prefer that the conventional/generated GraphQL folder name in Rails be app/graphql rather than app/graph.

A Google search for "rails graphql" yields much clearer results than a search for "rails graph". (E.g. by a newbie to a codebase that uses GraphQL wondering "what does this folder do...?")

@rmosolgo
Copy link
Owner Author

good point! I also use app/graphql/ because we have a different API definition in app/graph :P

@rmosolgo
Copy link
Owner Author

Do ppl suffix their type names with Type?

My current project puts types in the global namespace, so most of them are #{model_class.name}Type. But, if you put them in the Types namespace, you could call them Types::#{model_class.name}. You might be asking for trouble though, since code inside the Types:: namespace would have some ambiguity when looking up #{model_class.name}.

How do you handle this?

@dphaener
Copy link

In the first project I did, I used a Type suffix also, and it has worked out well. It's a run of the mill Rails app, so there really aren't any clashes with anything, as I don't really have anything that's typed.

I do have a project where I'm already using the Types namespace (because I'm using the dry-types gem), and have solved that by putting things in a Graph namespace. It does cause some ambiguity issues like you said, and I typically have to use the ::#{model_class.name} to ensure I'm at the root namespace.

I know that using namespaces is verbose, but I really like how explicit it is. I would rather see Graph::Types::User than UserType. The former is much more clear about what that class is, and what it's doing, just because of the naming convention. And IMHO that fits well with Rails conventions.

@rmosolgo
Copy link
Owner Author

Thanks for sharing that comparison! I hadn't considered the possibility of conflicts inside Types::.

The most-recently-suggested folder structure does not have a Graph:: namespace. (It could, but it doesn't at the moment anyways)

Another seriously verbose option is Types::#{model_name}Type which is actually what I've got so far in #521, mostly because I couldn't decide between the two 😆

@dphaener
Copy link

You know, the only reason I used the Graph namespace in that project was because I had a lot of other things going on that I didn't want to clutter the GraphQL namespace with, but since the folder structure is already setup that way, I don't see why using GraphQL::Types::User would be a problem. Is there anything internally in the gem that would cause an issue there?

@rmosolgo
Copy link
Owner Author

Not yet, but in general, I can't recommend adding to namespaces you don't "own" !

@cjoudrey
Copy link
Contributor

@xuorig and I had also went with Types::User at first, but we would constantly forget to use ::User in our resolver which would cause really weird exceptions. Moving forward we were thinking about going with UserType to prevent this.

However, by dropping Types:: we'd be putting our types in the same folder as all the rest of our GraphQL stuff, which feels a bit wrong.

As for GraphQL:: vs Graph::, we went with Graph:: because it didn't feel right to add stuff to a namespace we didn't own.

@aaronklaassen
Copy link

+1 to the namespace-ownership issue mentioned above. But I think it also makes more sense conceptually to use Graph:: over GraphQL::; your types, mutations, etc are a representation of your graph, right? Not of the query language, or that language's implementation.

@rmosolgo
Copy link
Owner Author

One issue with Graph:: is, how should we work that into Rails autoloading?

Rails will expect to find those constants in files like

app/:some_folder/graph/schema.rb            # => Graph::Schema
app/:some_folder/graph/types/query_type.rb  # => Graph::Types::QueryType

What should :some_folder be in that case? Or, should we modify Rails autoloading to support a different folder structure?

(I couldn't think of a good answer, so I ended up with the structure described in #521 😆 )

@xuorig
Copy link
Contributor

xuorig commented Feb 22, 2017

I think having it in models kind of makes sense + you get the autoloading working for free without another folder?

@xuorig
Copy link
Contributor

xuorig commented Feb 22, 2017

Graph::Types::QueryType this one solves the namespace problem, but I can't help to find it weird 😭 because of that Type repetition 🤔

@aaronklaassen
Copy link

aaronklaassen commented Feb 22, 2017

@rmosolgo Yeah this is what we've been working on today, actually 😆 We did run into autoloading issues - and hacked solved it with config.autoload_paths += Dir["#{config.root}/app/"]:

app
  graph
    types
      query.rb
    mutations
    appname_schema.rb # Graph::AppnameSchema

@xuorig Yeah I'm wondering if putting it under models makes the most sense/solved the autoloading.

@xuorig
Copy link
Contributor

xuorig commented Feb 22, 2017

A lot of people also have form objects / POROs in models too, and I think graphql models / types are just that too so 🤔

@rmosolgo
Copy link
Owner Author

I also put business logic in models/, but I thought that it grinds some peoples' gears 😬

@vergenzt
Copy link

+1 to the namespace-ownership issue mentioned above. But I think it also makes more sense conceptually to use Graph:: over GraphQL::; your types, mutations, etc are a representation of your graph, right? Not of the query language, or that language's implementation.

Ehh I disagree. The types, mutations, etc. are a representation of your domain within the language of GraphQL's type system. Every type system is different and has different limitations and idioms, so simply calling it "your graph" feels too broad. It's your app's graph as viewed through the constraints and conventions of a GraphQL schema.

@aaronklaassen
Copy link

@vergenzt Yeah fair enough. I'm still new at at these GraphQL shenanigans. I care more about some conventions existing than I do about what specifically they are.

@rmosolgo
Copy link
Owner Author

Your app's graph viewed through a GraphQL schema.

I think that's one case to put it in graphql/:

In a dream world, app/controllers/ connects your business logic to HTML & HTTP requests. (In the real world ... 😖 .) Similarly, app/graphql/ is another "window" to your business logic. They draw from shared sources in order to connect clients with application code.

@rmosolgo
Copy link
Owner Author

rmosolgo commented Mar 3, 2017

Thanks for the conversation here! I've merged some generators that will be released in 1.5.0 (next week 🤞 ). If you have another suggestion about Rails onboarding, please open an issue for it!

@rmosolgo rmosolgo closed this as completed Mar 3, 2017
@KevinColemanInc
Copy link

I am running into the same issue OP has where the ID pass in is the global id and not the integer I would expect it to be. It seems like the mutations should not be responsible for translating global ids to database ids.

How do you translate relational ids to database ids in a mutation without having to do the translation in the mutation?

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

No branches or pull requests

10 participants