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 RFC: Should the "event stream" be derived only from the root field? #282

Closed
robzhu opened this issue Mar 6, 2017 · 17 comments

Comments

@robzhu
Copy link
Contributor

robzhu commented Mar 6, 2017

The "event stream" should be derived purely from the subscription root field (and ignore selection set).
@dschafer @leebyron @rmosolgo @taion @OlegIlyenko @stubailo

Continuing the conversation from: #267 (comment)

@stubailo
Copy link
Contributor

stubailo commented Mar 6, 2017

Depending on the resulution of #284 (subscriptions vs. live queries), I think only allowing the event stream to be derived from the root field parameters would be an important way to make the distinction.

Knowing that you are going to get the same set of events regardless of the query passed in allows you to have more understanding of the system you're calling into, and means you can refactor your client code at will, for example by moving some of the fields into a follow-up query.

Most arguments for including information about the whole query/selection set I've seen so far involve building a live-query like system on top of subscriptions - people are suggesting that you might want to receive updates based on the fields you asked for, which seems to break my understanding of the fundamentals of the system.

@taion
Copy link

taion commented Mar 6, 2017

IMO the nicest way to write the spec has things just work out this way. I take back what I said about live queries.

@rmosolgo
Copy link

rmosolgo commented Mar 6, 2017

A worthwhile detail from dshafer's comment:

root subscription field (and its arguments)

(emphasis mine) In light of that, these two subscriptions may receive different events:

# Pushed only when a new comment is left on Post #4
subscription {
  commentCreated(postId: 4) { 
    # ...
  }
}
# Pushed only when a new comment is left on Post #5
subscription {
  commentCreated(postId: 5) {
    # ...
  }
}

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented Mar 7, 2017

I this is quite an import aspect, as dshafer pointed in in his comment.

Given this schema:

type Subscription {
  a: A
  b: B!
  c(d: D): Int!
}

and following query

  subscription {
    a {a1, a2}
    b {b1, b2}
    c(d: {foo: BAR})
  }

Here are several questions I was struggling with:

  1. Do a, b and c relate to each other in some ways or represent something special just like mutation fields represent mutations/effects thus must be executed sequentially?

  2. Should c allowed to be a scalar type?

  3. Are there other restrictions on an output type of a root field?

    1. Can it be not-null. What is semantic difference between nullable and not-null fields?

    2. Can it be an abstract type (union or interface)? Previously I experimented with 2 ways of representing events in subscriptions and both work quite well:

      as hierarchies

      interface A {
        eventId: ID!
      }
      
      type UserCreated implements A {
        eventId: ID!
        userId: ID!
        userName: String
        email: String 
      }
      type UserNameChanged implements A {
        eventId: ID!
        userId: ID
        userName: String                 
      }

      and a concrete types (every root field represents individual event)

      type A {
        eventId: ID!
        userId: ID!
        userName: String
        email: String
      }
      
      type B {
        eventId: ID!
        userId: ID
        userName: String                 
      }

      should both approaches be allowed. Should we prefer one approach over the other

  4. Does execution of types A, B and C follow normal parallel query execution rules (should a1 and a2 be allowed to execute in parallel)?

  5. If I have 1 incoming event of type "A", what is the right result format?

    1. all fields are always present

      {
        "data": {
          "a": {"a1": "foo", "a2": "bar"},
          "b": null,
          "c": // ???
        }
      } 
    2. only relevant field is returned

      {
        "data": {
          "a": {"a1": "foo", "a2": "bar"}
        }
      } 
  6. If I have 2 incoming events or pieces of information of type A and B, what is the right result format?

    1. all fields are emitted as a single result

      {
        "data": {
          "a": {"a1": "foo", "a2": "bar"},
          "b": {"b1": "baz", "b2": null}
        }
      } 
    2. every field are emitted as a single result

      {
        "data": {
          "a": {"a1": "foo", "a2": "bar"}
        }
      }
      
      // ...
      
      {
        "data": {
          "b": {"b1": "baz", "b2": null}
        }
      } 
  7. Should it be allowed to have 2 root fields simultaneously in a single emitted result?

  8. Can emitted result contain null values in root fields? e.g.

     {
       "data": {
         "a": null
       }
     } 

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented Mar 7, 2017

My personal take on it :)

  1. a, b and c conceptually represents an event/message/fact. It can as well represent a chunk of information that is delivered in several emitted results

  2. yes, scalar types should be allowed

  3. I would allow abstract types, but I would disallow not-null types. It is not yet clear for me how it should mechanically work. I think both approaches from 3.ii are valid without any particular preference.

  4. yes, all execution below root field should inherit normal query execution semantics

  5. I think 5.ii makes the most sense. Though, if not-null result types are not allowed then both variations would be valid (but I still prefer 5.ii)

  6. I think 6.ii makes the most sense.

  7. I would not allow this

  8. I would allow it only in case of an error. E.g.

     {
       "data": {
         "a": null
       },
       "errors": [
         {"message": "something went terribly wrong", "path": ["a"]}
       ]
     } 

What do you think?

@jamesgorman2
Copy link

Might be useful to generate some marble diagrams for some of these (or in the short term repurpose existing ones).

Re 4/6 the choice can be rendered as the distinction between merge and combineLatest.

Is subscription muxing and demuxing (cf queries in relay and apollo) important at this layer? I imagine it should be an overlay to the base subscription mechanism.

@calebmer
Copy link

calebmer commented Mar 7, 2017

@OlegIlyenko I differ on some of these points. Mainly the null/not null stuff.

  1. No. They represent different event types a user wants to receive.
  2. Yes. Nullable values (scalar or object) should also be allowed which gets brought up in 8 😊
  3. Abstract and not-null types should be allowed. I don’t think there should be any restriction on what gets returned. What a not-null type means is that whenever an event is received for a given root field there must also be a value. The only concession the spec may want to make is that a null is allowed for a not-null subscription field type only if there was an error. This differs from queries where if a not-null root field is null the null propagates up to the root level. In the subscription case nulls should only propagate to root level subscription fields.
  4. Yes.
  5. This is an implementation detail that shouldn’t need to be specified. I do prefer 5.ii as well for what it’s worth 😊
  6. This is also an implementation detail which may differ between servers. I think 6.i is fine and clients should probably support that format. Servers may then choose between which format it actually wants to send.
  7. I think this is an implementation detail? The spec should, perhaps, only care about the fact that each root field has an event stream. The timing of event emits is determined by the server. My position on how this should be implemented is reflected in by answer to 6.
  8. Yes. Saying no to null values is not a restriction I think we need. Although I am curious, your position in 3 is that we should not allow root non-null types, but your position in 8 is that we should not allow root nulls.

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented Mar 10, 2017

No. They represent different event types a user wants to receive.

So I guess it's actually "yes" since they share a common property: they represent event types :) Do I understand it correctly? The question was more about semantic similarities rather than a relation between values during the runtime.

Although I am curious, your position in 3 is that we should not allow root non-null types, but your position in 8 is that we should not allow root nulls.

a very good point! I guess I got a bit confused along the way :) I added 8 in an edit mostly because I thought that {"data": {"a": null}} does not hold much of a useful information.

Regarding nullable vs not-null. I don't hold a strong opinion on this one per se. I just wonder how it will interact with the fact that we now have 2 sources of field omission/null value:

  • event stream emitted null for particular event
  • other fields (assuming that they represent different event types) have not emitted any value

I think I'm also ok with defining it like this:

  • both nullable and not-null fields are allowed at the root level
  • undefined means that event is not emitted
  • null means that event type is emitted but an event value is null

Though it would look a bit strange when a is emitted, but c (which is defined as not-null) has undefined/null value.

6/7. I think this is an implementation detail?

I'm not sure about this one. I feel that this aspect has deeper implications on subscription semantics.

@stubailo
Copy link
Contributor

I think this conversation about multiple root fields and null should happen on a separate issue IMO

@calebmer
Copy link

@OlegIlyenko

  1. No. They represent different event types a user wants to receive.

So I guess it's actually "yes" since they share a common property: they represent event types :) Do I understand it correctly? The question was more about semantic similarities rather than a relation between values during the runtime.

The “no” was mostly directed at the serial execution bit at the end of point 1 😊

  • both nullable and not-null fields are allowed at the root level
  • undefined means that event is not emitted
  • null means that event type is emitted by the event value is null

I like this as well 👍

6/7. I think this is an implementation detail?

I'm not sure about this one. I feel that this aspect has deeper implications on subscription semantics.

Yeah, you’re right. I saw JSON and immediately jumped to the conclusion that it is a serialization concern 😊

I think it’s limiting to think about subscription responses purely in the context of the current response format. Let’s see if we can think of something else that better fits the subscription case.

I’m not sure that I like 6.ii when data continues to be a map that conforms to the shape of the selection set. The restriction to a single key/value pair in the data map feels arbitrary. What if instead the event response was:

{
  "event": "a",
  "data": { "a1": "foo", "a2": "bar" }
}
{
  "event": "b",
  "data": { "b1": "baz", "b2": null }
} 

This enforces only one event per message.

@calebmer
Copy link

calebmer commented Mar 11, 2017

Another thought is that the subscriptions grammar should be customized to only allow fields and not other selections like inline fragments or fragment spreads. For example:

SubscriptionSelectionSet :
  { Field[list] }

@OlegIlyenko
Copy link
Contributor

@caabernathy

The “no” was mostly directed at the serial execution bit at the end of point 1 😊

Ah, I see. Sorry, I was more focused on meaning of the field rather them execution characteristics :)

{"event": "..."} approach sounds quite interesting! I need to think about it.

Another thought is that the subscriptions grammar should be customized to only allow fields and not other selections like inline fragments or fragment spreads.

I haven't thought about it. It's definitely something worth considering. What is your reasoning for disallowing inline fragments and fragment spreads?

@stubailo Indeed, I agree. It looks like these two points are relatively independent and can be discussed separately.

@calebmer
Copy link

What is your reasoning for disallowing inline fragments and fragment spreads?

It would enforce a few properties about subscriptions:

  1. The subscription event types will always be present in the subscription definition. Any tooling may then easily know what to expect without resolving fragments.
  2. The user could not create a fragment with any special behavior like they may now for mutations. A fragment on the mutation root type may sometimes execute serially (if spread on a mutation), or in parallel in the rare case that a user recursively references the mutation type somewhere in their tree.
  3. The main use for inline fragments is type checking. On a root mutation or subscription type this is unnecessary most of the time. Unless the root mutation or subscription implements an interface, but is there a convincing reason to do this?

It may even be worth extending this argument for mutations as well.

A more thorough approach then only allowing Fields in a subscription’s SelectionSet would be to only allow fields on root mutations and subscriptions instead of object types. This provides more guarantees to the ecosystem and provides more flexibility to specification authors instead of retrofitting a current concept.


What do you think? I haven’t thought much about this idea either so I’m developing it on the fly 😊

@stubailo
Copy link
Contributor

That's a good point as well that it might be worth specifying that no field can return the Subscription type.

@OlegIlyenko
Copy link
Contributor

@calebmer Thanks a lot for detailed explanation! And I'm sorry for the late reply.

I definitely don't want to discard this idea, but I have some concerns:

  • Introducing a special rules, or exception from general rule has it's own price. I think one of the nice things about GraphQL is that it does have many rules to begin with. The only exception from a general rule i could find is an execution order of mutation fields. In this case I think it's kind of essential, so it definitely worth of inclusion. I guess something similar would be introduced for subscriptions as well. Disallowing fragments in subscription queries can add complexity in the language and and make it less uniform (which may be a disadvantageous for people who are just started to learn GraphQL or tools that relay on specific uniform structure and features), so I wonder whether gained benefits would be worth it.

  • Fragments also serve as a unit of decomposition. Event though query and mutation types are also object types (so there is no polymorphism involved), still libraries like relay take advantage of them to compose queries provided by different components. E.g.:

    query Foo {
      ...ComponentA
      ...ComponentB
    }

    I wonder whether this property of fragments can be beneficial for subscriptions as well. I feel that it can be quite beneficial.

That's a good point as well that it might be worth specifying that no field can return the Subscription type.

@stubailo @calebmer Indeed, It's good point. I feel that may be even necessary for subscription type since it's semantics is quite different from anything else. Maybe it would be an option to introduce it for mutations as well, though I wonder whether it can be just a linting/optional validation rule in case of mutations. I wonder what are the use-cases for mutation/query/subscription type as a return type of a field

@robzhu robzhu added the RFC label Mar 29, 2017
@calebmer
Copy link

calebmer commented Apr 1, 2017

@OlegIlyenko I don’t feel super strong on the mutation and subscriptions are not root objects point, and it kind of derails from the original discussion. We may want to separate into another own issue if the idea is worth exploring. At a high level I do agree though that the simplicity of the language is incredibly important, but it may actually be simpler to add special semantics for root mutations and subscriptions instead of adding special cases to object types which people may want to extend to other object types that are not root mutations or subscriptions (see #252). Instead of changing GraphQL syntax we should just be able to do this with a validation rule.

As for the original question, I don’t think there is any disagreement that the specification should require subscription events to be exclusively derived from the root field. Correct @robzhu?

@robzhu
Copy link
Contributor Author

robzhu commented May 2, 2017

Updated the RFC text to describe this more clearly: #304

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

7 participants