Lazy resolution API

[rmosolgo]

Here's another take on #274 , borrowing heavily from GraphQL::Batch's execution strategy.

Accept some user-provided objects that can create response values, but haven't yet. Then, later, trigger cascading resolution of those pending values.

I have a branch of GraphQL::Batch which uses this API

---

Another thing here is moving default execution to GraphQL::Execution::Execute. Here are some goals for this:

• Move execution out of the query namespace to make it clear that they ought not be coupled
• Refactor the existing execution flow: don't make instances of FieldResolution, instead, use a plain method call (This refactor made big dividends when applied to other classes in SerialExecution)
• Expose a smaller public API

I'm leaving SerialExecution in place because graphql-batch depends on a few parts of it. It's important to me that graphql-batch keeps working smoothly, so I won't remove it until we find a better integration point for graphql-batch's features and that integration point has done well "in the wild" for a little while.

That said, I do want to avoid the mistake I made with SerialExecution. Inviting people to "extend" things (which I should have made private) made refactoring hard, especially because these internals didn't have good specs covering the expectations of their users. As a result, there were a handful of compatibility bugs which were hard to predict (for me, anyways).

So, I'm hoping to provide move execution into the "library internals" category and offer this API instead!

[junosuarez]

Awesome to see this experimentation and adopting ideas from graphql-batch. Rather than creating a Boxed class to hold values, using an existing third party implementation, such as concurrent-ruby's Future would mean less code for graphql-ruby to test and maintain, and also fewer novel APIs for users to learn. Also, code for composing promises or futures should be part of that library, rather than included in graphql-ruby.

[rmosolgo]

Thanks for taking a look!

less code ... fewer novel APIs

While working on this commit, I kept putting in and taking out some third-party libraries 😆 Here are my thoughts for now:

• This implementation adds a very small API: just one method (Schema#boxed_value), plus the ability to return instances of a user-chosen class. If I coded against library X, users of libraries Y and Z would have new APIs to learn!
• I don't consider dependencies to be "free" code. By adding a dependency, you expose yourself to upstream bugs or poor maintenance. It looks like Shopify's use of promise.rb has not been free: https://github.com/lgierth/promise.rb/graphs/contributors. Mike Perham says it best in "Kill Your Dependencies".
• I'd like to support users of libraries X, Y and Z. Given that they all do basically the same thing, if its only 100-200 lines of code for generic support, it's worth it to me!

Lastly, I have a personal anecdote which influences my preference for avoiding a third-party dependency: early in graphql-ruby's development, a certain early adopter said "We're ready to put this in production, but we can't include ActiveSupport or Celluloid as dependencies." Fortunately, they were easy to remove, so we did. But I've kept an eye out for that ever since. I want to make graphql-ruby an easy choice for importing into existing projects, and part of that means keeping the dependencies down for people who take those issues seriously.

code for composing promises or futures should be part of that library

I agree! GraphQL::Batch could continue using using Promise.rb for this purpose, which has proven to be very valuable!

[dylanahsmith]

Rather than creating a Boxed class to hold values, using an existing third party implementation, such as concurrent-ruby's Future would mean less code for graphql-ruby to test and maintain

The problem is that it is coupled to concurrency. That is why I went with promise.rb, since it isn't coupled to a given method of resolving the promises. It's Promise class can be subclasses for concurrent promises (e.g. using concurrent-ruby's Future), parallel promises (e.g. eventmachine) or batch promises (e.g. GraphQL::Batch::Promises).

By adding a dependency, you expose yourself to upstream bugs or poor maintenance. It looks like Shopify's use of promise.rb has not been free. The library uses 100% mutation coverage, so bugs aren't really a concern, most of the work was getting the tests working again by updating the gems used for testing.

Actually, I took over maintenance of promise.rb, so it is basically free.

I'd like to support users of libraries X, Y and Z. Given that they all do basically the same thing, if its only 100-200 lines of code for generic support, it's worth it to me!

promise.rb has only 230 lines of code that provides generic support for promises. Note that your implementation currently is missing support for error handling.

However, using an adapter approach would allow you to leverage code in a typical promise implementation (e.g. promise.rb or Concurrent::Promise from concurrent-ruby) without adding a dependancy. So rather than creating Boxed so that you can call methods on them, you could instead call class methods on an adapter that pass in a object, letting it determine if it is a promise and how to chain methods on it.

This approach definitely is looking a lot better though over implementing batching in-house!

[rmosolgo]

missing support for error handling

Thing is, I'm not sure I want error handling! Each spot that the gem captured errors "for you" has come back to bite me. In the end, it turns out to be hiding useful information from users. For example, consider the old debug: or the recent (good) suggestion to let users handle coercion errors.

For that reason, I'm interested in an "agnostic" approach: users tell the library how to get a value. If an error is raised, it's raised (or the user may rescue it). Otherwise, the return value is treated as "GraphQL as usual."

[rmosolgo]

you could instead call class methods on an adapter

Could you help me understand this option better? At first I had in mind something more like an "adapter", but then I found calling a method on the object was sufficient for the one case at hand, but I wonder if I'm overlooking something ...

[dylanahsmith]

Each spot that the gem captured errors "for you" has come back to bite me.

I didn't mean for internal errors, I meant in cases like this where there is already exception handling logic. For instance, if a non-null field raises a GraphQL::ExecutionError, then that exception wouldn't be caught unless done by the registered unboxing method, and even if that method catches and returns the error it will result in a GraphQL::InvalidNullError that won't get caught by parent nullable fields.

However, maybe this could be simplified by doing this GraphQL::InvalidNullError error propagation by returning the error rather than raising the exception.

[rmosolgo]

Oh, I misunderstood! Yes, I definitely plain to maintain all the existing behavior ... but propagating nulls is harder when the response is already half-built :S

I'm exploring an approach where you keep the type info alongside the result, so that if a lazy value returns nil, you can backtrack to find a nullable field.

[rmosolgo]

Pushed with:

• Proper error handling & null propagation
• Rename "boxed" => "lazy"
• API docs

Also pushed an update to my graphql-batch branch where all tests are passing :D

I think this is sufficient and it should improve the integration between the two libraries. Is there anything still missing?

I'll write a guide for this feature before merging it.

[dylanahsmith]

This is a pretty massive PR. I think because you aren't sharing code at all between the two execution strategies. That also means execution changes will have to be done on both execution strategies. In contrast, the graphql-batch execution strategies are only 88 lines in total, since they use method overriding, so seem like a lot less maintenance to bring over in a way that is less tied to promise.rb

[dylanahsmith]

Oh, I thought the extra execution strategy would be to avoid extra overhead from the changes. Is the actual reason that you are worried about other execution strategies that inherit from SerialExecution and wanted to duplicate the code so you could refactor it?

[rmosolgo]

duplicate the code so you could refactor

Yes, exactly! Sorry I forgot to mention that above.

Next steps are to deprecate SerialExecution, and perhaps the execution_strategy API altogether. I hope to cover the previous uses (ok, use 😆 ) of that API in other ways.

[dylanahsmith]

It seems like you have been doing a lot of refactoring of SerialExecution lately. Are you just trying to not break graphql-batch? If so, then that leaves a lot of surface area that can break, since it only depends on SerialExecution#execute, FieldResolution#get_finished_value and FieldResolution#execution_context.strategy. SerialExecution could even be renamed if the class is also assigned to the SerialExecution constant.

[rmosolgo]

I've been refactoring in search of performance improvements and code simplifications. Yes, I've been trying not to break graphql-batch! I don't quite understand your point about surface area, could you say it again?

[dylanahsmith]

My point was just that a lot of refactoring can be done without breaking graphql-batch by just keeping those few methods in place. That way you wouldn't need a copy of the execution strategy to provide asynchronous support.

[rmosolgo]

Oh I see. If graphql-ruby has an API like this one, does graphql-batch still depend on SerialExecution? I'll update the description of this PR with some thoughts in this area.