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

specify the path instead of creating a deferred response from the root #1469

Closed
Tracked by #80
Geal opened this issue Aug 5, 2022 · 9 comments · Fixed by #1529
Closed
Tracked by #80

specify the path instead of creating a deferred response from the root #1469

Geal opened this issue Aug 5, 2022 · 9 comments · Fixed by #1529
Assignees

Comments

@Geal
Copy link
Contributor

Geal commented Aug 5, 2022

on this query:

query ProductById {
  product(id: "apollo-federation") {
    id
    ... on ProductItf @defer {
      package
    }
  }
}

we get:

{
  "data": {
    "product": {
      "id": "apollo-federation"
    }
  },
  "hasNext": true
}

then:

{
  "data": {
    "product": {
      "package": "@apollo/federation"
    }
  },
  "hasNext": true
}

while the second response should be:

{
  "data": {
    "package": "@apollo/federation"
  },
  "path": [ "product" ],
  "hasNext": true
}
@Geal Geal self-assigned this Aug 5, 2022
@BoD
Copy link

BoD commented Aug 5, 2022

An example with lists.

Query

query AllProducts {
  allProducts {
    id
    ... on Product @defer {
      hidden
    }
  }
}

Router current behavior

{"data":{"allProducts":[{"id":"apollo-federation"},{"id":"apollo-studio"}]},"hasNext":true}
{"data":{"allProducts":[{},{}]},"hasNext":true}
{"hasNext":false}

What we would expect

{"data":{"allProducts":[{"id":"apollo-federation"},{"id":"apollo-studio"}]},"hasNext":true}
{"data":{"hidden":null},"path":["allProducts",0],"hasNext":true}
{"data":{"hidden":null},"path":["allProducts",1],"hasNext":false}

We never expect to have to merge lists, instead we expect each item to have its own payload. This allows the client to know at each payload what has been received or not.

@Geal
Copy link
Contributor Author

Geal commented Aug 5, 2022

that one with lists is unlikely to happen, considering how the query plan is built. And the separate {"hasNext":false} is necessary because there's no reliable way to know when a deferred response is the last one

@BoD
Copy link

BoD commented Aug 5, 2022

The separate {"hasNext":false} is not an issue on our side 👍

For the lists: to clarify, on our side, the need is to have a 1:1 mapping between payloads and deferred fragments.

@alessbell
Copy link
Member

The separate {"hasNext":false} is also not an issue on our side. We would, however, also expect each item in a list to have its own payload.

Given graphql/defer-stream-wg#46, can we consider this question similarly resolved?

@Geal
Copy link
Contributor Author

Geal commented Aug 17, 2022

ok so #1529 should fix it, and the good news is that after thinking about this for a while, the only correct way I see to implement it solves both points at once, using a path prefix and generating one response per list element

@Geal
Copy link
Contributor Author

Geal commented Aug 18, 2022

@BoD @alessbell this will go in the next release, probably early next week

@BoD
Copy link

BoD commented Aug 18, 2022

very cool! 🙏

@BoD
Copy link

BoD commented Aug 22, 2022

Tested with v0.16.0, it's looking good on Apollo Kotlin's side! 🎉

@Geal
Copy link
Contributor Author

Geal commented Aug 22, 2022

Nice 😄

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

Successfully merging a pull request may close this issue.

3 participants