Subscribe property of a graphql subscription has wrong return type (AsyncIterable vs AsyncIterator)

[emyann]

Hello!

I think the return type of the Subscriber has the wrong type. It says AsyncIterable while I think what is expected is an AsyncIterator

pothos/packages/deno/packages/core/types/builder-options.ts

Line 18 in 731a570

export type Subscriber<Parent, Args, Context, Shape> = (parent: Parent, args: Args, context: Context, info: GraphQLResolveInfo) => MaybePromise<AsyncIterable<Shape>>;

Below is a screenshot of the Typescript error. pubSub here is an instance import {PubSub} from 'graphql-subscriptions'
I tested it at runtime while casting to any and the subscription worked as expected. I also modified the type from AsyncIterable to AsyncIterator in @pothos/core/dts/types/builder-option.d.ts as a sanity check and it fixed the typing mismatch

I'm happy to create a PR if you can confirm that's an actual bug :)

[hayes]

I think it is correct as is. The issue is likely that the object you are returning technically implements both the iterable and iterator interfaces, but only has type definitions for the iterator part.

An async iterable is an object that has a property with the asyncIterable symbol returning the iterator (this is the case here: https://github.com/apollographql/graphql-subscriptions/blob/master/src/pubsub-async-iterator.ts#L69-L71)
This is used by graphql here:
https://github.com/graphql/graphql-js/blob/main/src/execution/mapAsyncIterator.ts#L11

[hayes]

The explicit type definition here:  https://github.com/apollographql/graphql-subscriptions/blob/master/src/pubsub-engine.ts#L7-L8 prevents inferring the more accurate return type of PubSubAsyncIterator which would likely satisfy the expected asyncIterable return type.

[emyann]

Thank you @hayes for the deep dive! I'll try to remove the explicit return definition (from graphql-subscriptions and see what happens :)

An async iterable is an object that has a property with the asyncIterable symbol returning the iterator (this is the case here: https://github.com/apollographql/graphql-subscriptions/blob/master/src/pubsub-async-iterator.ts#L69-L71)

It looks like in the link you have shared the asyncIterator symbol is used and not the asyncIterable ? Sorry if I'm a bit confused 😅

What I found suspicious is when I tried to satisfy the type, it was not working at runtime. I'm an old Nexus GraphQL user happily moving to Pothos ❤️ and I remember the expected type was an AsyncIterator so I thought maybe there is something to dig

[hayes]

It looks like in the link you have shared the asyncIterator symbol is used and not the asyncIterable ? Sorry if I'm a bit confused 😅

Whoops, that was just a typo in my previous comment, was writing from a phone

I am not 100% sure here, but I believe that nexus is also incorrect here. I think a lot of libraries dealing with subscriptions and iterators/iterables get this wrong. Part of the confusion is that all the built in iterables (arrays, maps, sets, etc) and async iterables (async generator functions) expose iterators/async iterators that are themselves iterables as well. Many custom iterators follow this same pattern. In general this means that the overwhelming number of iterators will work mostly as expected when passed somewhere that accepts in iterable (eg a for-of, or a graphql subscription). This is usually implemented in iterators by having something like [Symbol.asyncIterator]: () => this. (eg. for all built in iterators: https://tc39.es/ecma262/#sec-asynciteratorprototype-asynciterator) or https://github.com/apollographql/graphql-subscriptions/blob/master/src/pubsub-async-iterator.ts#L69-L71 for graphql-subscriptions.

In general the way to think about iterators vs iterables is: Iterables are things that can be iterated over (array, maps, sets, etc). iterators are the stateful iteration of that collection. eg an array is iterable, but each time you iterator over it you create an iterator which knows where in the array the current iteration is, and how to get the next value. Async iterators/iterables are very similar. There are some things like generators that return something that is both iterable (has the Symbol.iterator/Symbol.asyncIterator property, that return this) and implements the iterator interface. These are things that generally can't be looped over an arbitrary number of times. This is the category of things that a pubsub iterator falls into.

Things that iterate over something almost always are designed to accept an iterable rather than an iterator. This is a good practice for a number of reasons: One of the most important is that is trivial to detect iterables (look for the iterator/asyncIterator symbol prop, so you know what type if object you are dealing with. Another reason is that iterators are always stateful, and designed to be consumed once. Iterables on the other hand have a function to return an iterator, meaning that if you have a source that can be iterated over multiple times, it can return a new iterator for each consumer (array, sets, maps, etc). This isn't true for async generators or the pubsub iterator, but it's better to support it whenever possible.

There are a lot of libraries out there that get this wrong, and I think both graphql-subscriptions and nexus are among those. It's possible that I am missing something, or some of my understanding here is not quite right, but I believe that subscriptions are a case where an async iterable (something that has a [Symbol.asynctIterator] property that returns an async iterator is expected. There are specific checks for this in the graphql-execution code that branch on detecting an iterable.

Here is a demo showing how an iterable works with graphql (without pothos) if you change it to return the iterator directly, you will see it no-longer works: https://stackblitz.com/edit/typescript-enxt6z?file=index.ts

[emyann]

@hayes Thanks for detailing your rationale about that decision. I went and look at the graphql.js implementation and I think what you said makes sense about the proper expected type when I look at this code

https://github.com/graphql/graphql-js/blob/67aefd9c1daefceacf937363c2da9e1117e550d9/src/execution/execute.ts#L1285-L1291

And then translate this statement into code

An async iterable object is any object that has a Symbol.asyncIterator property whose value is a function that returns an AsyncIterator

which corresponds to the code you share through stackblitz.

So if I understand it right, the right way to fulfill the types expected by Pothos and also have a right implementation at runtime (without casting stuff) would be to do that below ?

    subscribe: () => {
      const iterator =  pubSub.asyncIterator('some-channel')
      return {[Symbol.asyncIterator]: () => iterator}
    },

I tried it and it worked but I'd be happy to have a sanity check on that. Also if that's the case would it make sense to add that to the documentation ?

---

A subsequent question I had to this issue was how I could make sure the type coming from the iterator is properly inferred to the payload ?

I actually have something like

type Payload = GetSubscriptionPayloadType<ReturnType<typeof getSubscription>>
function getSubscription() {
  return subscriptionService.createSubscription('onFileUrlUpdate')
}

builder.subscriptionFields((t) => ({
  onFileUrlUpdated: t.prismaField({
    type: 'FileUrl',
    nullable: true,
    subscribe: () => {
      const iterator = subscriptionService.createSubscription('onFileUrlUpdate')
      return {[Symbol.asyncIterator]: () => iterator}
    },
    resolve: async (query, payload) => {
      const {id} = payload as Payload
    },
  }),
}))

where subscriptionService.createsubscription implementation looks like

  createSubscription<T extends PubSubChannel, R extends SubscriptionReturnType<T>>(channel: T) {
    return pubSub.asyncIterator<R>(channel)
  }

and that helper type extract the message type this way

export type GetSubscriptionPayloadType<T> = T extends AsyncIterator<infer U> ? U : never

I tried first to let it being inferred but it didn't work and it's fine as I had the same issue with Nexus (thus the reason why the helper function extracting the payload type exists). Then I dug in the generic of t.field and t.prismaField but was not able to pass the type as I ultimately would like to avoid casting like I do const {id} = payload as Payload, but I was not able to find a proper way, do you have any advice on how I could achieve that 😃

Thanks you a lot! I really appreciate the time you put into these thoughts

[hayes]

So if I understand it right, the right way to fulfill the types expected by Pothos and also have a right implementation at runtime (without casting stuff) would be to do that below ?

subscribe: () => {
  const iterator =  pubSub.asyncIterator('some-channel')
  return {[Symbol.asyncIterator]: () => iterator}
},

This works, and will make typescript happy. The real fix is just to update the pubSub.asyncIterator( to return the right type. it already returns an object with [Symbol.asyncIterator]: () => iterator} which means its a valid iterable, it just isn't typed correctly.

I tried first to let it being inferred but it didn't work and it's fine as I had the same issue with Nexus (thus the reason why the helper function extracting the payload type exists). Then I dug in the generic of t.field and t.prismaField but was not able to pass the type as I ultimately would like to avoid casting like I do const {id} = payload as Payload, but I was not able to find a proper way, do you have any advice on how I could achieve that 😃

This was a bug in t.prismaField, it did not correctly account for subscriptions. I pushed a fix that should make it work correctly now, the return type of subscribe should automatically be inferred and used as the parent value of the resolver now. This was already working for non prisma based subscriptions, but t.prismaField was overwriting resolve option in a way that was not correctly accounting for the possibility of parent changing bases on the subscription. Should be fixed now.

[hayes]

opened an issue here to track this: apollographql/graphql-subscriptions#261

[emyann]

Oh wow that's awesome! Thank you for the quick fix! I fetched the most recent version of @pothos/plugin-prisma and everything works as expected! 💯

[MikaStark]

Thanks @hayes for all the explanations and all the work you've done so far by the way. I had a very bad time trying to make it work and this issue saved my day. Now I have a complete project migrated from Nexus to Pothos :)