Add support for inline and named fragments#6
Add support for inline and named fragments#6khamusa wants to merge 8 commits intonettofarah:masterfrom letsevents:feature/fragments-support
Conversation
|
That actually sounds really great, @khamusa. |
|
@nettofarah let me know what you think! |
| end | ||
|
|
||
| dependencies | ||
| def preloadable_reflection?(class_name, selection_name) |
There was a problem hiding this comment.
This whole section is great. Love the abstraction :)
lib/graphql/query_resolver.rb
Outdated
| 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 |
There was a problem hiding this comment.
dig_deeper sounds a little weird. But I can't quite think of an alternative name here...
There was a problem hiding this comment.
it is almost like: "we want to replace the current selection with this nested option".
lib/graphql/query_resolver.rb
Outdated
| next | ||
| end | ||
| name = selection.name | ||
| next unless preloadable_reflection?(class_name, name) |
There was a problem hiding this comment.
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
endMostly because I find this easier to parse visually
nettofarah
left a comment
There was a problem hiding this comment.
This looks awesome.
Left a few comments on code style, mostly so we can keep this consistent with the rest of the codebase.
I guess it reflects intention better.
|
@nettofarah thanks for the comments! Take a look at the last Regarding the |
|
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. |
|
@nettofarah please hold this, i'm investigating an issue with relay pagination |
|
@khamusa sounds good! Take your time |
|
Hey, @khamusa. Did you learn anything new here? |
|
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.. |
Add support for inline fragments and named fragments, supporting queries such as:
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:
If you like the idea I can perform the refactoring as well