apollographql / graphql-subscriptions

:newspaper: A small module that implements GraphQL subscriptions for Node.js
MIT License
1.58k stars 133 forks source link

Return type of pubsub.asynctIterator should include AsyncIterable #261

Open hayes opened 2 years ago

hayes commented 2 years ago

the pubsub.asyncIterator method currently is typed as returning an AsyncIterator. While this is accurate, it misses an important detail:

The subscribe function of a graphql field expects an AsyncIterable, not an AsyncIterator. At runtime this works because the PubSubAsyncIterator also implements the AsyncIterable interface. Unfortunately this is hidden by the explicit return type in pubsub.asyncIterator Which is typed as returning an AsyncIterator, not a PubSubAsyncIterator, and does not include the [Symbol.asyncIterator] property.

This library is very commonly used when implementing graphql subscriptions, and I believe this has resulted in it becoming common to mis-type the expected return type of subscribe functions (for example in nexus)

This should be a relatively easy, and backwards compatible fix

kefniark commented 1 year ago

An ugly hack for anyone else running into it

You can redefine your own withFilter and ResolveFn, with as described above AsyncIterator => AsyncIterable

import { withFilter as withFilterOriginal, FilterFn } from 'graphql-subscriptions'

type ResolverFn<TSource = any, TArgs = any, TContext = any> = (
  rootValue?: TSource,
  args?: TArgs,
  context?: TContext,
  info?: any
) => AsyncIterable<any>

export const withFilter = withFilterOriginal as unknown as (
  asyncIteratorFn: ResolverFn,
  filterFn: FilterFn
) => ResolverFn

It's a really annoying issue, because it's a pure typing problem and it's probably a one liner to fix here

Aeolun commented 1 year ago

For those not interested in switching to a whole different server to fix a minor issue, I've published a fork here: Aeolun/graphql-subscriptions-continued that basically just releases the release-3.0 branch in this repository.

It doesn't update the dependent packages (e.g. graphql-redis-subscriptions), but if they feel like it they can switch.