subscribe: once execution starts....

[yaacovCR]

data should return null?

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	40adebf
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62a64cfbe351060009f58b72
😎 Deploy Preview	https://deploy-preview-3640--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[github-actions]

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[yaacovCR]

See some discussion in graphql/graphql-spec#894

See additional rough thoughts on how subscriptions fit in at: #3621 (comment)

[yaacovCR]

Probably should bring this up at next WG--

Basic question- how should request errors be distinguished from CreateSourceEventStream errors? Setting data to null is one option, but it's a little strange as data is never non null because when successful, you get a stream rather than a single result.

Previously, we just threw, but now that at are trying to align execute and subscribe, we return an errors result, so we have to decide when, if ever, to set data to null.

@benjie tagging you in terms of if you have any thoughts related to your spec PR about clarifying request vs execution errors

[benjie]

The transition between "request error" (payloads like {errors: GraphQLError[]}) and "field error" (payloads like {data: object | null, errors?: GraphQLError[]}) in the spec doesn't seem to happen at explicitly designated boundaries, but here's my take:

If a "request error" is raised, then that's seen as being "before execution" (even if it's part of ExecuteRequest(), for example the "request error" raised by CoerceVariableValues().

Asserts don't define an error type because they should not happen (due to validation, etc); but I see the assert in ExecuteQuery() and ExecuteMutation() as raising a "request error" (since there is no field at that point). Once these algorithms have moved onto the ExecuteSelectionSet phase we're out of "request error" phase and into "field error" phase.

For subscriptions the definition is somewhat harder to discern. Do we introduce the "boundary" before or after step 10 of CreateSourceEventStream()? What happens if the call to ResolveFieldEventStream results in an error?

One option is that because this is an error in a field method, it is a field error, and thus the division between "request error phase" and "field error phase" is between steps 9 and 10 of CreateSourceEventStream:

9. Let argumentValues be the result of CoerceArgumentValues(subscriptionType, field, variableValues)
   ~~BOUNDARY~~
10. Let fieldStream be the result of running ResolveFieldEventStream(subscriptionType, initialValue, fieldName, argumentValues).

So an error raised whilst creating the stream for a field should be treated as a field error, and thus should result in the field itself being null. However, this may be surprising to the user who is expecting a stream not a singular payload.

It seems to me that if you fail to create the event stream then that should be a request error (you've failed to set up a subscription, so the operation has failed). I.e. rather than putting the boundary at the aforementioned position, I think it should probably be once you enter MapSourceToResponseEvent(). But honestly, I could go either way on this. There does seem to be ambiguity here, but I'd have to read the spec more fully again to figure out if this was defined or not.

[yaacovCR]

It seems to me that if you fail to create the event stream then that should be a request error (you've failed to set up a subscription, so the operation has failed). I.e. rather than putting the boundary at the aforementioned position, I think it should probably be once you enter MapSourceToResponseEvent(). But honestly, I could go either way on this.

I guess any type of error for CreateSourceEventStream leads to failed input for MapSourceToResponseEvent which is a kind of request error…

I think this needs more hardening in the spec and likely a better way to distinguish between error types than the existence of data:null which doesn’t make much sense for subscriptions.

i am going to close this for now, but it’s here if we want to revisit

[benjie]

In summary, my current opinion is that "execution begins"

• for queries: when you enter ExecuteQuery()
• for mutations: when you enter ExecuteMutation()
• for subscriptions: when you enter MapSourceToResponseEvent()

[yaacovCR]

That makes data:null match current behavior, but I think a spec improvement would make all this explicit and also explicitly acknowledge that with subscriptions, there are two rounds of execution, one that sets up the source stream and one for each event....

[benjie]

Yeah, I agree that clarity here should come from the spec.