`@stream` is broken

[maraisr]

Hi 👋🏻

Looks like you have a working test that is in fact broken:

https://github.com/contrawork/graphql-helix/blob/d32567bbaa5852e91786126080247e54e32613f8/packages/core/test/integration.test.ts#L171

Should be hasNext: false

Here is a graphql-js example test to confirm this behaviour:

https://github.com/graphql/graphql-js/blob/a09e09c8d5e0322a855224a449f661087583c317/src/execution/__tests__/stream-test.ts#L157-L174

[n1ru4l]

@maraisr The graphql-js test case you are referring to is not using an AsyncGenerator resolver that yields values, but a resolver that resolves to an Array. I assume because of that graphql-js publishes the hasNext: false as part of the last result that includes an item. Instead of a separate execution result

I created the following code-sandbox: https://codesandbox.io/s/pedantic-glitter-8tudl?file=/src/index.ts

It showcases that for an AsyncGenerator the following results are published from execute:

const schema = new graphql.GraphQLSchema({
  query: new graphql.GraphQLObjectType({
    name: "Query",
    fields: {
      stream: {
        type: new graphql.GraphQLList(graphql.GraphQLString),
        resolve: async function* () {
          for (const word of ["ay", "sup", "oi"]) {
            yield word;
          }
        }
      }
    }
  })
});

[
  {
    "data": {
      "stream": [
        "ay"
      ]
    },
    "hasNext": true
  },
  {
    "data": "sup",
    "path": [
      "stream",
      1
    ],
    "hasNext": true
  },
  {
    "data": "oi",
    "path": [
      "stream",
      2
    ],
    "hasNext": true
  },
  {
    "hasNext": false
  }
]

Here is an example where the resolver only returns an Array of elements (similar to what the graphql-js test you are referring to is doing https://codesandbox.io/s/cool-hamilton-orizq?file=/src/index.ts

const schema = new graphql.GraphQLSchema({
  query: new graphql.GraphQLObjectType({
    name: "Query",
    fields: {
      stream: {
        type: new graphql.GraphQLList(graphql.GraphQLString),
        resolve: () => ["ay", "sup", "oi"]
      }
    }
  })
});

[
  {
    "data": {
      "stream": [
        "ay"
      ]
    },
    "hasNext": true
  },
  {
    "data": "sup",
    "path": [
      "stream",
      1
    ],
    "hasNext": true
  },
  {
    "data": "oi",
    "path": [
      "stream",
      2
    ],
    "hasNext": false
  }
]

---

The helix test you are referring to is working as expected. As the chunks[2] string is the following:

Content-Type: application/json; charset=utf-8
Content-Length: 47

{"data":"C","path":["stream",2],"hasNext":true}
---
Content-Type: application/json; charset=utf-8
Content-Length: 17

{"hasNext":false}
-----

I agree that the test should also check for the presence of the {"hasNext":false} string part though! I created #85 for this.

[maraisr]

In any case, will have this behaviour verified with the working group, because I suspect it should flush hasNext: false on the "last" payload.

[n1ru4l]

@maraisr An AsyncGenerator can sometimes not determine whether a yielded item is the last item as it might need to make roundtrips to a database first. It would be kind of weird to block the publishing of the current value until the next value has been retrieved just to save some bandwidth caused by sending an additional {"hasNext":false} payload over the wire.

[n1ru4l]

In any case, this is not a concern of graphql-helix as it just wraps graphql-js. So all your concerns are actually graphql-js related and should be raised on the graphql-js repository and be discussed in the working group.