Allow loaders to be used without an executor

[dylanahsmith]

@rmosolgo, @xuorig & @cjoudrey

Problem

As pointed out in #33 (comment), when using graphql-batch outside of the execution strategy, we run the risk of leaky state. This could be a problem when using the graphql gem's new lazy resolution API, but it could also be a problem in tests.

As such, I think we should move away from depending on global state, even if we support one for convenience and backwards compatibility.

The thread local executor is used for three purposes right now:

1. stores a list of loaders
2. executes those loaders
3. keeps track of the loading flag that unit tests can check to catch unbatched queries

Solution

promise.rb 0.7.2 now keeps track of the source of the promise and uses that to sync the promise by default. All that is needed to use that is to set the promise source on the promise returned by the loader's load method.

Without the need to use the executor for resolve the promises, we could then create per request loaders as documented in the dataloader README which could then be stored in the graphql query context. Doing this also allows loaders' cache to be used after their batch load is performed. However, this does bring the concern of cache invalidation if the loaders are used across top-level mutation fields.

For now the loading flag is kept on the executor and is just not updated when there is no current executor. That could be fixed in the future by moving the flag outside of the executor into it's own thread-local variable, but will more likely be replaced by a more generic instrumentation hook.

Follow-up

I plan on having the executor not get created automatically and require the code using the library to specify where batching starts and stops using a block. E.g.

product = GraphQL::Batch.batch do
    RecordLoader.for(Product).load(product_id)
end

would setup the executor for the block and pass the result of the block to Promise.sync

[xuorig]

do you still need as_promise_unless_resolved with Promise.sync ?

[xuorig]

Ah. of course you do, but I wonder if the name still makes sense for that method ?

[dylanahsmith]

as_promise_unless_resolved recursively looks for promises in the result and wraps the whole result in a promise if it finds any. Promise.sync only checks if the object itself is a promise

[rmosolgo]

👍 I think I see, basically if you .wait a promise which was created without an executor.current, it will resolve that promise's loader (its source)