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

Support for subscriptions yielding errors #170

Open
felamaslen opened this issue Jan 21, 2025 · 9 comments
Open

Support for subscriptions yielding errors #170

felamaslen opened this issue Jan 21, 2025 · 9 comments

Comments

@felamaslen
Copy link

felamaslen commented Jan 21, 2025

Let's say I want to write a subscription, which can yield a known type, or an error. I would expect to be able to do something like this:

/** @gqlType */
type MyType {
  foo: string;
};

/** @gqlSubscriptionField */
function* myField(_, args: { shouldThrowError: boolean }): AsyncIterable<MyType> {
  if (shouldThrowError) {
    yield new GraphQLFormattedError('Something went wrong', {
      extensions: { formattedMessage: 'A formatted error message' },
    });
  } else {
    yield { foo: 'bar' };
  }
}

This actually works, except it does not pass type check of course.

Adding | GraphQLFormattedError to the AsyncIterable generic type prevents grats from generating the schema.

Is there an expected pattern here? I'm wondering whether I'm missing something from the docs, or if we need to figure out an implementation for this.

@louy
Copy link

louy commented Jan 21, 2025

FYI, worth noting that GraphQL executable schema (IIRC, but could be just yoga) allows returning an instanceof Error from any resolver and translates that into an error returned to the client instead of data.

So this doesn't technically only affect subscriptions but also other resolvers. Would be nice if grats could permit any Error type to be returned and ignores it

@louy
Copy link

louy commented Jan 21, 2025

It is a graphql-tools (i.e. makeExecutableSchema) thing I believe. I can't find the exact source code but here's an example from their docs:
https://github.com/ardatan/graphql-tools/blob/c12a83ead203542317cbdf32cee87f5596ddd6cb/website/src/content/schema-directives.mdx#what-about-directiveresolvers

            return resolver(
              () =>
                new Promise((resolve, reject) => {
                  const result = originalResolver(source, originalArgs, context, info)
                  if (result instanceof Error) {
                    reject(result)
                  }
                  resolve(result)
                }),
              source,
              directiveArgs,
              context,
              info
            )

@louy
Copy link

louy commented Jan 21, 2025

I think I found it. Here it is:
https://github.com/ardatan/graphql-tools/blob/c12a83ead203542317cbdf32cee87f5596ddd6cb/packages/executor/src/execution/execute.ts#L812-L815

Not really well documented afaik

@captbaritone
Copy link
Owner

Interesting. It does sound like a thing that would be nice to be able to do. Am I following correctly that the goal here is to get the iterable itself to not error, but allow an individual item within the iterable to be in an error state?

I think Grats will limit itself to only having built in support for things which work out of the box with graphql-js and it sounds like this is not a feature supported by graphql-js?

I wonder if it's possible in graphql-js to yield a promise that rejects and get the behavior you want?

Related: graphql-js' handling of AsyncIterators: https://github.com/graphql/graphql-js/blob/16.x.x/src/execution/mapAsyncIterator.ts#L7-L57

@felamaslen
Copy link
Author

Yeah I suspect there may be a use case for yielding an error and then carrying on yielding successful results. I wouldn't rule that out at least.

The issue here isn't about what graphql-js is capable of, but the typing of the iterator. I think grats just needs to parse this properly:

/**
 * gqlSubscriptionField
 */
function myField(): AsyncIterable<MyType | GraphQLFormattedError> {
  yield myType;
  yield new GraphQLFormattedError('Something bad happened');
}

The easiest way imho would be to parse the union type declaration in the return type and extract GraphQLFormattedError from it, then assign the rest to the return type.

@captbaritone
Copy link
Owner

The issue here isn't about what graphql-js is capable of, but the typing of the iterator. I think grats just needs to parse this properly:

My hope was that there was some other way to structure this code such that the error is thrown instead of returned while still achieving the same observable behavior from the perspective of the GraphQL consumer. Can we confirm that there is no way to structure this code such that we don't have to return the error and can instead throw it (as we do with all other errors in graphql-js)?

Also, I'm a little unclear from the above discussion: Is the handling of yielded errors something graphql-js does, or a feature of graphql-tools makeExecutableSchema? If the latter, then I don't think Grats can safely ignore the error in the example you gave, since the user may be using graphql-js

@felamaslen
Copy link
Author

felamaslen commented Jan 31, 2025

I can't find much documentation for this, but judging from this test, it seems that calling throw Error(...) is expected to end the subscription.

However, calling yield new Error(...) does not end the subscription, it just passes the error through with the option to then yield more results, eventually completing in a successful state (although I'm not sure if this is a common use case).

I will investigate if the above is handled in graphql-js or elsewhere in the stack

@felamaslen
Copy link
Author

I just did a very basic test using graphql (without yoga or any other libraries), and yield new Error() does not seem to send errors through to the response. As far as I can tell, this seems to be a gap in the GraphQL spec when it comes to subscriptions. Maybe it's left open to interpretation by implementations?

Here is some test code:

// server.ts
import { buildSchema } from 'graphql';
import { useServer } from 'graphql-ws/lib/use/ws';
import { WebSocketServer } from 'ws';

const schema = buildSchema(`
  type Query {
    hello: String
  }
  type Subscription {
    greetings: String
  }
`);

const roots = {
  query: {
    hello: () => 'Hello World!',
  },
  subscription: {
    greetings: async function* greetings() {
      yield new Error('foo');
      yield { greetings: 'Hello!' };
    },
  },
};

const server = new WebSocketServer({
  port: 4000,
  path: '/graphql',
});

useServer({ schema, roots }, server);

console.log('Listening to port 4000');
// sub.ts
import { createClient } from 'graphql-ws';

const client = createClient({
  url: 'ws://localhost:4000/graphql',
});

(async () => {
  const subscription = client.iterate({
    query: 'subscription { greetings }',
  });

  for await (const event of subscription) {
    console.log(JSON.stringify({ event }));
  }
})().catch((err) => {
  console.error('Error', err);
});
npx tsx --watch server.ts
npx tsx sub.ts
{"event":{"data":{"greetings":null}}}
{"event":{"data":{"greetings":"Hello!"}}}

I suspect that this is just not well-defined in the GraphQL spec. I can't find any good documentation on it. Also, calling throw new Error() inside the subscription iterator there actually blows up the whole subscription, it doesn't send across an error to the client.

@captbaritone
Copy link
Owner

captbaritone commented Jan 31, 2025 via email

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

No branches or pull requests

3 participants