2021-12-02.md

GraphQL WG Notes - December 2021

Watch the replay: GraphQL Working Group Meetings on YouTube

Agenda

• Agree to Membership Agreement, Participation Guidelines and Code of Conduct (1m, Lee)
  • Specification Membership Agreement
  • Participation Guidelines
  • Code of Conduct
• Introduction of attendees (5m, Lee)
• Determine volunteers for note taking (1m, Lee)
• Review agenda (2m, Lee)
• Review previous meeting's action items (5m, Lee)
  • Ready for review
  • All open action items
• Update on TSC Elections (1m, Lee)
  • Issue with info
• Shorthand for variables/arguments on operation (Saihaj, 10m)
  • graphql-spec#889
• Client Controlled Nullability status update. (10m, Alex Reilly)
  • Current action item
  • Spec PR
  • GraphQL.js Implementation
  • WIP Integration with The Guild's GraphQL Code Generator
• Defer/Stream updates/issues/discussions. (30m, Rob Richard)
  • Enforcing correct delivery order of payloads
  • @stream directive initialCount validation
  • Stream on scalar lists
  • Allow @defer on root mutation or subscription fields
  • Mutation serial execution with defer & stream
  • Non-nullable field errors in defer/stream payloads
• Discuss GraphQL mutation design(s) with large schemas (nested mutations) (30m, Aleksandar)
  • Details document: GraphQL Mutations and Large Schemas
  • Nested mutations (PowerPoint Show) (DRAFT before the meeting)

Determine volunteers for note taking (1m, Lee)

• Benjie

Review agenda (2m, Lee)

• How do we get started contributing? Do we need to raise a full RFC?
  • RFC0 is "crazy idea" - we can open as many RFC0s as we want
    • explore via code pull request
    • open issue or discussion
    • or add a file to the RFCs folder in GraphQL-WG repo - even RFC0 stuff is fine
  • Whatever you find most useful to get the idea out
  • This helps you to know what you want feedback on during the meeting.
  • Matt: RFCs can be really light at RFC0 stage, we want this to be as lightweight as possible. If it's viewed as heavyweight let us know what we can do to reduce that - we need an ability for people to just throw out ideas and generate discussion.
  • Lee: heavyweightedness of RFC should scale up as the RFC scales up - once you're getting to stage 2/3 we need to be really precise, but at RFC0 they can be very high level.
  • Aleksandar: can we advance them between times?
  • Lee: having a breakout working group can often help, also async, and of course bringing them to the WG - whatever works to move things forward
• Lee: I've filed 2022 agendas into the WG. Currently they're in the 2022 directory, I might move the others around too but worried about breaking links. I've also cleaned up the template (more good stuff, less guidelines - all guidelines now in a comment block at the top so you see it first).
• ACTION - everyone have a quick scan over this, send PR to one file and I'll propagate it to the others.

Review previous meeting's action items (5m, Lee)

• Ready for review -
• All open action items
• We're just going to look at "ready for reviews" today due to a tight schedule.
• #792 - done
• #793 - done
• #795 - not sure if the READMEs are done or not, might be a long tail of finding dead links and fixing them as we go.
  • Overview of what we did: we used to have the working files in the -spec repo, this -wg repo was only for agendas and docs. This was an issue for the last round of the spec cut because we wanted to see who signed what when; there was a practical cost to people's ability to merge stuff - merging to spec repo is quite high, so few have commit rights there - but the wg repo is broader so we should be able to give commit rights to more people. When RFC docs are raised - merge them (no need to review at the PR level).
• Benjie: related - can we just merge typo fixes into the spec now that Oct21 is released?
• Lee: yes, simple editorial (typo fixes) can be merged to the spec.
• #765 - nominations -> elections, removing ready for review, will update as it progresses.

Update on TSC Elections (1m, Lee)

• Issue with info
• Super quick overview: TSC is me + 10 others. Every year half the 10 go up for election. We had a nominations process, and have collected names into a form. (Maybe a dozen nominations - will ask Brian.) Nominations have closed on 29th. Going to ping Brian to kick off the next phase.
• The 5 seats who aren't expiring will review the nominations and then we'll take a vote towards the end of December.
• We should have a new set of TSC members by January 6th.
• Mailing list / GitHub discussions

Shorthand for variables/arguments on operation (Saihaj, 10m)

• graphql-spec#889
• Saihaj: spec proposal. At The Guild when writing a query it's typically the same variable names as parameters that we use. We're proposing a shorthand for argument syntax e.g. user(id: $id) could be just user($id).
• Lee: thanks for the clearly written proposal.
• Aleksander: sounds nice but needs to be weighed against what syntax it might prevent in future.
• Lee [chat]: https://github.com/graphql/graphql-spec/blob/main/CONTRIBUTING.md#guiding-principles
• Lee: what you just expressed is a great example of "preserving option value"
• Lee: I don't want to take that principle to an extreme that limits our ability to make any change; but changes to the syntax itself should have a fairly high bar because of this issue. What might be helpful is to do ideas of what syntaxes this might conflict with so we understand what our lost options could contain. It's a good concern to raise.
• Aleksandar: what happens with clashes, how do we indicate this syntax is fine (older servers/newer servers). Federation needs to know how to handle this.
• Lee: given a server we don't currently know how to tell what features it supports. People either have documentation or they try it and it fails and they figure out why.
• Michael: many things can be checked with directives - if it has those directives then it has that functionality.
• Aleksandar: what happens if for example a federation tries to federate two schemas one which has the directives and one that doesn't - it gets hard to solve.
• Ivan: this proposal is fully AST based, so if Apollo Federation can parse it then it can just generate queries not using the syntax and send them. Maybe have a flag to disable automatic shorthand. Printer by default should use the shorthand, so newer version of GraphQL libraries would generate queries incompatible with all servers.
• Lee: having reviewed over the guiding principles again; a lot help us to govern cost
  • Value no change: high bar of added value
  • Motived by real use cases: I understand The Guild are seeing these problems.
• We need to see the positive case -
• "Simplicity is more important than expressiveness" - the long form is more descriptive.
• Michael: is it a positional parameter? By removing the label this becomes less clear.
• Matt: <action>
• ACTION: explicitly on the proposal do the cost/benefit - make sure we clarify the downsides and collect the upsides and motivations.
• Matt: still RFC0, but worth putting down all the details; it's still not super clear what the motivation is.
• Joey: how do we get these RFC0s on the agenda?
• Lee: file a PR! We merge them easily.

Client Controlled Nullability status update. (10m, Alex Reilly)

• Current action item
• Spec PR
• GraphQL.js Implementation
• WIP Integration with The Guild's GraphQL Code Generator
• I've implemented the syntax that Lee suggested: field[[!]!]?
• Questionmark operator changes something non-nullable to nullable currently - no error if it's null. There was suggestion that it could be used as an error boundary. The ! causes null to be an error, and some clients might blow up the entire response, so the ? could tell us not to do this. We could also use it to hold the unmodified return stuff in the error type so clients could look at errors and do interesting things with them (treat it as a non-destructive action). We didn't really land on a conclusion. How do we find consensus on this? I'd like to advance it by the next meeting
• Lee: great work! I'm super excited about this proposal.
• Lee: we have more questions than answers for the questionmark operator. I'm convinced we need it. We need to lay out all the options, work out the results of it (what would the spec look like? pros and cons?) and then use this to determine some paths are non-viable. If it comes down to one path then we can be confident that's the right one; if it's more than one then you can start tagging other folks to help get input.
• Alex: Great. Also, I've copied people's comments through to this thread.
• Lee: I'd like more details on what you're doing at Robinhood.
• ACTION - Lee: more details on Robinhood use case
• ACTION - Benjie: you said you had some thoughts but need time to write them down
• Michael: I'd like to see an implementation of this before we advance it much further; I'm excited about it. I'll try and implement it over the holidays. We've done this for defer/stream and we're discovering new things every day that are feeding back into the RFC process.
• Lee: there's a PR to graphql-js that you should be able to duplicate. The questionmark operator still has the basic implementation. You're right that we shouldn't advance to Stage 2 until we have at least one more implementation.
• Alex: I'm also working on a plugin to The Guild's plugin generator.
• Matt: if you and Jordan can present a united front of what needs to be done, because he's done the @required at Facebook and is familiar with all of the issues/etc - if he advocates for the same path that will be a very strong argument. (I know you've already been doing this, but it's worth reiterating.)
• Aleksandar: what does it do if it gets null but it says it's not
• Alex: it does the same as a non-null field in the schema.
• Michael: when we build the query execution plan we write the types on the selection, so we can rewrite the type to non-nullable or null, so the error behaves the same as it would for the field type.
• Aleksander: sounds like a "criteria". What happens if you get results from defer/stream that later are invalidated?
• Ivan: [19:50 UTC] Add timestamp to notes.
• Ivan: this could be implemented as an experimental feature (like we did for query variables on fragments) - can we merge it into GraphQL.js under a flag? For stream/defer we needed a separate release, but for this one we can learn from using it under a feature flag.
• Lee: sounds reasonable, I defer to your judgement.
• Ivan: I'm happy merging things with an active champion. We do need to discuss how to mark experimental features.
• Michael: why are you rushing to advance to stage 2 - is it just lack of time? We've had stream/defer in Hot Chocolate for a long time, people just need to enable a flag to use it.
• Ivan: I'm proposing we keep it stage 1, but merge it into graphql-js.
• Michael: if a PR is good we should move it, but we shouldn't rush it due to time concerns.
• Alex: I'm just looking to speed up iteration time, I don't want to advance something that's not ready to advance.
• Ivan: it's quite hard for people to try these experimental features when a proposal is just a PR, instead a feature flag can make it much easier for people to try it.
• Ivan: we don't need to merge it to stage 2; e.g. Matt's arguments on fragments proposal is still just a strawman and we have that available under an experimental flag.
• Matt: yeah, the expectation for experimental is that it might break on the next release - you're on the bleeding edge.
• Ivan: we use the word "experimental" in the naming so it becomes super obvious it's experimental.
• Alex: this works for me, I'm just aiming to try and get through the big iteration in Stage 1 this quarter as I have less time next quarter.
• [20:01 UTC] Lee:
• Alex: one more thing - does the number of dimensions in the list need to match for the non-null designator and the type? I've required that it does for now.
• Lee: thanks again!

Defer/Stream updates/issues/discussions. (30m, Rob Richard)

• Enforcing correct delivery order of payloads
• @stream directive initialCount validation
• Stream on scalar lists
• Allow @defer on root mutation or subscription fields
• Mutation serial execution with defer & stream
• Non-nullable field errors in defer/stream payloads
• [20:02 UTC]
• Rob: enforcing the correct delivery order of payloads
  • Discussion 17
  • A: A deferred fragment containing some non-deferred stuff and some more deferred (doubly deferred) stuff
  • I've made it so the server will know the fragments are paired and will hold it up to avoid inconsistency.
  • C: too hard for executor to know, so we'll still hold up the fragment
  • Lee: Example E seems completely non-controversial. Maybe reframe: "payload N is dependent on payload N-1 being delivered" - can this same constraint apply to the previous set of examples? Is it the merge path - it depends on the path existing?
  • Rob: I plan on describing it that way in the spec because that's how it's implemented in the code as well. I'll be sure to do that in the spec edits.
  • Lee: is the constraint too simplistic? Some of your examples have a lot of moving parts - are there cases when it cannot...
  • Aleksandar: this is a question of where we put the complexity - server or not. If we nail it down in the spec, we're preventing able clients from squeezing more performance out of the server.
  • Matt: just to clarify, we've established that existing clients (any that parse things into a graph format on the client) are incapable of handling paths that don't yet exist in the response. (E.g. Relay).
  • Aleksandar: so how do they do anything with in progress stuff?
  • Michael: they can apply patches; they've been doing this a long time.
  • Lee: there's another consideration to performance here - the order you put things on the network can be quite significantly important. There was a discussion: guarantees vs guidelines. We discussed the server having the ability to decide.
  • Lee: a counter would be to allow streams out of order, defers returning children before parents are ready - would it be okay for a server to decide not to prepare the nested field until the top one is prepared? If a child can be prepared earlier, is it required to return it...
  • Aleksandar: don't they want a JSON constraint that checks this?
  • Lee: giving the constraint right here could be important: graph payload vs tree payload. GraphQL -> TreeL. We've previously discussed a side-load of a tree with links.
  • You can't send a deferred payload unless the path at which it would be merged already exists. When we were to send a graph rather than a tree it would be merged into the existing graph - if the previous bit wasn't described then you'd have the same problem.
  • Aleksandar: it gets messy because of aliases.
  • Lee: yes, that's why we don't do it.
  • Michael: you essentially get rid of the fragments when processing this, and just keep the deferred fragments.
  • Aleksandar: it does not prevent an "unsafe" version later, so I think we're good.
  • Lee: extra feedback should go to this discussion.
  • Rob: before we move on, we're all in agreement that it should not be a free-for-all and they should go in a sensible order.
  • Lee, Matt: yes.
• Rob: next was validation of initial count
  • Discussion 24
  • Benjie raised there's no validation rule for this.
  • If we have initialCount: -1 as a variable then we cannot validate this at validation time because we don't have access to the variables.
  • Lee: i do not want to cross the line where validation requires that variables be available - a lot of services do this validation statically.
  • Validation should be generalised across all possible input values.
  • You could have a partial solve here where you throw an error if it's static.
  • Aleksandar: I'd like to see the value validation.
  • Matt: I agree with Benjie - it's outside the scope of this proposal to solve how to handle negatives for an argument that cannot accept negatives.
  • Lee: I don't think it's crazy to add a spec'd validation rule - we'd have to make it clear you also have to do runtime validation. But Benjie's right it wouldn't make sense to validate it statically but allow it to be bypassed via using variables.
  • Returning zero seems wrong as it might have been someone fat-fingering -3 for a count of three.
  • Michael: we'd have to put the validation into the runtime.
  • Aleksandar: the server has to validate one way or the other. GraphQL needs to remain simple. If the schema states that it should be positive, it doesn't say that it has to be positive and no client or server has to enforce it, and it's a hint more than a requirement. I'd love to have this type of stuff.
  • Matt: Rob do you have consensus on this?
  • Benjie: where would the runtime validation actually live?
  • <general consensus>
  • Lee: it'd be in the spec as an "assert initialCount is positive". This would result in an error origin
  • Benjie: "assert" is only used in a few places, and it's before the resolvers are called, I'm concerned about using "assert" here
  • Matt: it's okay if the whole query blows up when this is hit.
  • It'd be nicer to blow up the query immediately but if it has to blow it up later that's okay.
  • Aleksandar: the spec should specify the latest this error be thrown.
  • Benjie: I don't think that's viable because it it could be a @stream inside an @defer
  • Lee: I'm worried about introducing a new thing that only the @stream directive uses. At the base we're talking about "positive int" which our type system doesn't recognize. What if a user provided field wants to do this, @stream shouldn't have special handling. There shouldn't be two different ways of getting errors from these invalid inputs.
  • The majority of @streams will be top level fields so will be caught earlier.
  • Michael: so you would favour a runtime error.
  • Rob: are there other situations where the executor can do a runtime error like this?
  • Matt: the difference is this is coming from a directive rather than arguments
  • Aleksandar: something's got to validate these values.
  • Lee: result coercion throws. To Benjie's type we use "throw" and it's a "field error"
  • Benjie: yes I wanted to make it clear it's a "field error" not a "request error" - i.e. it's not a validation rule.
  • Lee: the cost of validating this as a validation rule would be spec complexity.
• [20:38]
• Steam on scalar lists
  • Discussion 21
  • "data" is normally an object, but for a streamed list of scalar lists it could be a scalar like 'banana'. Currently the spec states this is a map
  • Matt: we could clean up the spec itself and say "data is the type of the root type (which is always a map)" but "data is the type of the type that it's returning - when it's an object its an object, when it's a scalar it's a scalar"
  • Lee: history - it says it's a map because that's always true currently; but we should be able to refine that statement to make this work.
  • Aleksandar: that was always true because there was no deferral.
  • Michael: cool, we'll state it in the spec - that's what we need.
• [20:42]
• Other discussions, please leave comments on the discussion
  • Mutations / serial execution of mutation payloads
    • Please leave comments on the discussion
    • Lee: my opinion is that option 2 is the right option.
    • Benjie [chat]: My opinion: don't allow @stream/@defer on anything except query operations, address it later.
      • Aleksandar: I can agree to that.
    • Matt: I completely agree with that
    • Michael: I agree
    • Rob: relating to Relay, components might have @stream/@defer on them via fragments and this will cause issues when mutations pull in these fragments.
    • <general> Good point.
    • Matt: We definitely need to resolve this, but do we need to resolve it to advance to the next stage?
    • Lee: it seems that there's a strong signal that consumers want the option 2 behaviour.
  • Allowing defer on root mutation or subscription fields
    • Benjie suggests not to allow
    • <Everyone agrees>
    • Michael: it'd become totally unclear what happens when we execute the mutation
• Lee: I have to cut you off, but hopefully people will engage with these discussions. This is by far the most complicated RFC to hit GraphQL, using a separate repo with discussions to track it works well - well done.
• [20:49]

Discuss GraphQL mutation design(s) with large schemas (nested mutations) (30m, Aleksandar)

• Details document: GraphQL Mutations and Large Schemas
• Nested mutations (PowerPoint Show) (DRAFT before the meeting)
• [20:49]
• Aleksandar: most people seem to develop schemas with all mutations at root level of mutation type.
• This is not the only way it's done; I've seen a lot of places suggesting that we should not do mutations just at the root level.
• We face "namespace explosion" - I have hundreds of types, in an on-premises box, it's a massive API. It'd need 10s of thousands of root mutations if I only used root mutations.
• Another option is to complicate the input, which becomes a massive mutation that can mutate anything which beats the point the other way around.
• There's also an issue of "respecification of the context" - e.g. respecifying common fields over again.
• There's a lot of validation that needs to be performed (at write time); but if we only have root mutations we can only do individual root mutation validations and then validate at the very end to make sure the picture is complete.
• Why not concurrent mutations?
  • Data model can allow for concurrent mutations. Output can be the same and the mutations can not affect each other so it's fine.
  • I can't do them in parallel because the spec says I have to do them serially
• Who has control? We're telling the server to execute mutations sequentially. The server has to accept many requests at the same time from many sources and these requests might all come from the same user at the same time, so it has to know how to do this without exploding. The client also has to know what it wants to accomplish - travel to A first or B first before getting to C.
• For things that are not interdependent we're advising people not to do this, and some are ignoring that advice.
• I see GraphQL as "Mutual respect" - clients are clear about what they need, servers are clear what they need, GraphQL spec enables these both.
• I see examples of people nesting mutations inside the selection sets of parent mutations, providing context for inner mutations.
• Namespacing is implicit here, we don't have thousands of root mutations, it's clean. Input types are simplified.
• Elementary mutations are possible in addition to complex ones - mutation simplicity.
• We could execute a root level mutation and it's child selection set as a transaction with locking (independent of the data sources used - doesn't have to be a DB).
• I don't agree that mutations need to be serial.
• We could use a directive to tag that a field be executed sequentially rather than a "serial" type.
• If client requests sequential execution and server cannot comply, then server must throw.
• [21:05] Could add @dependsOn directive to detail the order of the mutations.
• Could use this to allow the output of one mutation to be used as the input in another.
• Clients want to be able to find something and then apply a mutation to that.
• We could apply directives to the schema itself so that clients know what capabilities it has.
• See the Markdown document for more details.
• This is important enough that quite a few people have documented how to do things in different ways.
• Lee: I've opened up a discussion thread in the WG channel to make sure people have the opportunity to weigh in:
  • #815
• Make sure you represent the historical context of this too.
• Oleg raised one 3 or 4 years ago on namespaces which has quite a lot of discussion.
• [21:10] Thanks everyone, we're 10 minutes late - have a great holiday period, see you next year.