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

Allow return raw values from resolvers #2699

Merged
merged 6 commits into from Feb 13, 2020

Conversation

DmitryTsepelev
Copy link
Contributor

@DmitryTsepelev DmitryTsepelev commented Jan 27, 2020

This PR allows to return raw values from resolvers. When Interpreter gets such a value, it should write it directly to the response, instead of visiting all the child nodes. This is how it could look like:

class Query < GraphQL::Schema::Object
  field :expansion_raw, Expansion, null: false
  
  def expansion_raw
    raw_value(sym: "RAW", name: "Raw expansion")
  end
end

This change is a first step of implementing partial response caching (current solutions are very limited and skip various built-in checks). Here is an example gist by @palkan, which contains the same change (using metaprogramming) as well as caching implementation.

What do you think? Do you have suggestions on the API design?

@DmitryTsepelev DmitryTsepelev requested a review from rmosolgo Feb 6, 2020
Copy link
Owner

@rmosolgo rmosolgo left a comment

I'm open to it! Thanks for sharing your thoughts here and digging in to the implementation. I just have a couple questions below.

lib/graphql/execution/interpreter/handles_raw_value.rb Outdated Show resolved Hide resolved
@object = obj
end

alias_method :resolve, :object
Copy link
Owner

@rmosolgo rmosolgo Feb 6, 2020

Choose a reason for hiding this comment

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

Why have an alias for this method?

Suggested change
alias_method :resolve, :object

Copy link

@palkan palkan Feb 6, 2020

Choose a reason for hiding this comment

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

The idea is to use resolve everywhere for consistency.
We assume that users/libraries would be able to sub-class RawValue and implement their own resolving logic.

For example, CachedValue could be smth like:

class CachedValue < GraphQL::Execution::Interpreter::RawValue
  def resolve
     cache_store.read(object.cache_key) || object
  end
end

Copy link
Owner

@rmosolgo rmosolgo Feb 10, 2020

Choose a reason for hiding this comment

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

Thanks for clarifying, that makes sense to me 👌

In that case, maybe we can get rid of attr_reader :object ?

Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev Feb 10, 2020

Choose a reason for hiding this comment

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

Agreed, removed it

lib/graphql/execution/interpreter/runtime.rb Show resolved Hide resolved
GRAPHQL

res = InterpreterTest::Schema.execute(query_str)
assert_equal({ sym: "RAW", name: "Raw expansion", always_cached_value: 42 }, res["data"]["expansionRaw"])
Copy link
Owner

@rmosolgo rmosolgo Feb 6, 2020

Choose a reason for hiding this comment

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

DmitryTsepelev and others added 2 commits Feb 8, 2020
Co-Authored-By: Robert Mosolgo <rmosolgo@github.com>
Co-Authored-By: Robert Mosolgo <rmosolgo@github.com>
@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Feb 9, 2020

I've applied two suggestions, let's figure out if we want to go with that alias_method

lib/graphql/execution/interpreter/runtime.rb Outdated Show resolved Hide resolved
lib/graphql/execution/interpreter/runtime.rb Outdated Show resolved Hide resolved
@object = obj
end

alias_method :resolve, :object
Copy link
Owner

@rmosolgo rmosolgo Feb 10, 2020

Choose a reason for hiding this comment

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

Thanks for clarifying, that makes sense to me 👌

In that case, maybe we can get rid of attr_reader :object ?

DmitryTsepelev and others added 3 commits Feb 10, 2020
Co-Authored-By: Robert Mosolgo <rmosolgo@github.com>
Co-Authored-By: Robert Mosolgo <rmosolgo@github.com>
@DmitryTsepelev
Copy link
Contributor Author

@DmitryTsepelev DmitryTsepelev commented Feb 13, 2020

@rmosolgo Could you please take a look at it again when you have a second?

@rmosolgo rmosolgo added this to the 1.10.3 milestone Feb 13, 2020
@rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Feb 13, 2020

Thanks for the bump. Looks good! It will be interesting to see where this goes 😈

@rmosolgo rmosolgo merged commit 1ad95df into rmosolgo:master Feb 13, 2020
1 check passed
@DmitryTsepelev DmitryTsepelev deleted the returning-raw-values branch Aug 28, 2020
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

3 participants