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

@defer initial support #2642

Merged
merged 16 commits into from
May 21, 2023
Merged

@defer initial support #2642

merged 16 commits into from
May 21, 2023

Conversation

StevenACoffman
Copy link
Collaborator

@UnAfraid @fiatjaf PR with one extra commit

This implements @defer support according to https://github.com/graphql/graphql-wg/blob/66a80c7e7ff5d881a7de515eec991ca3150c829b/rfcs/DeferStream.md

There are no tests and there may be some bugs yet, but I'm just opening this to ask if this change is desired and/or if the code quality is too bad to be considered -- before I put more time polishing it and writing tests.

(Don't look at the first commit, please, just look at the final result.)

There is a demo app at https://github.com/fiatjaf/gqlgen-defer-demo, you can try it live on https://defer.fiatjaf.com/debug/. Only the fields availability, here and there are concurrent, so they're the only ones that can be deferred (the others will just return in the normal response regardless). Try the query

query {
  fruits {
    name
    color
    availability {
      ... on Availability @defer {
        there
      }
      here
    }
  }
}
screencast.mp4

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

Coverage Status

Coverage: 78.834% (-0.3%) from 79.102% when pulling 0a7fbba on defer_patched into 4d945da on master.

@StevenACoffman StevenACoffman merged commit c313bf3 into master May 21, 2023
@StevenACoffman StevenACoffman deleted the defer_patched branch May 21, 2023 03:25
This was referenced May 21, 2023
@fiatjaf
Copy link
Contributor

fiatjaf commented May 21, 2023

Awesome, this is amazing.

@StevenACoffman
Copy link
Collaborator Author

@fiatjaf @UnAfraid Thanks for your help! I would really like to get some tests around the defer behavior if you get another chance.

@fiatjaf
Copy link
Contributor

fiatjaf commented May 21, 2023

Yes, will do. Now that you two have fixed all the things that should be easy.

@fiatjaf
Copy link
Contributor

fiatjaf commented May 22, 2023

Turns out not having hasNext breaks graphiql native support for @defer

@fiatjaf
Copy link
Contributor

fiatjaf commented May 30, 2023

I'm using this in production and finding some race condition bugs. The updated version is on my branch defer-prod. I'll come back here to propose the fixes and add tests once it is looking more stable and I get some time.

@danielcondemarin
Copy link

Thanks for your work on this @fiatjaf @StevenACoffman !
As I understand it @stream isn't supported with this change, how hard do you think would be to introduce support now that @defer has been merged? With a few pointers maybe myself or someone else can take a stab at it

@fiatjaf
Copy link
Contributor

fiatjaf commented Jun 13, 2023

oh, I saw this comment and decided it was a good time for me to bring my new changes and add some tests to the defer feature, but it looks like @UnAfraid had already done that along with a few fixes, and had even written tests! Amazing. Thank you!

@fiatjaf
Copy link
Contributor

fiatjaf commented Jun 13, 2023

@danielcondemarin I think @stream would be independent and somewhat different from @defer, although the same principles of having a thing that is pending and running on the background could be used. I suggest that you familiarize yourself with the generated code that drives all the resolver logic. That's what I did at least, using an IDE to navigate through the code.

https://github.com/fiatjaf/gqlgen-defer-demo/blob/master/gql_generated.go could be a good start, since it is very minimal and has defer stuff in it.

I am not very familiar with how @stream is supposed to work, but I'm also interested in that, so I might try to implement it if you don't, maybe in a couple of months.

@fiatjaf
Copy link
Contributor

fiatjaf commented Jun 13, 2023

I think the idea is that @stream only works in fields that return lists, right? Then instead of returning the full list it would return one element at a time.

I guess then you would have to allow resolvers to be declared on user code as returning chan Type instead of []Type. If they return chan Type you mark them as Streamable. Then when you call the resolver and get the channel you start listening to it and dispatching the results to the response -- maybe like executionContext.deferredResults exists today you can create a executionContext.streamResults that aggregates all streamed items from all fields in all resolvers.

After describing it like that it seems simple. The part that I'm not familiar at all with is how to make the code generator detect functions returning chan as valid resolvers for list types.

@danielcondemarin
Copy link

danielcondemarin commented Jun 14, 2023

I think the idea is that @stream only works in fields that return lists, right? Then instead of returning the full list it would return one element at a time.

Yeah my understanding is only for list fields, client can specify how many items to receive on the first response back, then from that point the server can return one or more at a time.
The idea of using a chan Type sounds good to me, I'll spend some time this weekend familiarising myself with the repo and hopefully get something working.

@carldunham
Copy link
Contributor

Hey, all. Any updates on @stream support? I might have some cycles to help out this week, if it will help!

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.

6 participants