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

@apollo/server + defer seems incompatible with URQL 3.0.3 #2995

Closed
3 tasks done
jtwigg opened this issue Mar 5, 2023 · 7 comments
Closed
3 tasks done

@apollo/server + defer seems incompatible with URQL 3.0.3 #2995

jtwigg opened this issue Mar 5, 2023 · 7 comments
Labels
needs more info ✋ A question or report that needs more info to be addressable won't fix 🛑 This issue won't be fixed as part of the current roadmap

Comments

@jtwigg
Copy link

jtwigg commented Mar 5, 2023

Describe the bug

When running URQL against @apollo/server@4.4.1 Im finding that the @defer keyword isn't working properly.

Using the canonical query of

query SongQuery{
  samples {
    song {
      firstVerse
      ... @defer {
        secondVerse
      }
    }
  }
}

and used in a component like:

const [result] = useSongQueryQuery()
console.log(`Song: ${JSON.stringify(result, null, 2)}`)

I'm finding that the delayed secondVerse does trigger a second re-draw of the component as though the data is coming in, however the payload is EXACTLY the same as the first payload (secondVerse) is missing.

  • Using apollo servers GraphiQL UI shows the query working as expected.
  • The "hasNext": false is occuring for the second payload.
  • I had to do some funky headers to get Apollo Server to work
headers["Accept"] = 'multipart/mixed; deferSpec=20220824, application/json, application/graphql+json'

** It feels close to working! **

image

Reproduction

Todo...

Urql version

urql@3.0.3

Validations

  • I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
  • Read the docs.
  • Follow our Code of Conduct
@kitten
Copy link
Member

kitten commented Mar 5, 2023

Honestly, I'm willing to convert this to a discussion, but I'm not willing to accept this as a bug report, I'm sorry 😅

From our standpoint, three things are important to note here right away:

  • Unfortunately, @defer and @stream are experimental spec-proposals (supported however in graphql@17 alphas and other prereleases)
  • The "Incremental Delivery" spec is an initial proposal to the GraphQL over HTTP spec (but works nicely in a normal pipeline, since we can always anticipate multipart responses)
  • It's up to servers to agree on and implement this specification (or not)

So, basically, I've extended our example and tried some of this out again. Our own server implementation with graphql-helix works unchanged when upgrading all packages and using the graphql@17 alpha.

Then I tried Apollo Server. It requires me to add Accept: multipart/mixed; deferSpec=20220824. That's fair enough, however, in our implementation we've assumed that in the future sending application/graphql+json with a @defer or @stream directive will imply that a multipart/mixed response is acceptable. (This has now changed to application/graphql-response+json which is unfortunate since in theory everyone will now have to account for both, but oh well)
So, fair enough, the spec here isn't clear, but we can assume that users can customise this and add their own Accept headers.

Unfortunately, even when adding this I can only get Apollo Server to either throw on a response or to send an empty JSON blob as a response 🤷 I'd say, the implementation here isn't working as intended.

Then I tried graphql-yoga. Unfortunately, graphql-yoga is now sending payloads like: {"incremental":ARRAY,"hasNext":true} instead.

How that has happened, I have no idea, but it's unfortunately not a format that I recognise. I would opt for supporting this in @urql/core of course, however, I can't seem to find anything in the spec that would imply that this is valid. 🤷 So, that's also confusing.

The tl;dr of this is: this is very experimental and you can't really trust most implementations of this just yet, unless you handle this yourself 😅 From my end, all I can say is, any example that's in meros will definitely work.


Edit: You can find the upgraded example servers on this branch: https://github.com/urql-graphql/urql/tree/example/upgrade-with-defer-stream/examples/with-defer-stream-directives

@kitten kitten added needs more info ✋ A question or report that needs more info to be addressable won't fix 🛑 This issue won't be fixed as part of the current roadmap labels Mar 5, 2023
@kitten
Copy link
Member

kitten commented Mar 5, 2023

@dotansimha: Actually, to quickly rope you into this, if you don't mind 😅 I'm looking at graphql-yoga's tests and there I'm actually seeing an explicit mention of incremental in the response payloads: https://github.com/dotansimha/graphql-yoga/blob/4705e559b095addeea2e05fd5a2755f4e25b2fc7/packages/plugins/defer-stream/__tests__/defer-stream.spec.ts#L131

Do you know how that came to be? I'm struggling to understand where that came from, because I can't find any mention of this in the specification of the payloads

To me this just seems off-spec, since it's a different format, but I'm sure it couldn't have come from nowhere 😅

@jtwigg
Copy link
Author

jtwigg commented Mar 5, 2023

@kitten
So the lay of the land for supporting defer with urql is:

YES: graphql-helix
NO: @apollo/server
NO: graphql-yoga
???

I appreciate the moving target this all is. I'm more coupled to URQL than I am any server (for now) and am happy with ANYTHING that works while its coming online over the next 6-12months.

@kitten
Copy link
Member

kitten commented Mar 5, 2023

Well, I'm hesitant to say that Helix works. Helix works because in our reference implementation we manually implemented the response logic 😅

I'd much rather say that it works with GraphQL Yoga, so I'm hoping we can at least get the issues with Yoga clarified and resolved

@enisdenjo
Copy link
Contributor

Hey just a quick note, the incremental field comes from the reference implementation graphql-js itself. Added with graphql/graphql-js#3659. The implementation follows the upcoming spec edits for @defer/@stream graphql/graphql-spec#742 (see incremental selection).

GraphQL Yoga simply forwards the result from graphql-js execution.

@kitten
Copy link
Member

kitten commented Mar 6, 2023

@enisdenjo Thank you so much for linking this. I really struggled to find a reference to incremental in Response payloads and thought I lost my mind.
That makes a lot of sense. I suppose this puts us in the awkward position of having to decide whether to simply adopt these changes or be backwards-compatible. I suppose, it makes sense for us to drop the old format entirely and be forwards-compatible. It could be quite a slippery slope otherwise.

Alright, that really gives me something good to work with. I'll track this in a new issue, if you don't mind, @jtwigg, since we now have a separate concern here.

Thank you so much again for linking this, @enisdenjo ❤️

@kitten
Copy link
Member

kitten commented Mar 6, 2023

Superseded by #3003

@kitten kitten closed this as not planned Won't fix, can't repro, duplicate, stale Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info ✋ A question or report that needs more info to be addressable won't fix 🛑 This issue won't be fixed as part of the current roadmap
Projects
None yet
Development

No branches or pull requests

3 participants