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

Add support for inline and named fragments #6

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add support for inline and named fragments #6

wants to merge 8 commits into from

Conversation

khamusa
Copy link
Collaborator

@khamusa khamusa commented Sep 8, 2017

Add support for inline fragments and named fragments, supporting queries such as:

fragment ChefFragment on chef {
  name
  recipes {
    title
    ingredients {
      name, quantity
      vendor {
        name
      }
    }
  }
}

query {
  restaurant(id: 1) {
    ... on restaurant {
      name
      owner {
        ... ChefFragment
      }
    }
  }
}

I had to pass a reference to the hash that contains the fragment definitions through the whole method chain, since it is only available from the context variable. I'd suggest we refactor a module and use an instance of a class to hold current state in order to avoid passing so many parameters on method calls. something like:

module GraphQL
  module QueryResolver
    def self.run(*args, &block)
      Resolver.new(*args).call(&block)
    end

    class Resolver
      attr_reader :model_class, :context, :return_type
      def initialize(model_class, context, return_type)
        # save values
      end

      def call
        to_load = yield
        # and so on...
      end     
  end
end

If you like the idea I can perform the refactoring as well

@nettofarah
Copy link
Owner

That actually sounds really great, @khamusa.
Let's try the refactoring idea :)

@khamusa
Copy link
Collaborator Author

khamusa commented Sep 10, 2017

@nettofarah let me know what you think!

end

dependencies
def preloadable_reflection?(class_name, selection_name)
Copy link
Owner

Choose a reason for hiding this comment

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

This whole section is great. Love the abstraction :)

def map_dependencies(class_name, ast_node, dependencies = {})
ast_node.selections.each do |selection|
if inline_fragment?(selection) || relay_connection_using_nodes?(selection)
dig_deeper = selection
Copy link
Owner

Choose a reason for hiding this comment

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

dig_deeper sounds a little weird. But I can't quite think of an alternative name here...

Copy link
Owner

Choose a reason for hiding this comment

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

it is almost like: "we want to replace the current selection with this nested option".

next
end
name = selection.name
next unless preloadable_reflection?(class_name, name)
Copy link
Owner

@nettofarah nettofarah Sep 10, 2017

Choose a reason for hiding this comment

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

I'm not a big fan of using unless in cases like this.
I would replace this with:

if !preloadable_reflection?(class_name, name)
  next
end

Mostly because I find this easier to parse visually

Copy link
Owner

@nettofarah nettofarah left a comment

Choose a reason for hiding this comment

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

This looks awesome.
Left a few comments on code style, mostly so we can keep this consistent with the rest of the codebase.

@khamusa
Copy link
Collaborator Author

khamusa commented Sep 11, 2017

@nettofarah thanks for the comments! Take a look at the last two three commits.

Regarding the dig_deeper (now to_proceed), I'm not 100% satisfied either. I can think of alternatives for refactoring but they involve switching to a more OO style which I'd rather not do since this code runs for every field and creating more objects could potentially impact performance.

@khamusa
Copy link
Collaborator Author

khamusa commented Sep 11, 2017

By the way, I don't know how you prefer to manage versions, but I also added these latest changes to the changelog, so that we don't forget what's to be added later. If you prefer I can remove it as well.

@khamusa
Copy link
Collaborator Author

khamusa commented Sep 11, 2017

@nettofarah please hold this, i'm investigating an issue with relay pagination

@nettofarah
Copy link
Owner

@khamusa sounds good! Take your time

@nettofarah
Copy link
Owner

Hey, @khamusa. Did you learn anything new here?

@khamusa
Copy link
Collaborator Author

khamusa commented Nov 23, 2017

hey @nettofarah I'm sorry I haven't been able to work on this anymore. The issue I found was actually this one: #9

I found out that the strategy we're using here was pretty much useless for relay connections.

I do think this PR is unrelated though..

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.

None yet

2 participants