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

Feature: GraphQL Support #1189

Open
AndiLavera opened this issue Mar 28, 2020 · 10 comments
Open

Feature: GraphQL Support #1189

AndiLavera opened this issue Mar 28, 2020 · 10 comments
Assignees

Comments

@AndiLavera
Copy link
Contributor

AndiLavera commented Mar 28, 2020

Hi, I saw #265 and tried it out. I have a working implementation that i think it is quite nice but i am open to feedback.

I wanted to create PR that adds some command/template that would install GraphQL to an amber project. The PR itself would add no new dependencies to amber. The command would probably be amber g graphql. I am not aware of any flags/options yet.

What would happen once the command is run:

Amber files that would be edited:
shard.yml:

  graphql-crystal:  
    github: ziprandom/graphql-crystal  
    branch: master  

application.cr

# below models
require "graphql-crystal"
require "../src/graphql/queries/*"
require "../src/graphql/mutations/*"
require "../src/graphql/schema"
require "../src/graphql/*"
# above controllers

Files that would be added:

- src
    |
    - graphql
    |
      - queries   # holds all GraphQL queries with query_type consolidating the modules
         |
         - query_type.cr
         - *{model}_queries.cr*   # Like user_queries.cr
         |
      - mutations   # holds all mutations with mutation_type consolidating the modules
         |
         - mutation_type.cr
         - *{model}_mutations.cr*   # Like user_mutations.cr
      - schema.cr   # GraphQL schema

Dev's using the framework would then specify a route & controller add include the query, mutation & schema like so:

class GraphQLController < ApplicationController
  include QueryType
  include MutationType
  include Schema

  def index
    schema.execute(graphql_params["query"])
  end

  private def graphql_params
    params.validation do
      required :query
    end
  end
end

Hiccups, side effects & things you should know and should be discussed:

  1. I would prefer the code in application.cr to be in an initializer but the GraphQL controller was throwing an errors because QueryType, MutationType & Schema modules are not defined.

  2. As per the graphql_crystal documentation, you would have to add fields that are queriable to your model like so:

class Post < Granite::Base
  connection mysql
  table posts # Name of the table to use for the model, defaults to class name snake cased

  column id : Int64, primary: true # Primary key, defaults to AUTO INCREMENT
  column name : String? # Nilable field
  column body : String # Not nil field

  # GraphQL
  # `id` would not be queriable
  include GraphQL::ObjectType
  field :name
  field :body
end
  1. I am not currently suggesting adding a command to generate the base for a query file like we do with models (amber g model User email:string). If we did add this, you could essentially run amber g model User email:string, amber g graphql-query User email:string. I am thinking the second command could insert the fields into the model & generate a base query file & add the type to the schema. However, i am not too keen on this. I think it would be cool it hook into the generating model command and adding these graphql files if a certain flag/yml setting is set similar to rails api mode.

Let me know what you think. The graphql shard can be found here

@drujensen
Copy link
Member

I could see adding a new attribute to the column definition to define which fields are queryable.

column name : String?, graphql: true

@AndiLavera
Copy link
Contributor Author

That would be a nice addition. I can take a look at granite after making a PR.

A concern has come up though. The author of the shard is MIA from all of github. Contributions to anything are null for 4 months. Furthermore, he isn't even available for issues or releases.

Idk what to do. I plan on forking it in the future and adding some features but no clue when that will be. Maybe I'll pick this idea up when/if that happens.

@eliasjpr
Copy link
Contributor

eliasjpr commented Apr 6, 2020

I think is a great addition @andrewc910 feel free to propose a PR. We can all collaborate. There is support for custom generators in amber you might want to take a look at that first. Let us know where we can help.

@AndiLavera
Copy link
Contributor Author

@eliasjpr Can you point me in the direction of those generators? I can only find recipes. I don't think this would be a good recipe as it's not really a 'base' per say. Like amber with react is a base. Maybe if you were planning only an api amber project. I thought some way to install graphql at any life cycle would be nice.

None the less, i am not fully taking over the graphql project but i am currently updating it, adding some tests & doing some clean ups so ameba stop yelling at me. If the shard creator comes back, i would request merging mine into his master branch. At the very least, i can create a simple tutorial on the readme for integrating with amber.

@itsezc
Copy link

itsezc commented May 5, 2020

This would be epic!
One could always look to projects like Prisma for inspiration.

@eliasjpr
Copy link
Contributor

@andrewc910 look into this PR #1197 which will allow this type of integration much smoother. You can work with @damianham for questions in regards the plugin approach.

I am very much looking forward to see in the next release of amber :)

@damianham
Copy link
Contributor

@andrewc910 @eliasjpr yes I think the plugin approach is the best way to add features like GraphQL support to Amber projects. PR #1197 makes minimal changes to existing code other than adding to application.cr to load plugins, a shard for modular rendering and refactors recipe_fetcher.cr to DRY up the code. Using the plugin approach, I don't think any changes would be required to grantite or any other base code if we could add a generate subcommand to the plugin command so that plugins could add their own generators and take arguments appropriate to the generator, e.g. given a plugin called graphql

$ amber plugin generate graphql query user **args**
$ amber plugin generate graphql mutation user **args**

A big advantage of the generator being in the plugin is that it would keep the Amber base code lean. Of course this depends on PR #1197 being approved and merged into the Amber codebase. As for the issue of the original shard owner MIA - I suggest to fork the shard into https://github.com/ambercommunity where we can all collectively contribute and to avoid this problem in future.

@AndiLavera
Copy link
Contributor Author

Hey guys, thanks for the updates. Been pretty busy with work since covid but in a few weeks my schedule should clear up. Still super interested in this.

Glad the plug-in support is making progress. I definitely support the plug-in approach over integrating graphql into Amber/granite.

The shard author merged by crystal version update a few weeks ago. I forked the repo and did quite a bit of code clean up. I'll try and contact the author to gauge his interest. If he is still active, the plug-in will be based off that. If he has little to no interest, I'll move my fork into the Amber community repo after I finish cleaning it up a bit.

@itsezc
Copy link

itsezc commented Apr 2, 2021

@awcrotwell any updates on your fork? I'd love to use Amber the only thing holding me back right now is a lack of support for GraphQL.

@atik7
Copy link

atik7 commented Apr 26, 2021

Any update on graphql?

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

6 participants