grats icon indicating copy to clipboard operation
grats copied to clipboard

Support for subscriptions yielding errors

Open felamaslen opened this issue 11 months ago • 9 comments

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.

felamaslen avatar Jan 21 '25 11:01 felamaslen

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 avatar Jan 21 '25 11:01 louy

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 avatar Jan 21 '25 11:01 louy

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

louy avatar Jan 21 '25 11:01 louy

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

captbaritone avatar Jan 24 '25 02:01 captbaritone

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.

felamaslen avatar Jan 28 '25 16:01 felamaslen

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

captbaritone avatar Jan 28 '25 17:01 captbaritone

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 avatar Jan 31 '25 09:01 felamaslen

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.

felamaslen avatar Jan 31 '25 17:01 felamaslen

Yeah, I expect this to be up to the implementor (graphql-js in our case). So maybe this is a feature request (or at least a documentation request) for graphql-js? Specifically, maybe there’s some way they could enable throwing an error which can be interpreted as just that one payload erroring and does not tear down the entire subscription?

Jordan Eldredge

jordaneldredge.com @captbaritone http://twitter.com/captbaritone

On Fri, Jan 31, 2025 at 9:38 AM Fela Maslen @.***> wrote:

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.tsimport { 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.tsimport { 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.

— Reply to this email directly, view it on GitHub https://github.com/captbaritone/grats/issues/170#issuecomment-2627881846, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABHXL6GNGULTDGXHBVRSJ32NOYKRAVCNFSM6AAAAABVSIXYE6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRXHA4DCOBUGY . You are receiving this because you commented.Message ID: @.***>

captbaritone avatar Jan 31 '25 18:01 captbaritone