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

Subscriptions: Final edits #392

Merged
merged 2 commits into from
Dec 21, 2017
Merged

Subscriptions: Final edits #392

merged 2 commits into from
Dec 21, 2017

Conversation

leebyron
Copy link
Collaborator

This does a last pass over the subscriptions specification to clean up some ambiguities and crisp up some language.

Two important things to take note of:

  1. The CreateSourceEventStream algo was underspecified and did not articulate how fields should be collected from the selection set. This makes it clear that CollectFields is used, just like any other selection set. It also did not describe behavior when executing an invalid query, this adds that a query-level error should be thrown.

  2. The Subscription Operation validation is similarly underspecified, and does not describe what should happen if a fragment spread is found as the first selection in a selection set. This clarifies that it is fields that exactly one is expected, rather than selections.

This does a last pass over the subscriptions specification to clean up some ambiguities and crisp up some language.

Two important things to take note of:

1. The `CreateSourceEventStream` algo was underspecified and did not articulate how fields should be collected from the selection set. This makes it clear that `CollectFields` is used, just like any other selection set. It also did not describe behavior when executing an invalid query, this adds that a query-level error should be thrown.

2. The Subscription Operation validation is similarly underspecified, and does not describe what should happen if a fragment spread is found as the first selection in a selection set. This clarifies that it is *fields* that exactly one is expected, rather than selections.
Copy link
Contributor

@robzhu robzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! One optional suggestion inline.

@@ -248,6 +252,20 @@ subscription sub {
}
```

```graphql counter-example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth documenting the expected behavior of sending a single document with multiple subscription operations and an operation name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - I can add that

@acjay
Copy link

acjay commented Dec 21, 2017

Wait, sorry -- subscription can only take one root field per request document?

@robzhu
Copy link
Contributor

robzhu commented Dec 21, 2017

Wait, sorry -- subscription can only take one root field per request document?

That's correct. Is that a limitation that will cause problems for you?

@leebyron
Copy link
Collaborator Author

Correct - this limitation solves a whole class of ambiguities regarding merging event streams. We've found in practice it's almost always preferable to have multiple open subscriptions rather than a single subscription that attempts to merge streams

@acjay
Copy link

acjay commented Dec 21, 2017

@robzhu & @leebyron I see. I haven't yet started implementing subscriptions. We're looking to potentially migrate to them to implement what's essentially a live query (hence my discussions on other tickets) from an existing system that uses an ad hoc Web Socket protocol. But since this is still in the early stages, I can't say for sure whether this is an issue. It's probably fine. It seems like the issue could be skirted by having a field represent a whole bunch of different events and return a union of event payloads.

I was just surprised, since Sangria's pre-spec subscription implementation doesn't have this restriction, so just wanted to make sure I understood. It seems like they solve the problem by largely treating the document as independent subscriptions, and the event stream simply only contain one of the root fields per payload.

@leebyron leebyron merged commit 3cee7aa into master Dec 21, 2017
@leebyron leebyron deleted the subscription-edits branch December 21, 2017 22:24
@leebyron
Copy link
Collaborator Author

Yeah - we could lift this restriction in the future should crisp semantics become more clear and the value outweigh the cost of the potential for confusion. At Facebook we faced some serious roadblocks to supporting multiple subscription root fields for this and other reasons - so felt it was best to specify a more limited functionality set out of the gates

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

Successfully merging this pull request may close these issues.

None yet

4 participants