Skip to content

Add Rails Generators#521

Merged
rmosolgo merged 10 commits intomasterfrom
generators
Feb 27, 2017
Merged

Add Rails Generators#521
rmosolgo merged 10 commits intomasterfrom
generators

Conversation

@rmosolgo
Copy link
Copy Markdown
Owner

@rmosolgo rmosolgo commented Feb 5, 2017

I think generators could make the Rails experience much better! We started chatting about them over at #511 . I've sketched out a few possibilities here, what do you think?

TODO:

  • Settle on naming conventions
  • Update yardocs
  • Write a guide

@dphaener
Copy link
Copy Markdown

dphaener commented Feb 6, 2017

👍 This looks like a great start. I think I would also vote for a generator that ties a bunch of those together. I typically like to create the model/type/mutations/resolvers all at the same time.

@rmosolgo
Copy link
Copy Markdown
Owner Author

rmosolgo commented Feb 6, 2017

That's a good point about tying them together -- I know Rails has hooks for rails g model (that's how Rspec makes tests and factory girl makes factories) so we could look into those too!

@dphaener
Copy link
Copy Markdown

dphaener commented Feb 6, 2017

My personal opinion on that would be to not automatically tie it in to a model generation, but rather wrap up the model generation in a "Higher Order" generator of sorts. Something like what I'm doing here: https://github.com/DNACodeStudios/react-rails-bootstrap/blob/master/lib/generators/graphql_resource_generator.rb

That makes it more of an opt in API, rather than an opt out.

rails g graphql_resource User email:string password_digest:string could then generate a model, type, mutations, etc. Or you could just generate each thing separately.

@vergenzt
Copy link
Copy Markdown

I know @theorygeek is working on goco-inc/graphql-activerecord--would that be the most appropriate place for Rails- and/or ActiveRecord-specific code?

#
# ```
# - app/
# - graphql/
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+💯

(re: #511 (comment))

#
# Accept a `--graphiql` option which adds
# `graphiql-rails` to the Gemfile and mounts the
# engine in `routes.rb` (Should this be the default?)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be default?

I'd think so. :) It seems like someone not wanting GraphiQL will likely be the exception and not the norm.

# ```
#
# Should we have a loader for the
# nearly-universal foreign key loader?
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a loader for the nearly-universal foreign key loader?

👎 Personally I think any code that's "nearly-universal" should probably be in a gem rather than generated into the application codebase. Not sure whether graphql-ruby is the most appropriate gem or not--but maybe graphql-activerecord?

@rmosolgo rmosolgo force-pushed the generators branch 2 times, most recently from 3dd7989 to 9158613 Compare February 16, 2017 04:24
# - query_type.rb
# - loaders/
# - mutations/
# - {app_name}_schema.rb
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for prefixing with {app_name}_ besides the fact that otherwise we'd be conflicting with GraphQL::Schema?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I did this in my project to avoid file picker conflicts with db/schema.rb. (I thought it was a real Ruby naming conflict, but looking back, I see that it's not.) Maybe that's not a good enough reason to do so!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'm a fan of namespacing the schema name. Rails autoloading expects the filename to match the constant name, and having the app-wide constant for the schema simply be called Schema doesn't seem right to me. :/

#
# ```ruby
# # config/routes.rb
# post "/graphql", to: "graphql_queries#create"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just graphql#create?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be good, then you could even do something like

resource :graphql, only: :create

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe:

post "/graphql", to: "graphql#create"

@rmosolgo rmosolgo modified the milestone: 1.5.0 Feb 17, 2017
@rmosolgo
Copy link
Copy Markdown
Owner Author

I pushed an install generator along the same lines as initially described, how do you think in played out?

I still need to give it a run with rails new and write some docs.

@rmosolgo
Copy link
Copy Markdown
Owner Author

Thanks for you input along the way! I've tried to cover all the ground here. Does anyone want to take another look before I get to documenting it? Any reasons not to recommend this kind of setup?

ps @dphaener Thanks for clarifying that. I think I'll start with some "simple" generators and then explore those options!

create_dir("app/graphql/types")
template("query_type.erb", "app/graphql/types/query_type.rb")
template("schema.erb", "app/graphql/#{schema_name.underscore}.rb")
template("graphqls_controller.erb", "app/controllers/graphqls_controller.rb")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be graphql_controller.rb instead? Pluralized GraphQL feels a bit weird to me.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be, then we'd have to switch:

- resource :graphql, only: :create 
+ post "/graphql", to: "graphql#create"

I guess it's not really a resource anyways, old habits die hard :P

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder then if we should keep create as the action. Sounds a bit off, maybe something custom like execute?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's nice, I updated to post "/graphql", to: "graphql#execute"!

case ambiguous_param
when String
if ambiguous_param.present?
JSON.parse(ambiguous_param)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't necessarily be a hash if ambiguous_param is "[]". We might want to deal with that?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could assert that the parsed value is a Hash.

I just realized that this won't work on Rails 5 since ActionController::Parameters won't match when Hash 😖

I'll take a look at whether I can return the parameters as-is or whether they need to be to_unsafe_hashed.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it worked ok with GraphiQL since it sends the body as a string. I'll improve the messaging a bit in case someone does send random input

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

donezo

@@ -0,0 +1,5 @@
<%= type_ruby_name %> = GraphQL::ObjectType.define do
name "<%= type_graphql_name %>"
<% if options.node %> interfaces [GraphQL::Relay::Node.interface]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use implements GraphQL::Relay::Node.interface now. 🎉

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

Copy link
Copy Markdown
Owner Author

@rmosolgo rmosolgo Feb 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

<% end %><% if options[:batch] %>
# GraphQL::Batch setup:
lazy_resolve(Promise, :sync)
instrument(:query, GraphQL::Batch::Setup)
Copy link
Copy Markdown
Contributor

@cjoudrey cjoudrey Feb 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Moved discussion to GraphQL-Batch: Shopify/graphql-batch#49

@rmosolgo
Copy link
Copy Markdown
Owner Author

I added a guide and yanked ResolverGenerator -- I'll add a FunctionGenerator along with #545 instead

@rmosolgo rmosolgo merged commit 0e0198b into master Feb 27, 2017
@rmosolgo rmosolgo deleted the generators branch February 27, 2017 14:12
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.

5 participants