RFC: __typename is not valid at subscription root

[benjie]

I'm writing up some of the GraphQL "query ambiguity" work currently and noticed this issue in the spec. In the case of a subscription it seems that

subscription {
  __typename
}

does not correctly get evaluated by the reference implementation during subscriptions (though you can query it with graphql(...) you can't access it via subscribe(...)). It doesn't really make sense to request __typename here since it's not ever going to change during the life of the schema, and subscriptions only support one root-level field, however I thought it was worth highlighting.

Reproduction with GraphQL-js (toggle which doc is commented to see a functioning subscription):

const {
  GraphQLSchema,
  GraphQLObjectType,
  GraphQLInt,
  GraphQLFloat,
  parse,
  validate,
  subscribe,
} = require("graphql");

const sleep = ms => new Promise(resolve => setTimeout(resolve, ms));

async function* everySecond() {
  for (let i = 0; i < 1000; i++) {
    yield i;
    await sleep(1000);
  }
}

const Query = new GraphQLObjectType({
  name: "Query",
  fields: {
    a: {
      type: GraphQLInt,
    },
  },
});

const Subscription = new GraphQLObjectType({
  name: "Subscription",
  fields: {
    ts: {
      type: GraphQLFloat,
      subscribe() {
        return everySecond();
      },
      resolve() {
        return Date.now();
      },
    },
  },
});

const schema = new GraphQLSchema({
  query: Query,
  subscription: Subscription,
});

async function main() {
  const doc = parse("subscription { __typename }");
  // const doc = parse("subscription { ts }");
  const errors = validate(schema, doc);
  if (errors && errors.length) {
    console.dir(errors);
    throw new Error("Errors occurred");
  }
  const result = await subscribe(schema, doc);
  console.dir(result);
  for await (const r of result) {
    console.dir(r);
  }
}

main().catch(e => {
  console.dir(e);
  process.exit(1);
});

Produces this error:

Error: Subscription field must return Async Iterable. Received: undefined
    at [...]/node_modules/graphql/subscription/subscribe.js:169:13
    at async main ([...]/test.js:57:18)

[leebyron]

Exception doesn't need to be as prominent since this sentence first introduces the idea of __typename. Maybe a Note: at the bottom of the section you're editing

Validation rule should change to "one non introspection field"

Example that needs to change https://spec.graphql.org/draft/#example-2353b

[benjie]

GraphQL.js PR here: graphql/graphql-js#2861

[yaacovCR]

It would be helpful I think if this would be valid to allow for a completely typed result to be passed to result transformers. Otherwise result transformers have to be passed additional state in terms of which operation type was sent and treat the data field differently than inner fields during processing, at least for subscriptions.

[yaacovCR]

Of course, you can save the request document and add __typename to all abstract fields and then combine request and result to know types of everything, including subscription root. That is another way. But seems like this way should also be allowed to work in terms of symmetry of the root types.

[yaacovCR]

Of course, then you need to repeat the same field collection algorithm that happened during field resolution on the server when processing the result, so we currently do that in graphql-tools only when we need to know the types of scalar fields. Perhaps a generic solution would be to annotate fields using aliases that would include the typename, that is yet another option, just would be easier I think if all of these options worked...

[IvanGoncharov]

@benjie I still think we need to allow adding __typename (and other introspection fields) and changes rules to require "single non-introspection field".
Rationalize:

1. That will make the validation rule a little bit more complex but it will save all other libraries from having to write logic that skips __typename. Example Upcoming Release Changes ardatan/graphql-tools#2290
2. It's more consistent from the client's point of view since you can have __typename both on queries and mutations.

[eapache]

FWIW I checked how graphql-ruby handles this, and it Just Works (I suppose in contravention of the spec). If you put a __typename in a subscription request you get that back (value "Subscription") beside your payload in the initial response and in every subsequent update.

[benjie]

@IvanGoncharov I've written up a solution that follows your suggestion (and might be how graphql-ruby works, @eapache?) in #806.

I'm on the fence between these two solutions. In general I'm leaning towards the simplicity of this one ("there can only be one field, and it can't be __typename") but #806's expansion of __typename to all contexts is also attractive, particularly for tooling as @yaacovCR notes.

My main hesitation against #806, beyond complexity, is that it feels like it's going to make the "subscriptions must only have one root-level field" rule muddier ("if we can have a normal field and an introspection field, why can't we have another normal field?").

Interestingly if we merged this PR, we could later merge #806 and it would be a non-breaking change (an expansion of capabilities). Both invalidate the operation subscription { __typename }, so if there's no clear consensus that #806 is superior I feel we should merge this PR to address the immediate issue, and then continue discussions about potentially merging #806 later.

[leebyron]

Looks like some of this language already leaked in via editorial changes in #844 - https://spec.graphql.org/draft/#sel-FAJZDABzCBjF8wX

[benjie]

I think this is fine to merge, it doesn't prevent us changing our mind in future 👍

One caveat: I think graphql-ruby (?) allows __typename in addition to the subscription field, but I think this just means it has "more lenient validation" rather than being specifically non-compliant?

[leebyron]

I know PR is still in review. The implementation there may change but the test suite looks 👌. I think we're good here to go to RFC 3:

• Consensus the solution is complete (via editor or working group)
• Complete spec edits, including examples and prose
• Compliant implementation in GraphQL.js (fully tested and merged or ready to merge)