Skip to content

Conversation

@matthewowen
Copy link

@matthewowen matthewowen commented Jul 18, 2018

I'm opening this PR as an exploratory suggestion for custom directives: at Flexport we're using this library and have a bunch of use cases that custom directives could really help with.

This PR doesn't yet have specs or docs and certainly isn't complete, but I wanted to throw it out there as at least a strawman for a possible direction for this feature: from reading the associated issues, it seems that custom directives has stalled out a little?

The high level is that we allow a directive to define specific hooks, and at the appropriate points in parsing and executing a query we can then call into those hooks. In this PR, we allow directives to define a hook to be called at query execution time, such that when when resolving a field the directive is inserted and can return some other value, call into the original resolve proc, modify the arguments to the inner resolve proc, etc.

A minimal example of use would be as follows:

Graph::Directive::NullIfFooDirective = GraphQL::Directive.define do
  name "null_if_foo"
  description "Directs the executor to make this field null if the provided argument is the string 'foo'"
  locations([GraphQL::Directive::FIELD])

  argument :special_string, GraphQL::STRING_TYPE, "Skipped if this value is the string 'foo'"
  default_directive false

  resolve_execution ->(directive_args, object, field_args, context, inner_proc) {
    roles = directive_args[:roles]
    if directive_args[:special_string] == "foo"
      return nil
    else
      return inner_proc.call(object, field_args, context)
    end
  }
end

In this case, given a query like

hero {
  name
  friends @null_if_foo(special_string: "foo") {
    name
  }
}

we would receive

{
  "data": {
    "hero": {
      "name": "R2-D2"
    }
  }
}

But with a query like

hero {
  name
  friends @null_if_foo(special_string: "bar") {
    name
  }
}

we would receive

{
  "data": {
    "hero": {
      "name": "R2-D2",
      "friends": [
        {
          "name": "Luke Skywalker"
        }
      ]
    }
  }
}

I think an approach that allows defining hook points on directives would be a pretty powerful way to provide flexibility. I would imagine that the two key points one would want to hook in at would be during AST generation (as skip and include do) and during query execution (as I've implemented here). I think we'd also want to ensure that these hook points are appropriately implemented for fragments as well as fields.

I guess the high level question I have here: do you consider this to be a plausible approach, and would you be receptive to me pursuing it further? And if so, what are the areas you'd like to see focused on (setting aside obvious details here like documentation and testing)?

@rmosolgo
Copy link
Owner

I love this idea! Thanks for sharing your sample code too.

Personally, I've shifted my thoughts about execution to a complete rewrite, although I haven't actually hacked on it since my initial spike at #1394 . Custom directives is definitely a requirement for a new executor. I was hoping to have made more progress on that by now but, uh, ... other things ... took priority at work 😆 That's how it goes sometimes.

I can't really pitch in on this one right now, but if you're able to get something working with minimal overhead, I'm definitely open to adding it.

@matthewowen matthewowen force-pushed the custom_resolve_execution_directive branch 2 times, most recently from 772141f to 6002d2f Compare July 28, 2018 00:36
@matthewowen matthewowen force-pushed the custom_resolve_execution_directive branch 5 times, most recently from 8db255c to cf1a267 Compare July 28, 2018 01:10
@matthewowen matthewowen force-pushed the custom_resolve_execution_directive branch from cf1a267 to c0e5e08 Compare July 28, 2018 01:28
@rmosolgo
Copy link
Owner

I'm not going to merge this branch in particular, but keep an eye on #1394 where we'll have custom directives of one sort or another, thanks again for your work here!

@rmosolgo rmosolgo closed this Sep 10, 2018
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