mercurius-js / mercurius

Implement GraphQL servers and gateways with Fastify
https://mercurius.dev/
MIT License
2.34k stars 234 forks source link

PubSub, SubscriptionContext and types #350

Open aldis-ameriks opened 3 years ago

aldis-ameriks commented 3 years ago

I'm trying to wrap my head around the subscription implementation and typescript types.

Looking at TS types:

export interface PubSub {
  subscribe<TResult = any>(topics: string | string[]): Promise<Readable & AsyncIterableIterator<TResult>>;
  publish<TResult = any>(event: { topic: string; payload: TResult }, callback?: () => void): void;
}

export interface MercuriusContext {
  app: FastifyInstance;
  /**
   * __Caution__: Only available if `subscriptions` are enabled
   */
  pubsub: PubSub;
}

We have the pubsub field on the context, which is then passed inside the resolvers and loaders. However, it seems that it's not typed as PubSub, but instead as SubscriptionContext?

But then interestingly, the same context is used as a type for context in subscription.subscribe(root, args, context). But here inside context we actually have the SubscriptionContext, not the PubSub?

I'm looking into this in the context of adding support for passing custom pubsub implementation to enable usage of other emitter libraries. However, stumbled upon this and wanted to clarify before attempting to make any changes.

mcollina commented 3 years ago

I would recommend to make the change in JS first, and then adjust the types to support said changes later.

luke88jones commented 1 year ago

@aldis-ameriks I see that you've added the functionality for custom pub-sub implementation but I wondered how you've resolved the context typing issue. We've added graphql-redis-subscriptions as a custom implementation but we've had to add some nasty typing solutions. Specifically, we've noticed that app.graphql.pubsub is our custom pub-sub implementation but in order to use the library correctly in our subscription resolvers we need to access context.pubsub.pubsub due to the subscription context actually being an instance of SubscriptionContext

aldis-ameriks commented 1 year ago

@luke88jones It's been a while since I've used mercurius. However, unfortunately, seems that the types aren't fixed yet. I recall having a similar issue as you've described and I ended up overwriting the context type.

There's a FIXME in the types which I believe is related https://github.com/mercurius-js/mercurius/blob/6782dd3a0f9d8e00e48f9758fe9a1fe8858d46e7/index.d.ts#L424

luke88jones commented 1 year ago

Thank you for replying to this old post @aldis-ameriks. I've been looking over the types quite a lot in the past few days but can't really get around this. I think I've just got to add some code comments to indicate to others why we need to access the custom pub-sub implementation in different ways for now.