Skip to content

Latest commit

 

History

History
289 lines (231 loc) · 26.7 KB

2020-03-05.md

File metadata and controls

289 lines (231 loc) · 26.7 KB

GraphQL WG Notes - March 2020

Video recording: https://www.youtube.com/watch?v=aezw-RYYDwY

Agenda

Introduction of attendees (5m, Lee)

Agree to Membership Agreement, Participation Guidelines and Code of Conduct (1m, Lee)

Determine volunteers for note taking (2m, Lee)

  • Benjie

Review agenda (2m, Lee)

  • Two new topics added (see below)

Custom Scalar Specification URIs Update (10m, Matt Farmer)

  • Still waiting for graphql-js 15.0.0 to be cut. Scheduled to be included in 15.1.0 which would be released a week or two later. Relevant Discussion.
  • PR has been rebased off of v15.0.0-rc.2
  • Custom Scalar Specification URIs RFC Issue
  • Custom Scalar Specification URIs RFC PR
  • GraphQL.js PR
  • Question: should the spec suggest or require that implementations validate the URL is well-formed?
  • Lee: what value does it add beyond pedantic correctness?
  • Matthew Farmer: If GraphiQL or other tooling depends on it, we shouldn't pass the validation of this on to them.
  • Evan Huus: We can use the IETF RFC for URL validation.
  • Andi: this is almost recursive, since URL parsing is non-trivial. You'll potentially end up with incompatible implementations. I'm not sure it's not adding value.
  • Lee: I agree with Andi. Precedent: if you put a malformed URL into an HTML anchor tag the page will still render.
  • Lee: the fact it's a URL is only a human concern; to tools it's an opaque string.
  • Morris: Even if it's a valid URL you may not be able to click it.
  • Evan: we can always add it later if it becomes a problem.
  • Lee: the power of these specified by URLs only become powerful when clients and servers agree.
  • Matthew Farmer: I agree; but devil's advocate to adding it later - adding it later would be a breaking change; adding it now would not be.
  • Lee: it could potentially be breaking
  • Matthew: I rebased last night, Ivan has given me feedback yesterday which I can look at today.
  • Matthew: Is this going to be in GraphQL 2020?
  • Lee: I'd like to draw a hard line - this will be the first major [thing] to be in the 2021 spec cut. People track the draft more than the releases.

Union fields with the same name but different types (10m, Sasha Solomon)

  • Discuss if this is an issue and if it can be addressed and the reasons behind the original change.
  • Relevant issues/links:
  • Sasha: Why was the change made? Was it deliberate? Can we move forward towards taking this restriction out?
  • Lee: the original concern was that the payload of that object would have a single "reason" field (based on the example) and the type of that field is unknown because __typename has not also been queried. So if you're a strictly parsing client you may not be able to proceed. For example if you have a: Date and a: URL (both custom scalars) then the client cannot know whether to treat the value of a as a Date or a URL - the consumer does not have enough information to deserialize the JSON into local objects.
  • Mike: this is a client issue - they've explicitly asked for ambiguous results.
  • Michael: the client can always ask for the __typename
  • Mike: or alias the field.
  • Sasha: clients that we're using are able to differentiate, but are being forced to use aliases.
  • Mike: this only fixes a smaller issue because field arguments prevent merging.
  • Michael: these are separate types/separate selection sets so the selection set do not get merged
  • Evan: you could reproduce this with interfaces
  • Michael: this is a separate issue with scalars
  • Mike: I agree. This won't solve all conflict issues.
  • Kewei: at GraphQL conf last year Matt Mohoney had a talk about this. At Facebook we group all of these issues under the name "Fat Interface". In order to prevent developers ever writing conflicts we have to do a full schema validation checking for conflicts everywhere. Causes a ridiculous build time, or write tests that takes 5 minutes to run. MM talked about modular fragment model that doesn't rely on this "Fat Interface Model" or having to resolve the union names.
  • Mike: I'll definitely check that out.
  • Sasha: what's the path forward? This is something we've hit into a lot at Twitter. Is this something we could change?
  • Andi: is this only for unions, or also for interfaces?
  • Kewei: it's the same problem if the interface does not specify these fields
  • Andi: we had edge cases in the past where the current spec doesn't capture all conditions, we should explore this if we want to go further
  • Greg: I think this problem has another root to the problem. When the fields are merged one is chosen and the others are collapsed.
  • Lee: this is one validation rule in a family of validation rules about how selection sets are allowed to exist and interact. It solves two problems:
    • Is the query unambiguous to run? This is about arguments -> if you see two fields with different arguments you won't know which one to run. We don't want to change this.
    • Something could be completely unambiguous to run on the server, but ambiguous on the client. That is what we're talking about there.
  • Lee: Historical context: originally it was expected that all validation would be done on the server. There was only one set of rules for validation, so they are strict since it has to make sure that the queries are unambiguous for both server and client.
  • Lee: Sasha you could start to look at what separating these two rules would be like. Can we take this rule out? Is it okay for response payloads to be potentially ambiguous? The client can make it unambiguous - it's on the client to do that.
  • Lee: Should there be a tool or a something in the spec to detail what the client's responsibilities are related to this. We've talked about "GraphQL Linter" in the past. Perhaps this rule doesn't go away, but it's optional?
  • Lee: one last thing - the validation rules are not always perfect in validation, they assume they'll be run as part of the group. Conflicting argument names does not recurse - it assumes that if there are two fields with different types being returned it that it's going to fail elsewhere. So we could get back to the server ambiguity case if we remove a rule to make sure we've not broken any edge cases.
  • Sasha: I can see how this would be a problem from digging in the Sangria code.
  • Greg: how does this impact the client in general if I have a client that uses an interface or union, and the server resolves, and the client specifies fragments on different types.
  • Lee: exactly, we're talking about unions here, but really the problem is on fragment spreads. We can get into a situation where the payload is ambiguous on the client. The question here is: "is that okay?" Is it okay to put the burden of this on the client?
  • Greg: there's always the spec defined __typename, so that should be the discriminator for the client, so maybe we need to say in the spec something like: please ask for additional type information and then we can decide what to do.
  • Lee: there's an interesting question here: should we completely remove the client ambiguity protections from the spec and allow this wholesale, or should we have something that disambiguates.
  • Evan: there's definitely something to be said for preventing people from going down a path that makes it easy for them to shoot themselves in the foot. We avoid this category of issues by being strict about this in the spec.
  • Ivan: we also need to think about other programming languages where a type needs to be defined for a specific fragment; e.g. where code generation in a very strict language and __typename is not sufficient because there's no way to handle disjoint unions.
  • Lee: Objective-C and Java did not have a way to implement disjoint unions which is why this validation rule was added.
  • Lee: "should this be a spec'd rule that affects all clients, or should it be left to the client?"
  • Lee: our code generators used to validate the query first and only generate the code if it validated and was non-ambiguous.
  • Lee: we were worried that problems would be detected late (i.e. at the code generators stage) rather than giving you realtime feedback in your editor. That's part of the historical context.
  • Sasha: Action: Piece all of this together and come up with some kind of proposal.
  • Lee: this at least shows that it was worth discussing!
  • If anyone is interested, ping Sasha an email.

Full unicode support graphql/graphql-spec/issues/687 (10 min)

  • Andi: I've raised a PR against graphql-js and a spec issue to discuss about extending to full Unicode support. Other language (e.g. ruby) already support this, but were not tested, so I've added tests. And in graphql-js added a validation rule.
  • Looking for feedback and how to move forward. It's basically a cleanup. The big change in the spec would be to change the range of codepoints to support full unicode.
  • Lee: does this cover everything that you want full unicode support to cover, or is it a first step?
  • Andi: everything.
  • Andi: Lee raised a PR that removed a lot of the restrictions which lead to some interesting discussion around control characters 2 years ago, this builds on that factoring in the discussion.
  • Lee: you should address multilingual plane control characters. I think the PR currently only handles surrogate pairs?
  • Andi: we already have escape codes inside the BMP; this PR uses surrogate pair escapes and leans on JSON to escape codepoints outside the BMP. I didn't want to add any new syntax or new way of doing escape codes, so this is the most minimal change.
  • Lee: if the thesis of this change is having first class unicode support, the surrogate pairs is kind of a hack to expand code that wasn't designed to handle full unicode support to actually meet it. The Math to turn a surrogate pair into a full codepoint is complex.
  • Andi: there's a reason people invented the more convenient 6-digit escape codes to handle this.
  • Andi: I didn't see a reason to prevent surrogate pairs
  • Lee: good point - we shouldn't prevent people doing this; but it's definitely worth having the escape sequence in addition.
  • Lee: I'd like to see you turn this into a spec PR also.
  • Andi: happy to open a PR
  • ACTION - Andi - open a PR
  • Ivan: are you supporting surrogate pairs, variable length, or both?
  • Andi: this PR adds surrogate pairs, but Lee is suggesting that we should also add the 6-digit escaping
  • Ivan: JSON only supports surrogate pairs, so adding the other escape form may be surprising. JSON the dominating format, so having something that is different would be weird.
  • Lee: I hear you, but before this we didn't have surrogate pairs support, right?
  • Andi: actually we did, because Java and JavaScript use UTF16, we just didn't have tests for it or mention it in the spec.
  • Lee: I was on the fence about surrogate pairs, but since we already supported it implicitly making sure we support it explicitly makes sense.

Input Union RFC (10m, Vince Foley)

  • graphql/graphql-spec:rfcs/InputUnion.md@master

  • Vince: Mostly there hasn't been a lot of activity and my time has been eaten up

  • Vince: Lee opened a PR adding himself as a champion to one of them

  • Vince: I've added myself as a champion to two of them. Benjie wrote the

  • Benjie: oops, I thought I had added myself as champion - I'll add a PR

  • ACTION - Benjie - add yourself as a champion to the Input Union spec

  • Vince: I'm adding myself as a champion of it too, I think it should be in anyway

  • Evan: I'll add myself as a champion for number one with the explicit __typename

  • ACTION - Evan - add yourself as a champion - Done graphql/graphql-spec#696

  • Vince: what's the job of a champion going to be here?

  • Lee: it's on us to push on this a little bit. I've also been slogged down on other non-GraphQL things.

  • Lee: one idea is that we can have an out-of-schedule one-off meeting for the champions to meet alone, and hopefully we can come back with some clarity. Maybe we should have some homework before we do that.

  • Lee: the job of the champion is to hold the null hypothesis - "I'm excited by this one but it's on me to set this one down when something else is more exciting"

  • It's hard to do this because we've been working on this for a long time; but if you want to be a champion expect to do movement on these.

  • Gabriel: (potential agenda item) - one thing that we could improve is details around oru processes and roles - it's not so clear what a "champion" involves and how to be one. TC39 has great documentation around this, we could work on having something like that.

  • Lee: I agree, we have some guidelines on this (not as detailed as TC39) but we at least have definitions for what a champion is and what the stages are: https://github.com/graphql/graphql-spec/blob/master/CONTRIBUTING.md

  • ACTION - Lee - Add https://github.com/graphql/graphql-spec/blob/master/CONTRIBUTING.md

    to the agenda template

  • Gabriel - I'll look through this and maybe submit some changes

  • ACTION - Gabriel - contribute to CONTRIBUTING.md

  • Lee: I've written this mostly myself, and I'd love suggestions on how to improve it.

  • Vince: I think having a meeting of the champions is a great idea.

  • Vince: note that more than one person can put themselves down as a champion for an idea. That's a bit of a signal. Would you agree Lee?

  • Lee: I think you're right; but the role of a champion is someone who's willing to do the work on something; if they're communicating well then it may be fine, but it's easy to get into the situation where each person is expecting the other to do the work.

  • Lee: certainly you shouldn't take the sign that someone else is the champion for blocking you from helping.

  • Vince: lets give a bit of time to get everyone to get their PRs in, then have a meeting in 2 weeks?

  • Lee: maybe 3 weeks - 1 week before the next week

  • ACTION - Vince to open an issue to discuss when we'll have it exactly.

Move "Add deprecated directive to arguments and input values" to stage2 RFC graphql-js RFC (10m, Ivan)

  • In a nutshell: we can deprecate fields and enum values, but not fields within input types or arguments.
  • A year ago we agreed to fastforward this. I contacted the original author and he raised a GraphQL Spec and GraphQL.js PR. There's a couple things missing from the GraphQL.js PR
  • Should we forbid deprecating required arguments/input fields?
  • I don't want to add additional restrictions on directives. For me I don't see any big issue to deprecating a required argument.
  • I don't mind if it doesn't go into GraphQL 2020 but I'd like to merge it into GraphQL.js.
  • Lee: the PRs are not in a state where they're ready for Stage 2 yet. The Spec PR is a bit of a mess.
  • Ivan: I think went something went wrong on the branch, I can rebase it - I reviewed it before and it was in a good shape.
  • Lee: suggestion - stage 1 today and raise it to stage 2 offline if these are in a good shape.
  • RESOLUTION (Lee) - moved to stage 1
  • ACTION - Ivan - get these PRs into a good state

@defer/@stream (30m, Rob/Liliana/Jafar/Kewei)

  • Batched Responses
  • If the server dumps a bunch of patches really quickly it could cause a client to re-render really frequently negating the gains of defer
  • Debouncing on the client is a simple solution, but it's hard to figure out what the right number is to debounce by, and the debounce adds latency.
  • Server could debounce; we should put in the spec there's these two different options.
  • Server should only do this if it understands how the data is coming in, otherwise you get the same problems as on the client.
  • Server can treat @defer and @stream as a hint.
  • Lee: to confirm
    • Whether @defer / @stream are rules vs hints.
      • Personal opinion: yes, server should be able to decide this.
    • What's the actual response shape?
      • Is it a stream?
      • Can multiple items happen simultaneously?
      • The main task here is documenting the stream and response format.
  • Jafar: is there any value on being able to configure a server debounce, e.g. @stream(debounce: ...). Can we get away without (adding this to the protocol)?
  • Lee: If your server is naively streaming things and they're tightly packed together then performance can be limited by react re-renders. This is a specific problem for React, for other kinds of clients it might be less of an issue. Other iOS clients I've worked with have streamed GraphQL into the database and then rendered those back out separately which works around this issue.
  • Lee: the server needs some flexibility to deliver things in the right order. It needs to do both what's best for it whilst also keeping in mind the clients' needs and restrictions.
  • Lee: we tried a client debounce for the React rendering problem - the performance was about the same or worse, latency was worse even though the CPU churn was less.
  • Jafar - that's the context I was looking for there. The value in the server delivering batches is that the server has context the client does not, so letting the server batch if it wants too doesn't seem to have a downside.
  • Lee: at some point you get into the realm of trying to predict the future - and that's when you know you're getting into trouble!
  • Lee: I know there's some databases that support chunked streaming.
  • Lee: if our backend wants to give us 100 results 10 at a time we should send them 10 at a time to the client - certainly not 1 at a time.
  • Jafar: so we definitely want to standardise a batch format.
  • We don't have a batch format in Facebook right now, it's not been a significant issue for us though we have had some performance issues.
  • Kewei: we've not had a global debounce on the server side that factors in everything, we only have it for stream which just uses the chunking from the backend
  • It's an async iterator.
  • Jafar: so we don't currently have a batch format
  • Kewei: correct.
  • Jafar: so I'm leaning towards [adding a batch format] - does anyone have opinions?
  • Lee: subscriptions does not have a batch format - maybe it should? We should investiage if we can re-use the stream details from subscriptions for defer/stream. Batch might be a case that ends up being useful in both cases.
  • Evan: "last update wins" - we debounce and throw away intermediate responses
  • Jafar: this is for stream where we're returning all the records rather than subscriptions-style "last update wins"
  • Lee: subscriptions are designed for realtime data, defer/stream are not.
  • Jafar: one thing that might make it useful for the client to indicate is the page size - I might opt to tell the server to send batches that correspond to page sizes (based on the size of the screen). I'm reaching here
  • Lee: I don't think you are, there's historical precedent here. Facebooks News Feed hacks together a bunch of this and a lot of science went into this to figure out the optimal batch sizes. We learned that "3" was the magic number - because there's always space to render 3 feed items.
  • My first argument for @stream actually had an argument where you could specify the page sizes.
  • Evan: smooth scroller on Android works well for page sized chunks
  • Jafar: we control a lot of these values on the server side for configuration. We've converged on a server-side configuration model - because data delivery is hard to get right, so we wanted to take the decisions out of the GraphQL query so we can change them without updating the apps.
  • Kewei: to add more context, a lot of the time you cannot find a static value that's always correct - it might be 2 around peak time to render a minimal amount of stories to everyone, but non-peak you might want to return batches of 10 because the servers are under less load.
  • Jafar: to summarise: before we add things to the directives, we should figure out if we can find ways to associate directives with a label (or another approach we've done is to rewrite GraphQL parameters on the server side) - the headlines is that there's terrific benefits in the client specifying WHAT data, but there's terrific benefits in letting the server decide HOW to deliver the data.
  • Lee: treating @stream/@defer as hints seem to be emerging as the right way to do this - it allows for both smart and naive servers.
  • Jafar: adding labels to any directive that influences HOW data is delivered, then you can associate configurations on the server side with these labels. This suggests that WHAT / HOW is a good separation - arguments change WHAT, directives control HOW. (This isn't what we do currently, but maybe we should.) This may not work so well for open APIs like GitHubs vs closed like Facebook.
  • We can also hint to the server which parameters can be overridden.
  • Rob / Liliana: we agree it should be a hint; we've worked on an implementation in GraphQL.js where if it's not a promise we shouldn't even attempt to defer. For batching it's pretty easy to implement.
  • Jafar: @defer(enabled: $var) allows the client to turn it on/off. Lets the server do experimentation by toggling the variables. Makes it so the server can decide to ignore @defer.
  • Rob: we found having an if on @defer was useful - on the server we don't want @defer but on the client we do - better for SSR.
  • Lee: to summarise: "stream and defer likely need some arguments (we're not sure how many yet); notably "if" which toggles it on/off, also an opaque label, maybe a chunk size". We also need to figure out a batching model.
  • ACTION - Kewei - "chunk iterable" at Facebook - can you talk about that outside of the context of Hack?
  • Lee: did I miss anything?
  • Liliana - we want defer independent of its arguments to act like a hint.
  • Lee: yeah, absolutely! It could be the entire thing is a hint, or each of the arguments are also hints.
  • Kewei: we want to introduce these variables, but we don't want to detail in the spec that this will guarantee anything.
  • Lee: the general rule of the server being able to override seems important, but we're not sure of the specifics of this yet. Clients have to opt in, but server can opt-out.
  • ACTION - Jafar - explore a strawman around this regarding client versus server control
  • Kewei: one more missing piece - should we try and specify the batch format?
  • Lee: I suggest you look at how subscription streams are specified in the spec (they don't rely on JS or JSON or iterables, they're very abstract) and the goal would be to write something equivalently extract that handles all the scenarios that we need it to handle. The spec is 20% details and 80% examples. Describing the chunked iterable in spec terms instead of Hack terms could be a really good first step.
  • ACTION - Kewei - have a look at how to describe chunked iterable in a more language agnostic way.
  • Jafar - it might be that we just need a different payload format.
  • Lee - agreed, let's abstract it down and figure out how much of it needs to be in there vs not.
  • Kewei - I think it'd be good to have a small sub-workgroup on this to discuss before the next working group.
  • ACTION - Jafar - post issue for people to stay involved/have a break-out session

Any other business

Action: Lee to add recording to agenda template (both so we remember and for legal consent purposes)?