Skip to content

Latest commit

 

History

History
279 lines (260 loc) · 29.2 KB

2022-05-05.md

File metadata and controls

279 lines (260 loc) · 29.2 KB
Raw

GraphQL WG Notes - May 2022

Watch the replay: GraphQL Working Group Meetings on YouTube

Agenda

  • Agree to Membership Agreement, Participation & Contribution Guidelines and Code of Conduct (1m, Lee)
  • Introduction of attendees (5m, Lee)
  • Determine volunteers for note taking (1m, Lee)
  • Review agenda (2m, Lee)
  • Review previous meeting's action items (5m, Lee)
  • Discuss new intersection type proposal (15m, Yaacov)
  • Defer/Stream
    • asynchronous iterators of iterables versus of items, continuation of last WG meeting (15m, Yaacov)
      • Should resolvers returning AsyncIterables returning AsyncIterables of items or AsyncIterables of Iterables of items?
      • Should the AsyncGenerator returned by graphql-js yield items singly or yield all available items (in array or otherwise)?
      • Background: robrichard/defer-stream-wg#38
      • See above link for polls, still waiting for feedback explaining votes.
    • Stream Payload index format (10m, Rob)
  • CCN status update and rollout plans (10m, Alex Reilly)
  • @oneof - time to promote to RFC2? (20m, Benjie)
    • Spec PR
    • graphql.js PR (thanks @erikkessler1)
    • Main hesitation: should we mirror @oneof in the output types (i.e. type Foo @oneof { ... })
      • If yes: what does this mean for unions?
      • If yes: should we spec this a little before merging the input-only @oneof?
      • If no: is it desirable that input polymorphism and output polymorphism is so different?

Determine volunteers for note taking (1m, Lee)

  • Benjie
  • Stephen
  • Tim

Review agenda (2m, Lee)

  • No notes.

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

Discuss new intersection type proposal (15m, Yaacov)

  • Alternative to unions implementing interfaces discussed at last WG
  • spec PR: graphql/graphql-spec#941
  • graphql-js PR: graphql/graphql-js#3550
  • discussion: #944
  • {time}: 19:14
  • Yaacov: New discussions around intersections. Motivation is from perusing the open issues on the GraphQL Spec repo. Imagine generic Connection / Node / Edge interfaces. We have interface Pet and type Cat/Dog implements Pet & Node. How do we define a HousePetNode that states it implements Pet & Node and is currently either a Dog or Cat?
  • We already have union HousePetNode = Dog | Cat but that doesn't allow us to use their interfaces - we don't know it implements Node. If we inferred this and then we added another house pet that cannot support Node then the type system would break.
  • First suggestion was to add a constraint/annotation, e.g. union HousePetNode implements Pet & Node = Dog | Cat - then anything added to the union MUST implement those interfaces. implements might be misleading since the union itself doesn't really implement it, but generally speaking we could change the keyword.
  • I've come across the concept of "intersections." We could have intersection HousePetNode = HousePet & Pet & Node where union HousePet = Dog | Cat. This would be an alternative type, rather than a constraint on union.
  • [19:21]
  • Con for additional constraint: If you wanted to add Fish, you couldn’t in the union scenario
  • Interfaces could be an option as well for this scenario, for example HousePetNode.
  • We have to add this interface to Dog and Cat, but we'll end up with a lot of duplicate interfaces (HousePetNode is just the same as Node, except we've only added it to Dog and Cat).
  • Michael: [very paraphrased] these pros/cons that you've prepared is what I wanted - it's like what we did with input unions and really helps us explore the solutions.
  • Yaacov: I’m in favor of the intersection type as the best solution to this problem, but not sure if it’s worth it. Not yet sure, if it’s necessary.
  • "second order abstract types" would be great to have.
  • Anthony: there might be another con with the intersection type. Unsure on intention, you mention adding Fish (without Node interface) to HousePetEdge would break the types. It seems to me that in the intersection if you added Fish to the union then it simply would not be available in intersection HousePetNode since it does not match the given constraints. This would fail silently, rather than throwing an error, which may not be the desired behavior.
  • Yaacov: great point. If you were to query on Fish then you'd get a validation error because it's not one of the valid types. Because you have to include fragments you might get the issue at query time. If you don't explicitly need the subtypes and only the interfaces then you might not encounter it, and get the silent fail you mention - this is a potential footgun.
  • Anthony: If you do a query on HousePetNode you cannot add fields directly from the interfaces? You need to use fragments?
  • Michael: this was my question too.
  • Yaacov: we did discuss this, most people were for but I think Ivan was against. Because intersections don't have to include any interfaces, we should allow them to not implement any fields at all.
  • Michael: isn't it then basically a union?
  • Yaacov: it's very similar to a union.
  • Michael: so it's a union that can chain in other unions?
  • Yaacov: yeah, and it could even be an intersection between two unions.
  • Michael: this seems confusing; I thought it would constraint the union so that you know that certain fields are always there.
  • Roman: I agree with Anthony's concerns - it's potentially confusing. Replacing validation at schema build time with runtime is not a good enough substitution. I'd have to look at Fish to figure out why it doesn't appear in the final intersection type, because I'd expect it to be there from looking at the HousePet union.
  • Anthony: what's the purpose in general other than querying fields in the union (which you're not proposing).
  • Yaacov: I'm not too excited about losing wrapping fragments, it's only a few characters. The motivating example is can we allow this "HousePetEdge" to truly implement Edge. There's no way to do that currently, which offends me. This is the new feature; but this is based on my reading through issues on the repo.
  • Anthony: I'm the iOS developer at Apollo, and we do a lot of code generation. Compiled clients have to deal with the schema changing out from under us in ways that web clients don't have to. The biggest issue with unions is that I can't guarantee anything about the new types. If the union defines that it implements a particular interface then there are things I can rely on for new types. Being able to query those fields directly is a big deal for us.
  • Yaacov: if something returns a HousePetNode and the server tries to return Fish you'll get a server error.
  • Matt: on the server you won't get an error when Fish implements HousePetNode; but the client won't get this information. The server's schema is newer than the client. I'm responsible for Meta's client native code implementations, and in my view using interface inheritance feels like a clean way to solve this, and is explicit. Intersection, like unions (I hate unions and discourage their use) - you don't know how the fields will change underneath you, and it tells you nothing for a new type.
  • [19:40]
  • By using an interface you're very explicit about what fields are supported - whatever comes along in future is guaranteed to have these fields. With the intersection we're disaggregating fields on interfaces and types that ripple through to... I can invalidate a usage of an interface by removing a field on an interface that's never used on the interface itself and only on the intersection - I have to work through more hops to figure out why my code isn't compiling.
  • Michael: you're combining unions in an intersection, maybe changing it, it's much more difficult to see how things are affected.
  • Yaacov: in general, I agree - there's complexity - but most is contained inside the idea of a union, and they're union rather than interface because they don't have any common fields. There's not much you can do about that problem. What intersection does is to say that those types in the union do implement those interfaces without having to create a number of duplicate interfaces.
  • Lee: we're hitting up against the time limit. Thanks Yaacov for preparing this doc - it really helps us understand the space. Much easier to compare/contrast possible solutions - it's incredibly valuable. I agree with most of the feedback. Last week I didn't quite get it, but marking it as an intersection really clarifies it, but makes me prefer your original proposal. It's not just a filter - if you do something in the schema that invalidates the constraint we can statically make noise because we know the intent.
  • Lee: "implicit vs explicit" is a tradeoff against expressivity. The more expressive you are the closer you get to Turing completeness and you have to do more work to find out what the effects are. We've been careful to try and make things obvious by just looking at the local types, without having to follow chains. Because types always have to be concrete you never have to follow a chain. This abstract type of abstract types would break that - I agree with the con you raise here. I'm skeptical, but it does make me feel better about your original proposal.
  • Yaacov: I'm hearing that the intersections come with the additional complexity.
  • Anthony: we talked about this about a year ago at Apollo and we had basically universal support so I'm a proponent of doing something.
  • Lee: I'm coming around to it. What would GraphQL unions have looked like if we hadn't implemented the union type.
  • Yaacov: why can't we have empty interfaces?
  • Lee: the only use case is a "reverse union" which (in my opinion) is everything a union is but more confusing. Facebooks PHP codebase was littered with these, trying to represent unions. Using interfaces can however become a really confusing large list that makes it hard for you to wrap your head around which ones are important and which ones just nominal.
  • Lee: what you've done here has proved that we're not reinventing the wheel, we're just reassembling the parts in a pattern that we've not previously seen. What we want is a schema build time constraint that we can validate, and intersections don't give us that. implements is probably the right keyword.
  • ACTION [Yaacov]: I'll work on refining the tests and so on for the next meeting.
  • [19:56]
  • From chat:
    • From Anthony Miller to Everyone 07:40 PM
    • “Hate unions and discourage their use” has always been my disposition
    • From Michael Staib - to Everyone 07:40 PM
    • agree
    • From Laurin to Everyone 07:43 PM
    • depends on the use-case - for this use-case (shared fields) interfaces might make sense
    • but for other uses cases e.g. error handling (oneOf or unions make more sense as an error and success usually dont share fields)
    • I also like on unions that I see what results are possible from looking at the union definition, where with interfaces I first need to find the types that implement the interfaces
    • From Anthony Miller to Everyone 07:44 PM
    • Definitely agreed Laurin. I’m great with limited unions that have 2-4 types in them for very specific use cases (like union Result = Error
    • From Marc-Andre Giroux to Everyone 07:45 PM
    • Good point @Laurin. Unions are defined in a local way and semantically are better suited for scenarios like you describe, even though I do think like Matt they are quite annoying in practice.
    • From Anthony Miller to Everyone 07:45 PM
    • Union Result = Error | Success*
    • From Marc-Andre Giroux to Everyone 07:45 PM
    • Having errors backed by an interface makes the down points of the union much better. IMO this can be solved by tooling (linting, validation rules) for those specific cases
    • From Laurin to Everyone 07:45 PM
    • I also agree that backwards-compatibility and unions/interfaces is a science in itself
    • From Matt Mahoney to Everyone 07:50 PM
    • The nice thing with the constraint: it’s a promise to existing clients that the server will never remove the constraint. It would make removing a constraint from a union a major breaking change, but that’s probably a good thing.
    • From Anthony Miller to Everyone 07:50 PM
    • !00% ^
    • From Michael Staib - to Everyone 07:50 PM
    • Exactly 100%
    • From Laurin to Everyone 07:51 PM
    • 💯
    • From Michael Staib - to Everyone 07:55 PM
    • I think this would also make the error patterns that exist for mutations easier
    • https://productionreadygraphql.com/2020-08-01-guide-to-graphql-errors
    • There could be a stage 6b 🙂

CCN status update and rollout plans (10m, Alex Reilly)

  • {time}: 19:57
  • Alex: we have an initial implementation, relying on a canary release of graphql-js, it does code generation and works.
  • Michael: latest HotChocolate supports it in .NET too
  • Alex: excellent!
  • Hugh: we're also working on it across the board at Apollo, preliminary stages but will share more soon.
  • Alex: what's the tried and true process?
  • Lee: a valuable thing we did before was to open an issue/discussions board saying there's a flag to turn on and encouraging people to do so. Most of the people who'd provide that feedback are in this room, but sometimes you get lucky.
  • Michael: what about Relay? Do they have more feedback? Have they implemented? Any feedback?
  • Matt: Jordan's not here and he's the best person. Relay's not done any work with server capability of client defined nullability, but they have a facade that fakes it for the client.
  • Michael: that's just duct-tape?
  • Matt: yeah. I don't want to put words in Jordan's mouth, but it probably doesn't make sense to implement server-side CCN until there's a mechanism for fragment-level keys/saying that a fragment "is null."
  • Alex: they want to strip out client designators and process on the client.
  • Lee: they still want to use the syntax. There's open discussions around semantics.
  • Alex: Yelp will be piloting it in a couple quarters; is anyone else intending to try it out early on?
  • Stephen: What's Yelp's server stack?
  • Alex: (python and node)
  • Stephen: we have a version of this that's not quite compatible with what's in the spec, we'd love to get this to work so we can get feedback from our UI developers.
  • Matt: this has been glossed over because it can be solved by validation; but it allows fragments from different products to have more overlapping concerns, which means that it's easier for me to break other people's fragments far away by introducing some CCN in a central query. That's not a concern that should stop us advancing, but it may block Relay pushing/encouraging it on Relay-level servers.
  • Anthony: I was against it before, but I do think it makes sense for fragment modularity.
  • Alex: we're looking to merge parsing/lexing relatively soon (Ivan just reviewed). We need to land on how errors work.
  • ACTION: Lee: sounds great. To collect the feedback, create a canonical discussion thread talking about the alpha release, things you want discussions on/etc - one nexus point for feedback.
  • Alex: can we add a log or something to link to the discussion thread.
  • Roman: I suggested renaming CCN to "client assertions" because it seems confusing. Seems people agreed? It's an assertion rather than a server error. Client is asking to make an assertion that this is true.
  • What does it mean for "?"
  • Roman: it's impossible, you cannot make nullable what is not nullable.
  • Lee: to generalize, we should make sure we have the appropriate names/ways to describe these features. The names that make sense for one semantic might not make sense for another. But I do agree, the name we land on must make sense. I've been thinking ? is an "error boundary". ! is an assertion; other languages have similar syntax, but all use different names, e.g. Swift uses "unwrap".
  • Roman: let's at least continue the discussion
  • ACTION - Alex: open a discussion thread on naming.
  • Ivan: Alex has done a great job, what's concerning me is naming. Even though it's experimental, if we rename it, it will be a breaking change. I'd rather we did the discussion up front before we commit to an API. Separate thread for naming makes sense. Need generic name for entire syntax for ?, ! and []. In AST we have one union type for these three types of nodes - so we need 4 names, and ideally decide those before we merge.
  • Lee: I don't want perfection to be the enemy of the good, especially when we're iterating - we could spend a month picking the right name, and then change behavior based on feedback and now the name doesn't make sense any more.
  • Michael: there's similar names in other languages, we should look at their syntax trees.
  • ACTION: look through some naming conventions.
  • Roman: philosophical thing: if you want to understand a thing, name it. It's important for end users to name it right - it should bring them immediately 50% of understanding.
  • Lee: thanks for the update, great work.

Defer/Stream: asynchronous iterators of iterables versus of items, continuation of last WG meeting (15m, Yaacov)

  • Should resolvers returning AsyncIterables returning AsyncIterables of items or AsyncIterables of Iterables of items?
  • Should the AsyncGenerator returned by graphql-js yield items singly or yield all available items (in array or otherwise)?
  • Background: robrichard/defer-stream-wg#38
  • See above link for polls, still waiting for feedback explaining votes.
  • {time}: 20:15
  • Yaacov: I reported on issues with the performance of @stream where we might want to yield a subset of items in each payload rather than item by item. Two levels: resolvers returning a list of items; and also when GraphQL.js is returning payloads with completed results. At both levels, are we returning lists/subsets of items.
  • Yaacov: Proposing that what goes inside the AsyncGenerator that execute returns should be an array of execution results rather than a single execution result. For the same reason, we may want to allow the underlying services to return batches at the same time, so should we be returning an item or a list of items?
  • Lee: it seems to me that it would be useful to ensure that as batches of data become available we should deliver batches to the client. This isn't just about how it's described in the payload, it also has a concrete effect on how clients would consume it. Knowing that you have three changes that logically happened at the same time you can make on change to your store rather than three separate changes. Can we make execute/subscribe have the same signatures? AsyncExecutionResult can never be an array type. Maybe: either it's an AsyncExecutionResult or it's an array of AsyncExecutionResults.
  • Yaacov: I was thinking that we should make it so that they're always arrays.
  • Ivan: It'd generally be more logical for people to implement an async generator, but wrapping it in an array makes it surprising. Returning an array from the resolver may lead to strange developer experience.
  • Rob: I thought it might be nice to force client developers to always consider it returning an array - then they're not getting into the footgun situation of thrashing their client on a lot of concurrent updates.
  • Roman: why is this discussion here? The document says a spec change does not look necessary - why are we discussing a graphql.js issue that does not affect the spec in any way?
  • Yaacov: the spec alludes, it's not specifically in there, but in terms of the actual GraphQL payloads that is in the spec and it needs to be clarified, but the other part isn't. I've linked to two spec change PRs that I would make.
  • Lee: I was also wondering if this was TypeScript changes, or if it was actually about the shape of the payloads (which we've discussed before).
  • Yaacov: they're definitely related to payload shapes.
  • Rob: at least for the execution result. Maybe we should talk a bit about the return from the resolver as that may be more of an implementation detail.
  • Lee: making sure the payload shape can represent simultaneous events in a batch whilst also deciding if "result | array<result>" vs "array<result>". If 99% is going to be array of size 1, 99% of users are going to be wondering why that array is there.
  • Yaacov: great, we'll take it back to WG/out of band discussions.

Defer/Stream: Stream Payload index format (10m, Rob)

  • robrichard/defer-stream-wg#39
  • {time}: 20:29
  • Rob: we've gone through a few iterations. We determined we should always return an array of size one right now. Previously it has the index in the path array, there was discussion if it was needed at all, should it be in a separate property, or should we keep it in the path.
  • It's clear it's ambiguous without the index in the payload at all - two fragments both deferred with the same stream inside them, so there's no way to distinguish the order, and they could become interspersed. So we can rule out option C of omitting the index entirely.
  • I'm leaning towards atIndices that can be an array, giving us room for expansion for allowing out of order responses, sparse responses, etc.
  • Lee: I worry about the tradeoff between flexibility and the complicated logic about merging the results back together. It's useful to support streaming in chunks per list. But these are typically always clean concatenations. Are you saying there's an ambiguity about how to perform that concatenation?
  • Michael: is it necessary to add this complexity for a case that we might remove?
  • Lee: even if the index is there to validate right now, it seems good because we may have missed a corner case and this would catch it.
  • This example suggests that there's two streams that write to the same spot.
  • Michael: this feels like a very constructed case.
  • Lee: barring the label argument, could you not determine the fragments are duplicitous?
  • Benjie: do we need to support nested @stream and @defer
  • Lee: yes, because fragments
  • Michael: we have an example application of this, we need it
  • Lee: concat is not idempotent. "Set at index" is idempotent, but limits us to be able to do one thing. "Set at index range" feels right; atIndicies makes this more complex, index range makes it a lot simpler.
  • Matt: at Meta, our implementation of stream merges - we cannot give you two instances of the same item in the list in the streamed payload. Maybe we really want to ensure that even if you get a stream under a defer - do you really need to know which one it came from? The path dedupes it, maybe we should have the server guarantee it only sends one instance of each item in a streamed list.
  • Lee: if everything's idempotent then the worst case is spinning a bunch of compute that you didn't need to, rather than putting you in a bad state.
  • Rob: I'd love to know Meta's logic to dedupe that; it's not something the GraphQL.js system is doing.
  • Michael: How is it working with data masking in Relay?
  • Matt: for Relay you have fragment level subscriptions to a central store; so we know an update happened to this node in a normalized store.
  • Lee: feels like we settled at option A. Instead of saying ignore the index and concat, the operation is explicitly "set at a range, where the beginning of the range is at the given index, and the length of the range is the length of the data."
  • Michael: so if we get the same data again, we just overwrite it.
  • Rob: it might make it trickier to support out-of-order payloads for sparse lists.
  • Lee: there's a difference between saying "here's a single operation where I'm setting at this range" versus "here's a batch of asynchronous payloads".
  • Greg: <question on hasNext>
  • Rob: this relates to the entire operation rather than an individual stream inside it, and we cannot answer that ahead of time since we don't know when the data will be done. When the data is complete we can send simply {hasNext: false} to terminate, even if delivered out of order.
  • [via chat]
    • Matt Mahoney to Everyone (9:49 PM)
    • I was wrong: we do not de-duplicate, and in fact it’s even worse: we send ALL the chunks for one deferred fragment then ALL the chunks of the second.
    • So we have the same bug, but because the last element in our path is the index, we can de-duplicate client side
  • Rob: so in summary, are we leaning towards option A but updating the language to "indicates starting position, overwrites length of array"
  • ACTION - Rob: ensure the spec wording is accurate.

@oneof - time to promote to RFC2? (20m, Benjie)

  • Spec PR
  • graphql.js PR (thanks @erikkessler1)
  • Main hesitation: should we mirror @oneof in the output types (i.e. type Foo @oneof { ... })
    • If yes: what does this mean for unions?
    • If yes: should we spec this a little before merging the input-only @oneof?
    • If no: is it desirable that input polymorphism and output polymorphism is so different?
  • {time}: 20:53
  • Benjie: It’s not immediately obvious if it should be part of the output schema or not. Spec PR is finished to add this as input. We could theoretically merge it.
  • Have we satisfied ourselves here?
  • I’d like to answer questions around output before going into input.
  • If we’d use it as a replacement for union, they can represent scalars, lists, …, which adds new capabilities.
  • What does it mean for unions? They’d be semi deprecated.
  • If we don’t want to add as output type, is one of the input types a solution?
  • We’d have different type polymorphism for input and output here.
  • Lee: We should decouple the clear and unclear parts.
  • Original goal was around input unions.
  • Way to explore this: Explore output types far enough, to make sure it doesn’t infer too much with the way we do input types.
  • Benjie: Makes sense, happy to do that. If we’d add it as an output type, that research makes sense.
  • There were a number of strong voices who never wanted it as an output type.
  • If you have a strong opinion, please let us know now.
  • Anthony: Agrees, that you’d have 2 competing things accomplishing the same thing. Once we make unions more powerful, do we do that to both? Will get dangerous.
  • Why was this chosen as a better alternative to extending unions?
  • Lee: There’s documentation on this that is worth the read.
  • Let’s separate design from decision.
  • We had many considerations, polymorphism, type-reuse, agreed that they were still cleaner over input unions. We should still explore the design space, especially the input type design space.
  • We don’t need to find out, if the output types are worth doing or not, in order to have progress on the input types.
  • Benjie: Let’s move it to RFC 2. I don’t mind to do it next time either.
  • Lee: I’ll do it now. It should be a safe change. It has been sitting in a solid state. Done.
  • Wrapping up nearly on time!
  • Thank you all!
  • [21:01]