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

[Proposal] Resolve directly vs promise #42

Closed
mcg-web opened this issue Jun 6, 2016 · 13 comments
Closed

[Proposal] Resolve directly vs promise #42

mcg-web opened this issue Jun 6, 2016 · 13 comments

Comments

@mcg-web
Copy link
Collaborator

mcg-web commented Jun 6, 2016

When using GraphQL to a higher level you might need promise resolver to manage data loader by example. My proposal it to implement this missing feature. We can use one of those two lib to full fill that need:

amphp/amp look to be the right solution but required php >= 5.5...

what do you think of this?

@jenschude
Copy link

Another possible one:

https://github.com/guzzle/promises

@vladar
Copy link
Member

vladar commented Jun 10, 2016

We should keep this library decoupled from any specific library for promises.

JS version just uses isThenable function to check if something is a promise (which by default only checks if object is returned and if it has method then).

If we choose this path, users will be able to pick any library for their app. Alternatively, I know that there is a discussion about PSR for Promises.

If they come up with some generic interface, then we could use it instead of isThenable function.

But another question is why do you need promises at all? They seem to be useless in pure PHP and only make sense for some event-loop projects like ReactPHP and possibly in HHVM. Do you work in such environment?

Because we really need at least one maintainer who uses this feature in real async environment, otherwise it might be a pain to maintain (and use for end-users).

@mcg-web
Copy link
Collaborator Author

mcg-web commented Jun 10, 2016

When working on very big schema (+250 Types), promise is very useful to gain performance. Promise helps to put performant data loaders in place, gathering a max of info before requesting DB.

Example:

lets say we have:

type Human {
  name: String!
  dogs: [Dog]
}

type Dog {
  name: String!
  owner: Human!
}

type Query {
  humans: [Human]
}

query:

query humanList {
  humans: {
    name
    dogs {
      name
      owner: { name }
    }
}

with promise we can get all Humans ids and request all Dogs for these Humans in one single request to DB.

That just an example but in a more complex schema this can help to gain execution time...

@chrissm79
Copy link

@mcg-web In order for this to work we would need to execute everything in an event loop, correct? I took a look at FB's DataLoader and that seems like a great idea for a separate package that could work with graphql-php to optimize queries to the DB. But I've yet to run this library in an event loop environment so not sure how difficult/easy that is.

@vladar Not sure how much work it would be to add a thenable check into this library, but if it could be done (and if it's maintainable) it seems that it would open up the door to some interesting possibilities.

@mcg-web
Copy link
Collaborator Author

mcg-web commented Jun 11, 2016

@chrissm79 i'm not talking of async for the moment but just deferred promise to help gather all resolvers needs before querying DB. The next step could be event-loop but in a separate project would be great.

@smolinari
Copy link

smolinari commented Jun 11, 2016

To do a Promise, or even better Observables, right within graphql-php, you'd need to alter PHP (i.e. through event loop extensions), which means graphql-php couldn't be used with standard PHP installs. In other words, to make any library work asynchronously within PHP is a major architectural decision.

Personally, I can't see PHP serving as a performant and practical GraphQL backend for any kind of major application, without making some low level changes to PHP like adding event loops, or like in our case, using a threaded PHP environment, like appserver.io. Or like Facebook using HHVM.

If you watch what Meteor is doing with their Apollo server, you can quickly imagine where PHP as a GraphQL server is going to be blown out of the water by such NodeJS solutions.

So, I guess what I am saying is, I am for the architectural decision to make graphql-php dependent on something that will make it more attractive for bigger projects. Because, if it stays as it is, I just can't imagine it ever taking off and I truly believe GraphQL has a future. Making this library was a great start for GraphQL in the PHP world. We shouldn't limit it, which plain vanilla PHP does, unfortunately.

Modern apps need modern APIs like graphql-php. The PHP internal devs need to finally get PHP into the big leagues with threading, event looping or asynchronous functions or anything along those lines. It's a huge decision for them I know, which needs a lot of poking and argumentation. This library being dependent on anything outside the PHP box will help make those arguments for sure!

My 2 cents. 😄

Scott

@vladar
Copy link
Member

vladar commented Jun 13, 2016

@mcg-web But you can't use Dataloader to defer requests with promises unless you are in event-loop/async environment. To rephrase - when/where will you call resolve on your deferreds?

All promise libraries mentioned here do require specific environment (ReactPHP for React/Promise, amphp for amphp/amp, guzzle is mainly used for http requests where async tools are possible with curl or non-blocking sockets).

But maybe I am missing something, so please let me know if I do. Example would help a lot.

At the same time I do agree that leveraging Dataloader concept would be beneficial. But this will require deeper modification of execution process with another step in execution process to run promise resolvers. Alternatively we could do this without promises (but just adding defer callback to field definition along with resolve)

@mcg-web
Copy link
Collaborator Author

mcg-web commented Jun 13, 2016

@vladar not really, React/Promise and [React/Event-loop] are totally separated. That mean that we can use promise in a sync environment to deferred execution of resolver (defer callback can be the solution for Dataloader in fact). Don't have examples for the moment but I'll try to work on a POC as soon as possible.

@vladar
Copy link
Member

vladar commented Jun 13, 2016

@smolinari In terms of performance, graphql-php application has little difference over any complex php application, so your statement about PHP (in it's current form) being bad backend for graphql applies to any complex PHP application.

As for this library it will stay focused on vanilla PHP. Yet if we manage to integrate promises then event-loop based engines for PHP could use it as well.

But what we really need to make it work - is a person (or group) who works with graphql-php in threaded (or event-loop) envrionment and can report bugs or even help with maitainance in such environment.

@ooflorent
Copy link

ooflorent commented Jun 13, 2016

graphql-js has an opened experimental PR (graphql/graphql-js#304) to introduce execution phases. If implemented in PHP, it would enable deferred data fetching.

@vladar
Copy link
Member

vladar commented Jun 13, 2016

@ooflorent Thanks for bringing this up. I've been watching for this thread for a while. I think it requires much more efforts and tests before it could be integrated. This stuff is really complex with it's own pros and cons.

Yet in vanilla PHP we need something like this to enable batching queries (e.g. with Dataloader). I also followed similar discussion in dotnet implementation - graphql-dotnet/graphql-dotnet#21

@smolinari
Copy link

smolinari commented Jun 13, 2016

so your statement about PHP (in it's current form) being bad backend for graphql applies to any complex PHP application.

Agreed.

But what we really need to make it work - is a person (or group) who works with graphql-php in threaded (or event-loop) envrionment and can report bugs or even help with maitainance in such environment.

I'll do my best, when I get to that point. 😄

Scott

@vladar
Copy link
Member

vladar commented Sep 14, 2016

I'll close it for now. Feel free to reopen if you have new ideas or pull requests.

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

No branches or pull requests

6 participants