Add a generic promise supports

[mcg-web]

This promise adapter interface should help implement easily differed resolver without having any external dependencies.

[vladar]

That is the part which makes me wonder. Calling wait here basically means resolve promise right away. It won't execute any fields in between resolve and wait calls. So there is no deferring/buffering actually. Am I wrong?

[mcg-web]

that's where the chaining promise enters in action! let's say we have this query:

{
  user1: user(id: 1) { name friends { name }}
  user2: user(id: 2) { name friends { name }}
}

user resolver:

function ($id) use ($userDataLoader) {
  return $userDataLoader->load($id);
}

friends resolver:

function ($value) use ($userDataLoader) {
  return $value->then(function($ids) use ($userDataLoader) {
    return $userDataLoader->loadMany($ids);
  } 
}

wait will call user resolver that return promise for user 1 that is resolve by the data loader, resolving promise for user 1 will also resolve user 2 promise (thanks to dataLoader). friends for user 1 will be resolve and same for friends for user 2. only 2 requests to db against 2 + (x * friends user 1) + (n * friends user 2)...

It is difficult to show without a concrete example, thats what i'm working on...

[vladar]

Looks like your example passes promise to nested field resolvers. I didn't think about it this way, as I only considered examples where resolvers received actual parent value (resolved promise value, not promise itself).

This is interesting approach. I do have doubts about it, yet I am really interested to see where it leads to. Looking forward to see your concrete examples!

[chrissm79]

@mcg-web Love the syntax for this, it's really clean! I went the pre-fetch route for my Laravel package and was able to minify my requests to the DB, but I much prefer this promise based approach.

Like @vladar I didn't consider passing down the promises through the resolvers either. Interested to see your examples... might have to throw away my implementation :-)

[mcg-web]

Here the concrete example. Need some cleanup again but it works. 2 requests against 9 without data loader...

[vladar]

Cool. I will be able to play with it somewhere on weekend.

[vladar]

Sorry, couldn't find time on week-end, as there is lot of work now, but I remember about it %)

[vladar]

@mcg-web I played a bit with your example. It appears that it doesn't actually defer fields execution. You will see what I mean if you change your query to the simplest possible variant:

{ 
  character1: character(id: "1000") {
    name 
  } 
  character2: character(id: "1002") {
    name
  }
}

Solution with deferred fields would do 1 round-trip to storage, but both examples do 2. Query with friends actually never defers too - I put a breakpoint to friends resolver and $character argument never arrives as promise - only as array value.

The number of requests in original examples vary that much due to cacheMap in dataloader and lack of IN(?)-style request of friends in without-dataloder example.

I expected this because implementation of deferred fields just can't be that simple %) Though maybe I am missing something.

Also real test for deferreds should include at least following examples:

  stories {
    author {
      name
    }
    likers {
      edges {
        node {
          name
        }
      }
    }
  }

It covers different cases, including one where deferred entities are on different levels of query.

[mcg-web]

I'll try to defer the resolution of the promise later in the process, this should do the job...

[vladar]

Sorry I didn't realize it is not complete yet.

[mcg-web]

The sandbox-dataloader-graphql-php is up to date with the query:

{ 
  character1: character(id: "1000") {
    name 
  } 
  character2: character(id: "1002") {
    name
  }
}

I approach a little more the js implementation, is for that reason i must add more tests to be sure that promise work properly on error flow.
@vladar i'm waiting for your review :)

[vladar]

Will do - hopefully till Monday. In the meantime, I posted null value support + some other changes to master and it seems to be conflicting now with this PR. Can you resolve the conflict when you have a chance?

[vladar]

Hey that's awesome and very close to what we want! I'll post comments inline for follow-up.

[vladar]

I afraid this breaks spec compatibility in section about field execution order.

The basic idea is that root-level fields of mutations must be executed sequentially, but with this change order of their execution is not guaranteed (since if some field returns Promise we delay it until other fields complete).

Previously we always had sequential execution but with promises it is not the case anymore. So executeFields and executeFieldsSerially must behave differently.

Can you change it to behaive according to spec?

Alternative option would be to disallow Promises returned from root-level fields (for mutations only), but such limitation seems unnecessary.

[mcg-web]

yes this more an introduction to dataLoader and promise, after working on this , I'm sure now the easiest way without adding BC is to implement 100% promise support following node version... The only difference is promise will be send by executor only if need a promise. I create a little lib that abstract promise and can help to easy do that here.

[vladar]

One problem I have with wait method is that it only makes sense for sync environment. But if we integrate complete Promise support it doesn't make sense as a required part of Promise interface (say ReactPHP promises do not have to wait).

Other problem is that we will likely need to create promises in Executor some day and then single PromiseInterface will be not enough.

I had an idea to introduce PromiseAdapter interface instead of PromiseInterface, something along these lines:

interface PromiseAdapter {
  public function isPromise($value);
  public function then($promise, callable $onFulfilled, callable $onRejected); // -> Promise
  public function runTillCompletion($result); // this is the same as current Executor::completePromiseIfNeeded
}

This gives several benefits comparing to PromiseInterface:

1. runTillCompletion can be optional depending on platform
2. We do not need PromiseWrapper
3. Obviously we can add other methods here like createPromise() or createRejectedPromise(), etc if we need to extend implementation (so no new abstractions are required).

Concrete instance of PromiseAdapter will be injected to Executor.

Default NoPromiseAdapter will just throw on attempt to then and pass-through $result in runTillCompletion. GuzzleSyncPromiseAdapter can leverage wait, etc

Does it make sense? I'd like to hear your thoughs on this, maybe there are some cons which I didn't notice?

[mcg-web]

wait method is equivalent to await in node implementation (with react vs with guzzle) . this helps to get promise completion without parsing by then. Minimum interface to promise should be like that. I try a lot different promise implementation in past weeks. I can provide a full working version (with all tests) using my lib. No more need of using PromiseInterface or PromiseWrapper with this lib. I can understand that using a external requirements it not really what we want here, but promise is a more and more a global need for a lot of projects, for example thats what we use in dataloader lib. We can think of a PromiseAdapter but in a separate project, to be easily shared...

[vladar]

Hey I like your library - it's interface is similar to what I imagined. So we are likely thinking in the same direction.

But I'd like to avoid external dependencies for GraphQL lib. Actually I considered adding very simple and limited implementation for our sync case as most of the users do not need full-featured promises in sync environment.

So I'd really like to have some abstraction for Promises as a part of this library. We can add your lib to composer suggests entry for those who need full-blown promise support.

[vladar]

As for wait method. My point about wait as a part of PromiseInterface is that in event-loop environment you do not need to block ever. In ReactPHP you would expect something like this:

GraphQL::execute(...)->then(...)

You don't want to block untill all of our promises are resolved.

Also wait is simply not a part of common Promise interface. If you check ReachPHP promise interface or Promises A+ spec - they do not mention wait or block.

Guzzle does, but that's because it works in non-event loop environment. And even their promises docs mention wait in section called Synchronous Wait. They had to add it, because Guzzle HTTP client is often consumed from sync environment where devs are used to do things synchronously.

Of course you could force all promises to implement blocking wait, but for platforms with event-loops it would be an unnecessary limitation.

Also I don't think that your implementation of await for ReachPHP actually does what it claims to do, as it doesn't seem to block.

Are you sure it works as expected in ReactPHP? As for me it looks like it will work only with resolved promises, but not those which are still pending. Though maybe I am missing something.

[mcg-web]

To implement promise in this lib we need Promise.all, Promise.resolve, Promise.reject without all this it is impossible. I'll propose my full implementation using my lib and a that moment ,we'll see if it possible to do the same an another way. If you want to understand my lib install it and watch tests. event-loop need calling run method to really work, this behavior is equivalent wait.

[mcg-web]

Here the new implementation with PromiseAdapter interface, the reactPhp adapter comes out of the box. The dataloader example is also up to date. I started to implement the async tests, must finish this before merging... ping @vladar

[vladar]

@mcg-web It's awesome! I will likely do minor refactoring after merging (will convert Executor to instance to avoid passing context everywhere and instead have context and promise adapter as properties of this instance), will also try to implement sync version of adapter to avoid hasPromiseAdapter() checks (we will always have such adapter even for sync case to enable fields deferring out of the box for sync environments).

Also I will probably release 0.8.0 before we merge this in: it is a big change and we want to play with it for some time before releasing. So please let me know when it's ready for merge and I'll merge it when ready.

Thanks a lot for your great work!

[mcg-web]

I'll refactor before submitting this to merge no deal. This was a fast implementation, to confirm that we Okay before continuing. Thanks for your feedback!

[vladar]

Please rename interface to PromiseAdapter. We don't use Interface suffix anywhere else in this code base. Also we should move Promise namespace to Executor\Promise as it will only be used during Execution step.

[mcg-web]

@vladar if test run correctly and you ok, you can merge this, so we can start playing with it. I'll continue adding promise tests and documentation in others PR. Moving promiseAdapter in ExecutionContext left to you :D. This PR is becoming to huge

[coveralls]

Coverage increased (+0.1%) to 88.458% when pulling 76c31df on mcg-web:promise into 4945317 on webonyx:master.

[vladar]

@mcg-web I'll merge this somewhere this week when I get a chance. Want to give some room for 8.0 bugs to get reported (if any). Sorry for the delay.