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

deferred fragments on Query produces undefined key in data response #3122

Closed
3 tasks done
tompuric opened this issue Apr 1, 2023 · 4 comments · Fixed by #3124
Closed
3 tasks done

deferred fragments on Query produces undefined key in data response #3122

tompuric opened this issue Apr 1, 2023 · 4 comments · Fixed by #3124

Comments

@tompuric
Copy link

tompuric commented Apr 1, 2023

Describe the bug

Hi, i've been exploring adding the @defer directive to our graphql server and believe I've come across an issue with urql.

When deferring on the root Query fragment, urql will respond with an undefined key in the data response.

Versions:

  • "@graphql-yoga/plugin-defer-stream": "^1.7.1"
  • "@urql/core": "^4.0.1"
  • "graphql": "17.0.0-alpha.2"
  • "urql": "^4.0.0"

Expected behavior

Entire query is resolved and merged together

  data: {
      song: { firstVerse: "Now I know my ABC's.", secondVerse: "Next time won't you sing with me?" },
  },

Actual behavior

Query is split via an undefined key

  data: {
      song: { firstVerse: "Now I know my ABC's." },
      undefined: { song: [Object] }
  },

This was verified by using the with-defer-stream-directives example with the following script below

import { Client, gql, debugExchange, fetchExchange } from 'urql';
import fetch from 'node-fetch';

const SONGS_QUERY = gql`
  query App_Query {
    ... on Query @defer {
      song {
        secondVerse
      }
    }
    song {
      firstVerse
    }
  }
`;

const client = new Client({
  url: 'http://localhost:3004/graphql',
  fetch: fetch,
  exchanges: [debugExchange, fetchExchange],
});

client.query(SONGS_QUERY).then(response => {
  console.log('LOG response: ', response);
});

This is resolved if we do not @defer the root Query fragment. E.g.

  query App_Query {
    song {
      firstVerse
      ... on Song @defer {
        secondVerse
      }
    }
  }

However I'm unable to do so ATM due to lack of schema stitching support (ardatan/graphql-tools#1941)

I believe this to be a bug from the side of urql

Reproduction

Apologies, found this difficult to setup

Urql version

urql v4.0.0

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
@tompuric tompuric changed the title deferred fragments on Query produces deferred fragments on Query produces undefined key Apr 1, 2023
@tompuric tompuric changed the title deferred fragments on Query produces undefined key deferred fragments on Query produces undefined key in data response Apr 1, 2023
@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Apr 1, 2023

Hey,

I quickly looked into your issue and found a solution from our side, however I am not entirely sure yet on whether we want to support the full breadth of this issue as it seems unspecified in the spec in the depth of what's allowed.

Basically when you do a deferred fragment on a root-type like this then path will be empty for streamed results, this means we have to merge them a bit differently compared to what we have been doing. When reasoning about this issue I mainly start worrying in how deep this could go as at a certain point we'd have to start doing deep-merges on these objects which could become problematic when it comes to lists/..

This issue also seems highly specific to @defer as @stream most likely won't be specified on the root-type and will keep delivering consistent results.

@tompuric
Copy link
Author

tompuric commented Apr 1, 2023

Thank you for the quick response and hasty fix PR, much appreciated.

I understand the concerns where you're coming from here. It's an odd situation here where the server supports defer on the root-type but the spec recommends differently. Ideally I would not be querying the manner stated in this issue but currently need to due to schema stitching not supporting defer yet (deferring on the root type is the workaround).

However, since this query is allowed, and urql doesn't parse it correctly, it's a gap in urqls capabilities (imo).
If this isn't resolved moving forward, we could workaround this by doing the merge ourselves

@kitten
Copy link
Member

kitten commented Apr 1, 2023

no need, really ✌️ the linked patch PR will fix this and add the missing deep-clone merging as well, which was missing. The spec wasn't too clear on it being necessary.

@kitten
Copy link
Member

kitten commented Apr 1, 2023

Fixed in @urql/[email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants