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

loosen resolvers subscribe return type #7015

Merged
merged 4 commits into from
Nov 14, 2021
Merged

Conversation

n1ru4l
Copy link
Collaborator

@n1ru4l n1ru4l commented Nov 12, 2021

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Nov 12, 2021

🦋 Changeset detected

Latest commit: 69332db

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphql-codegen/typescript-resolvers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@theguild-bot
Copy link
Collaborator

theguild-bot commented Nov 12, 2021

The latest changes of this PR are available as alpha in npm (based on the declared changesets):

@graphql-codegen/[email protected]

@Urigo
Copy link
Collaborator

Urigo commented Nov 12, 2021

maybe worth adding a test case with the IxJS types?

@dotansimha
Copy link
Owner

maybe worth adding a test case with the IxJS types?

It can also be with dev-test examples

@dotansimha dotansimha merged commit 3d57ec6 into master Nov 14, 2021
@dotansimha dotansimha deleted the fix-async-iterable branch November 14, 2021 08:31
@Cellule
Copy link

Cellule commented Nov 22, 2021

This change seems incompatible with https://github.com/apollographql/graphql-subscriptions which uses AsyncIterator not AsyncIterable and the 2 types are not compatible.
I'm wary of changing any runtime code since what we have currently is working in apollo

@dotansimha
Copy link
Owner

graphql-subscriptions considered legacy implementation for subscriptions. It doesn't have any added value with today's tools.
@n1ru4l from what I understand, the AsyncIterable is the correct type here, so I guess graphql-subscriptions is just using the incorrect type?

@n1ru4l
Copy link
Collaborator Author

n1ru4l commented Nov 23, 2021

graphql-subscriptions is using an incorrect type. I created a PR for fixing it a long time ago. Nobody from apollo seems to care. We shouldn't generate wrong typings because of some unmaintained library. AsyncIterable (which has a Symbol.asyncIterator property) is the correct return type here. You can use patch-package or as any if you use graphql-subscriptions.

I wouldn't say it is completely legacy as it is still used by a lot of people (especially the distributed redis version that builds on graphql-subscriptions). It is sad to see the package in this state. All offers for helping out or transferring the package to someone that actually cares for the community are ignored (and nobody seems to feel responsible 🤷‍♀️).


relevant links

apollographql/graphql-subscriptions#232
apollographql/graphql-subscriptions#240

@Cellule
Copy link

Cellule commented Nov 23, 2021

Thanks for the explanation. Since we're using Apollo, I don't want to diverge from their implementation.
I did use patch-package to change it back in our codebase so all good on my end, just wanted to understand the reasoning behind the change :D

@n1ru4l
Copy link
Collaborator Author

n1ru4l commented Nov 23, 2021

@Cellule You can also patch graphql-subscriptions, this is what I am doing in a project where we are using redis:

diff --git a/node_modules/graphql-subscriptions/dist/pubsub-engine.d.ts b/node_modules/graphql-subscriptions/dist/pubsub-engine.d.ts
index 58fa1b1..c8168e5 100644
--- a/node_modules/graphql-subscriptions/dist/pubsub-engine.d.ts
+++ b/node_modules/graphql-subscriptions/dist/pubsub-engine.d.ts
@@ -2,5 +2,5 @@ export declare abstract class PubSubEngine {
     abstract publish(triggerName: string, payload: any): Promise<void>;
     abstract subscribe(triggerName: string, onMessage: Function, options: Object): Promise<number>;
     abstract unsubscribe(subId: number): any;
-    asyncIterator<T>(triggers: string | string[]): AsyncIterator<T>;
+    asyncIterator<T>(triggers: string | string[]): AsyncIterable <T>;
 }
diff --git a/node_modules/graphql-redis-subscriptions/dist/redis-pubsub.d.ts b/node_modules/graphql-redis-subscriptions/dist/redis-pubsub.d.ts
index af38928..c4784f3 100644
--- a/node_modules/graphql-redis-subscriptions/dist/redis-pubsub.d.ts
+++ b/node_modules/graphql-redis-subscriptions/dist/redis-pubsub.d.ts
@@ -20,7 +20,7 @@ export declare class RedisPubSub implements PubSubEngine {
     publish<T>(trigger: string, payload: T): Promise<void>;
     subscribe<T = any>(trigger: string, onMessage: OnMessage<T>, options?: unknown): Promise<number>;
     unsubscribe(subId: number): void;
-    asyncIterator<T>(triggers: string | string[], options?: unknown): AsyncIterator<T>;
+    asyncIterator<T>(triggers: string | string[], options?: unknown): AsyncIterable<T>;
     getSubscriber(): RedisClient;
     getPublisher(): RedisClient;
     close(): Promise<Ok[]>;

In other projects where I only have a single server instance I use the following PubSub instead:

// Node.js events module
import { EventEmitter, on } from "events";

export const createChannelPubSub = <
  TPubSubPublishArgsByKey extends PubSubPublishArgsByKey
>() => {
  const emitter = new EventEmitter();

  return {
    publish: <TKey extends Extract<keyof TPubSubPublishArgsByKey, string>>(
      routingKey: TKey,
      ...args: TPubSubPublishArgsByKey[TKey]
    ) => {
      if (args[1] !== undefined) {
        emitter.emit(`${routingKey}:${args[0] as number}`, args[1]);
      }

      emitter.emit(routingKey, args[0]);
    },
    subscribe: async function* <
      TKey extends Extract<keyof TPubSubPublishArgsByKey, string>
    >(
      ...[routingKey, id]: TPubSubPublishArgsByKey[TKey][1] extends undefined
        ? [TKey]
        : [TKey, TPubSubPublishArgsByKey[TKey][0]]
    ): AsyncGenerator<
      TPubSubPublishArgsByKey[TKey][1] extends undefined
        ? TPubSubPublishArgsByKey[TKey][0]
        : TPubSubPublishArgsByKey[TKey][1]
    > {
      const asyncIterator = on(
        emitter,
        id === undefined ? routingKey : `${routingKey}:${id as number}`
      );
      for await (const [value] of asyncIterator) {
        yield value as any;
      }
    },
  };
};

This is pretty cool as it allows typing the event payloads.

const pubSub = createChannelPubSub<{
  eventName: { payload: string }
}>()

pubSub.publish("eventName", {payload: "ayy" })

@Cellule
Copy link

Cellule commented Nov 23, 2021

Thanks for tip!
We've already greatly improved typings of our subscriptions thanks to graphql-code-generator (I couldn't work with graphql without you guys ♥)
The payload is one the last portion using manual typings so I'll definitely look into improving this in the future

@n1ru4l
Copy link
Collaborator Author

n1ru4l commented Apr 13, 2022

I would not recommend using the code example in #7015 (comment)

It does not do proper cleanup. I recommend using https://repeater.js.org/ for building this instead.

For the new version of graphql-yoga we actually decided to build a typed pubsub and helpers based on repeater.js.

I will keep this link here for people interested in this: https://github.com/dotansimha/graphql-yoga/blob/3740c1248029527f6dc1fc552c20746420c27478/packages/subscription/src/index.ts#L1-L6

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

Successfully merging this pull request may close these issues.

5 participants