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

Please make subscription value structure consistent with query value structure #1433

Closed
jedwards1211 opened this issue Jul 26, 2018 · 14 comments

Comments

@jedwards1211
Copy link

jedwards1211 commented Jul 26, 2018

So imagine the following basic schema:

type Query {
  foo(tag: String): MetadataItem
}

type Subscription {
  foo(tag: String): MetadataItem
}

My resolver for Query.foo can return a Promise<MetadataItem>.
You would think my subscribe resolver could return an AsyncIterable<MetadataItem>.
But noooo...
For whatever reason it has to return an AsyncIterable<{foo: MetadataItem}>.
Why do I have to wrap the values I yield in an object?
Is there a good reason for this or is it just a mistake?

This is really annoying and it always trips me up. Why were subscriptions designed this way instead of being more consistent with queries? I feel very angry about how often I have had to debug issues stemming from this, because it's just not an intuitive inconsistency. It's very easy to forget about.

GraphQL could so easily wrap the values I yield in an object with the field name, the only reason I can imagine that it doesn't is some performance nitpick.

@jedwards1211 jedwards1211 changed the title Please make subscription data structure consistent with query data structure Please make subscription value structure consistent with query value structure Jul 26, 2018
@IvanGoncharov
Copy link
Member

@jedwards1211 It's actually very consistent with query if you have the right mental model. You yield root value for the entire query (not just for foo field) and then it executing exactly the same as a query:
http://facebook.github.io/graphql/June2018/#ExecuteSubscriptionEvent()

Actually, it's so consistent with a query that if you register resolve on Subscription.foo it would be called with the value you yielded from your stream.

@jedwards1211
Copy link
Author

Oh. Is this documented anywhere?

@jedwards1211
Copy link
Author

jedwards1211 commented Jul 26, 2018

I thought the resolution was just happening on the types of objects I yield, not the root query.

Is there an example of what would actually be the point of having a resolve next to subscribe on foo?

@IvanGoncharov
Copy link
Member

Oh. Is this documented anywhere?

It's documented in GraphQL specification:
http://facebook.github.io/graphql/June2018/#ExecuteSubscriptionEvent()
Currently, we have big problems with the documentation and if you want to help you can volunteer in this issue:
#1368

Is there an example of what would actually be the point of having a resolve next to subscribe on foo?

From the top of my head:

  1. Events are yield by code that can't/shouldn't be coupled with GraphQL and you want a single resolve function to do the transformation.
  2. Same code yields events for multiple different subscription fields and you put the field-specific code into resolve.

@jedwards1211
Copy link
Author

jedwards1211 commented Jul 26, 2018

Okay, great! So to be clear what you really mean by

You yield root value for the entire query

Is really:

You yield root value for the entire subscription

i.e., the value you yield is resolved against the Subscription type in the schema, not against the Query type in the schema, right?

@IvanGoncharov
Copy link
Member

i.e., the value you yield is resolved against the Subscription type in the schema, not against the Query type in the schema, right?

Yes exactly.

@IvanGoncharov
Copy link
Member

query can mean three things:

  1. query document - all operations + fragments
  2. query operation
  3. query root type

You yield root value for the entire query

I meant query document here

@jedwards1211
Copy link
Author

Okay, makes sense. Thanks!

@jedwards1211
Copy link
Author

You know, now that I think about this, I kind of wish I could yield a root value for the Query type, because then the client would be able to automatically determine which nodes to update in the cache in some cases.

@IvanGoncharov
Copy link
Member

@jedwards1211 Not sure it's a good idea but nothing prevents you from doing:

type Query {
  foo(tag: String): MetadataItem
}

type Subscription {
  foo(tag: String): Query
}

@IvanGoncharov
Copy link
Member

@jedwards1211 Not sure about Apollo but in Relay caching is based on object ID so it should be updated automatically without requiring matching path in a query.

@jedwards1211
Copy link
Author

Oh I see, good point.

@jedwards1211
Copy link
Author

jedwards1211 commented Jan 21, 2020

@IvanGoncharov another thing that is JS-specific and not well documented -- what's the intended behavior when subscribe returns Promise<AsyncIterable> instead of returning the AsyncIterable? Most of my subs still seem to work when I changed to returning Promise<AsyncIterable>, but I noticed one of my resolvers is getting called with a Promise instead of a plain object yielded by the iterator...

Nevermind, I misunderstood my own code 😅

@jedwards1211
Copy link
Author

jedwards1211 commented Mar 25, 2020

@IvanGoncharov I still find this terribly confusing.

Although this doesn't seem to the behavior described in the spec, from what I'm observing with running code, it appears that:

  • If subscribe has a sibling resolve function, then subscribe can yield just the value of its field without any wrapper object?
  • Otherwise subscribe has to yield an object corresponding to the shape of the Subscription type

I am using Apollo's buildExecutableSchema so maybe the confusion's coming from there...

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
@jedwards1211 @IvanGoncharov and others