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

Spec: Formal Subscriptions Definition #305

Merged
merged 22 commits into from May 17, 2017

Conversation

Projects
None yet
@robzhu
Contributor

robzhu commented May 2, 2017

This PR contains my proposal for formally describing GraphQL Subscriptions in the Spec. It will be accompanied by a PR against GraphQL-js (will provide link when that PR is available).

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

Halts the event stream at moment of unsubscribe? If an event stream is in-flight, will the client still receive it?

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

Halts the event stream at moment of unsubscribe? If an event stream is in-flight, will the client still receive it?

This comment has been minimized.

Show comment
Hide comment
@robzhu

robzhu May 2, 2017

Owner

Addressed below.

Owner

robzhu replied May 2, 2017

Addressed below.

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

Is this an ID uniquely identifying the client?

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

Is this an ID uniquely identifying the client?

This comment has been minimized.

Show comment
Hide comment
@robzhu

robzhu May 2, 2017

Owner

It refers to the pair of subscriber and subscription. I'll update the wording to clarify.

Owner

robzhu replied May 2, 2017

It refers to the pair of subscriber and subscription. I'll update the wording to clarify.

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

initial publish

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

initial publish

This comment has been minimized.

Show comment
Hide comment
@laneyk

laneyk May 3, 2017

hmm, this opens a can of worms since this is generally not possible.

laneyk replied May 3, 2017

hmm, this opens a can of worms since this is generally not possible.

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

"normally" doesn't feel like the right word, though I understand from context that it means to allow parallelization

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

"normally" doesn't feel like the right word, though I understand from context that it means to allow parallelization

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

a list of *field errors*? So in the next line {errors} is not empty makes more sense.

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

a list of *field errors*? So in the next line {errors} is not empty makes more sense.

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

s/optionally/you *may*/ ?

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

s/optionally/you *may*/ ?

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

Weird caps Mixing

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

Weird caps Mixing

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

nit: dedicate missing a d

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

nit: dedicate missing a d

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

maybe mention the GraphQL server means for GraphQL queries and mutations, and mention that the state persistence is for Subscriptions

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

maybe mention the GraphQL server means for GraphQL queries and mutations, and mention that the state persistence is for Subscriptions

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

s/Subscribe step/Subscribe operation/

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

s/Subscribe step/Subscribe operation/

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

You don't define what initialValue is

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

You don't define what initialValue is

This comment has been minimized.

Show comment
Hide comment
@robzhu

robzhu May 2, 2017

Owner

Initial value is defined earlier in the spec. Its use here is consistent with how we use it a few paragraphs above for queries and mutations.

Owner

robzhu replied May 2, 2017

Initial value is defined earlier in the spec. Its use here is consistent with how we use it a few paragraphs above for queries and mutations.

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

Shouldn't unsubscribe take in a subscription?

tcr commented on spec/Section 6 -- Execution.md in c4aeaf9 May 2, 2017

Shouldn't unsubscribe take in a subscription?

This comment has been minimized.

Show comment
Hide comment
@robzhu

robzhu May 2, 2017

Owner

Added some more text to clarify.

Owner

robzhu replied May 2, 2017

Added some more text to clarify.

@tcr

This comment has been minimized.

Show comment
Hide comment
@tcr

tcr May 2, 2017

s/set/list/ possibly

tcr commented on spec/Section 5 -- Validation.md in 4bba743 May 2, 2017

s/set/list/ possibly

This comment has been minimized.

Show comment
Hide comment
@robzhu

robzhu May 2, 2017

Owner

"set of one" is consistent with the wording used in the rest of this doc.

Owner

robzhu replied May 2, 2017

"set of one" is consistent with the wording used in the rest of this doc.

@@ -143,6 +143,84 @@ ExecuteMutation(mutation, schema, variableValues, initialValue):
selection set.
* Return an unordered map containing {data} and {errors}.

This comment has been minimized.

@wincent

wincent May 2, 2017

Contributor

Putting a comment here because silly GitHub won't let me comment on the lines above. Up on line 106-107 it says:

If mutations are supported, it must also provide a mutation root object type.

That should probably say:

If mutations or subscriptions are supported, it must also provide a mutation and subscription root object type, respectively.

Or words to that effect.

@wincent

wincent May 2, 2017

Contributor

Putting a comment here because silly GitHub won't let me comment on the lines above. Up on line 106-107 it says:

If mutations are supported, it must also provide a mutation root object type.

That should probably say:

If mutations or subscriptions are supported, it must also provide a mutation and subscription root object type, respectively.

Or words to that effect.

This comment has been minimized.

@leebyron

leebyron May 14, 2017

Collaborator

Also noticed that a step should be added to {ExecuteRequest} above to handle subscription requests.

@leebyron

leebyron May 14, 2017

Collaborator

Also noticed that a step should be added to {ExecuteRequest} above to handle subscription requests.

Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
@robzhu

Reworked this section.

Show outdated Hide outdated spec/Section 6 -- Execution.md
@leebyron

This is looking awesome. More fine-grained feedback this time

Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
@robzhu

Thanks for the thorough review! I think I addressed all of your comments.

Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
@leebyron

Getting excited! Most feedback here is within the algorithms

Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
@robzhu

This comment has been minimized.

Show comment
Hide comment
@robzhu

robzhu May 13, 2017

Contributor

I took another pass and made changes per your suggestions. Reading everything top-to-bottom, it feels a lot more dense/verbose. We also have some duplication, for example with ExecuteSubscriptionEvent. Do you feel this restructuring buys us consistency/clarity?

Contributor

robzhu commented May 13, 2017

I took another pass and made changes per your suggestions. Reading everything top-to-bottom, it feels a lot more dense/verbose. We also have some duplication, for example with ExecuteSubscriptionEvent. Do you feel this restructuring buys us consistency/clarity?

@@ -143,6 +143,84 @@ ExecuteMutation(mutation, schema, variableValues, initialValue):
selection set.
* Return an unordered map containing {data} and {errors}.

This comment has been minimized.

@leebyron

leebyron May 14, 2017

Collaborator

Also noticed that a step should be added to {ExecuteRequest} above to handle subscription requests.

@leebyron

leebyron May 14, 2017

Collaborator

Also noticed that a step should be added to {ExecuteRequest} above to handle subscription requests.

Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
*normally* (allowing parallelization).
* Let {errors} be any *field errors* produced while executing the
selection set.
* Return an unordered map containing {data} and {errors}.

This comment has been minimized.

@leebyron

leebyron May 14, 2017

Collaborator

Similarly, it would be nice to have a Note: which mentions the intentional similarity to {ExecuteQuery}

@leebyron

leebyron May 14, 2017

Collaborator

Similarly, it would be nice to have a Note: which mentions the intentional similarity to {ExecuteQuery}

Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron May 14, 2017

Collaborator

We also have some duplication, for example with ExecuteSubscriptionEvent. Do you feel this restructuring buys us consistency/clarity?

Yes, I think that it helps to illustrate the seam between creating a subscription and executing each event, which will help those building this across two services.

Collaborator

leebyron commented May 14, 2017

We also have some duplication, for example with ExecuteSubscriptionEvent. Do you feel this restructuring buys us consistency/clarity?

Yes, I think that it helps to illustrate the seam between creating a subscription and executing each event, which will help those building this across two services.

Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
Show outdated Hide outdated spec/Section 6 -- Execution.md
@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron May 17, 2017

Collaborator

This looks awesome. Imminently mergeable.

Collaborator

leebyron commented May 17, 2017

This looks awesome. Imminently mergeable.

@leebyron leebyron merged commit d7e3f06 into facebook:master May 17, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron May 17, 2017

Collaborator

This turned out great.

Collaborator

leebyron commented May 17, 2017

This turned out great.

@josephsavona

This comment has been minimized.

Show comment
Hide comment
@josephsavona

josephsavona May 17, 2017

Contributor

Nice job with this, @robzhu!!!

Contributor

josephsavona commented May 17, 2017

Nice job with this, @robzhu!!!

IvanGoncharov added a commit to APIs-guru/graphql that referenced this pull request Jun 17, 2017

Spec: Formal Subscriptions Definition (#305)
* Subscription validation rule: single root field

* typo in explanatory text

* Updated wording to match Execution section

* Execution section for subscriptions

* Remove leftover section on Subscription functions

* Incorporate feedback from first review

* Line feed after subscription sections

* Fix spec syntax

* Address wincent's feedback

* Modify wording from Laney's feedback

* Address Sashko's feedback

* Fix inline code snippets

* Address Lee's feedback

* Address second round of feedback w/ Lee

* Address third round of feedback from Lee

* Add subscriptions to other sections

* Address more feedback on Execution section

* Remove extra single root field check and reword subscriptions description

* and/or

* Minor editing updates

* Mini-header for event streams
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment