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

FieldExtension: pass extended values instead of originals to after_resolve #3168

Conversation

swalkinshaw
Copy link
Collaborator

This is a follow-up after #3138 which had breaking changes for our application without a real way to work around it. This proposes a solution to make extensions dealing with extended arguments more flexible.

Field Extensions let you extend (modify) the resolver object and arguments in the resolve hook. Previously the after_resolve hook would receive the original object and arguments and not the extended ones. This was a lossy process with no way for after_resolve to access the extended values.

This changes after_resolve to receive the extended values instead. If you need access to the original non-extended values, the memo argument can be used.

Example:

class MyExtension < GraphQL::Schema::FieldExtension
  def apply(field)
    field.argument(:my_new_arg, :string, required: false)
  end

  def resolve(object:, arguments:, context:)
    next_args = arguments.dup
    next_args.delete(:my_new_arg)
    yield(object, next_args, { original_arguments: arguments })
  end

  def after_resolve(value:, context:, arguments:, memo:, **_args)
    # `arguments` => extended/modified arguments
    # `memo` => hash with `original_arguments`
  end
end

cc @jturkel what do you think?

@swalkinshaw
Copy link
Collaborator Author

Should be noted this is another breaking change after #3138 as well.

@swalkinshaw
Copy link
Collaborator Author

Oh I'll probably add a more explicit test case for this behaviour

@rmosolgo
Copy link
Owner

Should I wait for another test before merging this? I'm happy with the test coverage as-is, but also happy to wait if you want to push a proper test for this behavior.

@swalkinshaw
Copy link
Collaborator Author

😅 I completely forgot. I do want to add another test

@swalkinshaw swalkinshaw force-pushed the extensions-pass-extended-values-to-after-resolve branch from 026040a to 53330be Compare September 29, 2020 21:29
@swalkinshaw
Copy link
Collaborator Author

Added a test

@swalkinshaw swalkinshaw force-pushed the extensions-pass-extended-values-to-after-resolve branch from 53330be to 6f11b75 Compare September 29, 2020 21:30
Field Extensions let you extend (modify) the resolver object and
arguments in the `resolve` hook. Previously the `after_resolve` hook
would receive the *original* object and arguments and not the extended
ones. This was a lossy process with no way for `after_resolve` to access
the extended values.

This changes `after_resolve` to receive the extended values instead. If
you need access to the original non-extended values, the `memo` argument
can be used.

Example:

```ruby
class MyExtension < GraphQL::Schema::FieldExtension
  def apply(field)
    field.argument(:my_new_arg, :string, required: false)
  end

  def resolve(object:, arguments:, context:)
    next_args = arguments.dup
    next_args.delete(:my_new_arg)
    yield(object, next_args, { original_arguments: arguments })
  end

  def after_resolve(value:, context:, arguments:, memo:, **_args)
    # `arguments` => extended/modified arguments
    # `memo` => hash with `original_arguments`
  end
end
```
@swalkinshaw swalkinshaw force-pushed the extensions-pass-extended-values-to-after-resolve branch from 6f11b75 to 147c317 Compare September 30, 2020 14:11
@rmosolgo rmosolgo added this to the 1.11.6 milestone Sep 30, 2020
@rmosolgo
Copy link
Owner

Thanks!

@rmosolgo rmosolgo merged commit bebd78c into rmosolgo:master Sep 30, 2020
@swalkinshaw swalkinshaw deleted the extensions-pass-extended-values-to-after-resolve branch September 30, 2020 15:56
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.

2 participants