Watch the replay: GraphQL Working Group Meetings on YouTube
- Agree to Membership Agreement, Participation 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)
- Update on next version of GraphQL specification (5m, Lee)
- Schema Coordinates update (5m, Lee, Mark)
- Looking for support in advancing from Draft to Accepted.
- Spec PR
- Implementation PR
- Usage for error messages PR
- Full unicode support (5m, Lee, Andi)
- Looking for support in advancing this iteration from Proposal to Draft.
- Spec PR
- Pending Implementation PR
- Fragment Arguments RFC (10m, Matt)
- @defer/@stream Updates (5m, Rob)
- Roundtable discussion on @defer/@stream (15m, Brian Kim)
- Benjie
- Mike Cohen
- Ready for review
- All open action items
- #581 Lee: We will talk about this today so this can be closed.
- #693 Ivan: GraphQL.js was frozen but is now unfrozen and in TypeScript. No major interruptions expected, though still some cleanup required.
- If anyone needs help updating their PR to TS - let us know
- Huge shout out to Sahid
- #638 List of topic to cover in GraphQL Spec Release Marketing.
- Leaving open for Brian to pull out highlights
- [ACTION - Brian] (use existing issue)
- #505 Require argument uniqueness
- Ivan: now that I’m unblocked with typescript migration I will try to do it before next WG
- #584 Editorial review of no root introspection of subscription RFC: complete; closed.
- #509 Read over GraphQL Custom Scalar PR: done; closed.
- Milestone
- Cut PR
- Lee: all tasks completed; the last one was to write a change log. I've opened a PR for this for a TSC vote. We'll probably force a vote in the next couple of days because it's been open for a couple weeks.
- I've built a script to generate this which we can use next time too.
- Ivan: we have 80 character limit on diff of spec; it makes a lot of text changes appear even when only one word or character was changed.
- Benjie: when I reviewed this I used a special diff command that ignored all whitespace. I also considered that we can format with prettier to change the line wrapping so that wrapping differences don't show up.
- Lee: Let’s open an issue as a place for discussion so we can hash this out.
- Matt: Should this block the release of the spec?
- Lee: I’m not going to block the release for this.
- Lee: only issue is Benjie's prettier PR: we might want to decide on this before adopting
- Benjie: I don't think we need to make a decision here, we can change it later if we want (and diff things retroactively by reformatting).
- Looking for support in advancing from Draft to Accepted.
- Spec PR
- Implementation PR
- Usage for error messages PR
- Lee: I’ve been working on this to get it from draft to accepted.
- Lee: review the PR to see if it is doing everything we want it to; then we can find out if there's any resistance to promoting it to Approved.
- [ACTION - everyone]: review the Schema Coordinates PR and implementation
- Ivan: Hesitations over introspection fields:
__typename,__schema, etc. Should these fields be handled specially?__typenamecan only be present on output types not inputs. Benjie pointed out a while ago that the error path indicates field name but doesn't indicate the type (e.g. if it was an interface or union). - Lee: spec text reads that meta fields do not have schema coordinates. "A schema coordinate must not refer to a meta field." If that's reasonable opposition to approve this then we can hold it.
- Ivan: I've not read the spec text, but I think we need to consider usa cases outside of error messages. Would
__typenamecause an issue for this? - Ivan: other than default values, we prefer structural data throughout the spec; we don't have structural representation for schema coordinates. E.g. error messages attaching strings to type. If you have argument and you want to know what type it belongs to you need to do parsing.
- Lee: let's move this discussion offline, I've not considered a non-string use case for this but it warrants more discussion.
- Lee: leaving this as draft (not approved) until we've resolved this discussion.
- Ivan: if anyone else has ideas on how to use Schema Coordinates (e.g. what can we improve by using schema coordinates?) please share your thoughts as it will help the proposal.
- Mike: where's the best place to leave this feedback? RFC?
- Lee: yes. Either of the two PRs are reasonable places for feedback.
- Looking for support in advancing this iteration from Proposal to Draft.
- Spec PR
- Pending Implementation PR
- Lee: gathered some feedback from people I've worked with who are unicode experts and made some changes to the RFC. Some details:
- Changed the "Source Characters" to be "any unicode scalar value" - anything that could potentially be a character. Previously we forbid the ASCII control characters (but no the non-ASCII control characters); so I'm proposing getting rid of that.
- Clarify semantics for how escape sequences can be used. Legacy 4 digit unicode escape syntax used by Java and JavaScript. Many of the languages (Java, JavaScript) already use this and we should make sure this pattern doesn't break so I've documented this in the PR.
- The implementation PR may be an easier way to review this. Read code first then spec.
- Lee: if everyone's feeling okay with where this is heading I'd like to move this to Draft rather than Proposal.
- Ivan: everything looks good to me; there's some complexity in escape sequences but it's expected because programming languages implement them.
- Lee: that's one of the remaining questions why I'm not suggesting jumping straight to Approved. Unicode experts have frowned on it, but I want to avoid breaking people.
- Michael: so the main change of this is that we allow UTF16 code points? The query logic is still in ASCII, so only comments and string values can contain unicode? Emoji's can be in there using unicode codepoints?
- Lee: yes: either via code points or via escape sequences or via surrogate pair escape sequences. I'm proposing adding legacy support for this, but advising that when you're printing it you print it directly not with surrogate pairs.
- Lee: my biggest concern was are we handling the source characters correctly? I'm pretty confident now, but we went through a lot of changes over the last few months.
- Lee: I'm confident this is in good draft state.
- Ivan: I'll raise some questions in the spec PR. Just to clarify are we tied to a particular unicode version, or is future unicode stuff supported?
- Lee: it's not tied to a specific version, same as most languages, Unicode is designed to be forward-compatible.
- Lee: a query variable is a Name in the same way that a field name is a Name.
- Ivan: I mean like a String value - if I send surrogate pair inside as a literal; that'll be transcoded into proper code. What if I send it via variables, what happens to the surrogate pairs there? Potentially people can inline arguments (e.g. simplification of queries), so we need feature parity with query variables.
- Lee: I agree we should generally have the same. The Unicode rules we apply to the grammar do not apply to values that come in in the same way (because the Spec doesn't have purview over them). This spec change hopefully clarifies some of this. As long as your internal string can represent the value then you're good to go. We'll take a careful look at the text to make sure that we repeat this point wherever needed, e.g. in the serialization section.
- [ACTION - Lee] - ^
- Michael: shouldn't be too much of a problem to implement, just a re-working of the lexer.
- NOTE: I created a GitHub group called "implemeters" and have invited a chunk of people to it (some people who attend the WG, some who do not) who maintain GraphQL implementations in various languages.
- Andi and I have been working on it together (Java and JavaScript) but both use UTF16 which means we have to convert codepoints to code units on the fly; hopefully most languages don't have to do that.
- Michael: we have the same UTF16 string, but we parse the UTF8 directly on the byte stream so it doesn't expand the memory usage.
- [ACTION - Lee] Fix visibility of new implementers team.
- Ivan: suggest we create a do in the GraphQL WG repo so interested people can add themselves to it
- Lee: good idea - good action
- [ACTION - Lee] Create document for interested people to add themselves to.
Fragment Arguments RFC (10m, Matt)
- Pending Implementation PR
- Matt: I took the feedback from the last working group that maybe we should treat fragment arguments to be as part of the implementation. So I've added a step in GraphQL.js so that when we hit the fragment spread that defines arguments we go through and match the relevant variables. It might not be performant, but I've opened it up on GraphQL.js but it's turned off by default.
- Matt: the harder part will be defining the validation rules to provide good errors because all variables used to be defined at the operation root or arguments at the field level; however now we have arguments whose type is defined in the executable document itself. Weird thing to resolve with the current validation setup.
- Matt: RFC blocker is getting the validation rules up for the GraphQL.js implementation. After that, not certain what the next best steps will be.
- Lee: I have a hunch that validation will be the bulk of the work and will inform the executor - may let you do something more performant for example.
- Matt: one validation rule that might simplify the executor is only allowing constant arguments to be supplied to the fragments.
- Matt: I'm trying to prevent implementations having to create an argument/variable stack. By making it so that you can only use variables defined as fragment arguments in the same fragment then the collectfields scope is extremely limited and means we don't have to pass the stack down.
- Ivan: can we require that if you specify variables on a fragment then all downstream fragments can only reference fragments passed down explicitly?
- Matt: that was one of the options in the original issue (posted 2016); now that we have the giant corpus of actual GraphQL that would mean only new queries could use it - we couldn't really add a fragment with arguments that calls a fragment without arguments. This spec RFC is preserving Relay's @arguments / @argumentDefinitions directives. We allow the fragment variables and operation variables to be intermixed at will.
- Ivan: so if you explicitly define a fragment with variables you can still use the operation variables too?
- Matt: yes. That's definitely a discussion point though; we should discuss the pros/cons on that RFC.
- Lee: great progress - nice work!
- Lee: Next steps: continue work on validation/execution. The PR that you have now is added but disabled; but I'd love to see it go through the whole process. I reviewed it before the meeting and was surprised it was disabled.
- Matt: is anyone else interested in pushing this along? There's 4-5 validation rules that need writing and I'd love to have some help on this. It's probably Ivan's purview, but I don't know if it's in a state where we should add it in a branch on GraphQL.js to allow people to send PRs directly or if they should send to my fork.
- Lee: I suggest that the PR is open to GraphQL.js and allow others to contribute to your PR. Start building a test suite and use that to gut-check the validation rules.
- Ivan: I requested the rule to forbid during validation because people will start using this feature; if we change something later it's important the user explicitly agreed to enable this experimental feature, rather than implicitly. Maybe instead of a branch we can merge it as is but make it clear that it's experimental. I'm against flags; it complicates code and makes everything harder.
- Matt: makes sense to me.
- Lee: I anticipate the default value coercion stuff colliding in potentially interesting ways.
- GraphQL-JS PR
- Spec PR
- Rob: We've been getting feedback, nothing major mostly bugfixes. Ivan's going to publish an experimental version soon. I'm waiting for 16 to be released so we can start getting stuff merged ready for the v17 alphas.
- Lee: nice work!
- apollographql/apollo-client#7691
- Brian: for GraphQL.js it's going to be in v17? That's the plan? (Not 16?)
- Ivan: we have separate GraphQL.js working group that Uri helped set up. Video of previous discussion with Rob is on YouTube. Currently we have stable main; preparing for new release is stressful/hard, so after the 16.0.0 TypeScript conversion we're going to switch to an unstable main that we can publish alphas from automatically after PR merges. Then cut major releases once or twice a year to avoid breaking the ecosystem too frequently.
- Plan is to release 16.0.0, wait a few weeks for feedback, and then switch to unstable main and release 17.0.0-alpha.1. Maybe 5-6 weeks to get @stream/@defer merged into main.
- Brian: I'm excited to see this getting merged into GraphQL.js. I have feedback that maybe I should raise against the PR - I always feel like a Promise of an AsyncIterator is an anti pattern. (
Promise<AsyncIterator>) - Brian: I've been working on @defer for Apollo client; I have some questions about the spec in an Apollo issue. I don't want to be the spokes person for Apollo (these are my personal opinions). I have questions like:
- for the patches as they come in, incrementally, is there a topological order? Do I have to treat it like a mkdirp recursively creating objects?
- Rob: I'm almost positive that what the path leads to should be an existing object or an array that the results would be inserted into at the position. The path cannot go deeper than a previous result.
- Brian: is it implemented in the spec yet?
- Michael: we've implemented it this way too. I don't think it could be any other way because of the way that the GraphQL executes adding the deferred tasks... Maybe if you parallelise the tasks you might have this issue, but we serialize them so that we get the best throughput.
- Yaacov: it's not explicitly in the spec, but it's implicitly.
- Michael: I think it's implicit in the algorithm, but it would not hurt to put a note in
- Brian: What is the relationship between incremental delivery and patching.
- multiple patches could come in at the same time
- Michael: Incremental delivery is part of the HTTP spec. We're using the same spec for batching because it talks about how to incrementally deliver content. In the HTTP spec repo there's a PR that talks about the transport for incremental delivery and I think it covers @defer/@stream.
- Brian: in Apollo's batching we send an array of operations and receive an array back; is that not what batching is any more?
- Michael: we're currently trying to spec the transport. It's not finished yet; but we have examples from various implementations including Apollo, but it feels logical to have incremental delivery do the transport for these things.
- Brian: is there a multipart/mixed library yet?
- Michael I worked with Marias on this.
- Rob: I have one that works with XHR that falls back. Both are linked in the GraphQL-over-HTTP repo
- Brian: the decision to make @defer work only on fragments: is it an arbitrary limitation?
- Michael: it's very deliberate. Putting it on single fields leads easily to people deferring a lot of separate fields which results in really bad performance - deferring very small fields lots of times. With the fragment it makes it clear you're deferring the block. It is an artificial limitation, but it makes it more clear and indicates that it's expensive.
- The second thing is that in Relay we have a label approach which this fits better.
- Rob: someone else on the Relay team detailed another reason why it's more complex if we allow both fragments and fields to be deferred.
- Michael: from a backend POV it makes it better for the server because it puts a logical boundary around things.
- It's the first thing people ask when they see the PR.
- Michael: we analyse the query plan of our execution first and if only the scalars are deferred then we don't bother to actually defer.
- Brian: the mobile developers are struggling with this where the @defer is optional. Kotlin. Apollo Android. Code generation. How should defer/stream work with code generation / type safety.
- Michael: .NET is also strongly typed so we have to generate everything in advance. We use the labels to mark everything in the graph that we generate. We use a reactive way to interact with the data. Others may do it in a different way.
- Matt: we've had fairly significant problems with this; the reason was our native codegen uses type models so we can't tell whether it's deferred or not. One solution is to really rely on subscriptions - provide a callback that may be called an arbitrary number of times. Heavily accept the data might be updated 14 times and I have to be okay with that.
- Matt: another thing that helped a tonne is that we transitions from type models to request models (which still has the same model) and now to fragment models. If you have strong boundaries for your fragments then it works much more obviously.
- Yaacov: have a question about hasNext vs isFinal
- Michael: the reason is that if we have isFinal we have to put it on every response to mark it as not final. So we flipped it so you can send the response is when there's nothing else, and only add hasNext when it's a stream with multiple parts.
- Matt: Facebook implemented with isFinal and I ran into a bug where a client that expected isFinal was used against a GraphQL server that doesn't support isFinal.
- Yaacov: is hasNext the right term?
- Matt: it's not
hasMoreDatait's "has next response"
- Brian: are there ways to prevent @stream being misused like we do for @defer with fragments?
- We could use it for pagination or for subscriptions - can we do anything to prevent it being used
- Rob: it's called out in the RFC that it's not intended for long-lived connections, it's more about getting some of the data sooner rather than fetching data that may exist later.
- Lee: we definitely need to make this clear because the two use cases have very different use cases, e.g. hanging HTTP has problems.
- Michael: the misuse would be by the backend engineer rather than the frontend engineer.
- Ivan: distinction is pretty clear - subscription is event based, but stream is data based.
- Benjie: Should stream/defer be supported on subscriptions?
- Rob: I do have it spec’d out for mutations and subscriptions. Won’t get the next event until the last payload for the previous event is received.
- Rob: The point of defer is to get part of the payload sooner, not to make the entire request faster.
- Benjie: Interesting that you’ve implemented it for mutations. Has there been any pushback?
- Rob: No, and I think it makes sense. Especially if you’re using Relay and are using fragments.
- Each mutation only waits for the initial payload of the previous one. All other payloads get added to the dispatcher queue and are solved asynchronously.
- Michael: this seems like it could cause issues e.g. returning the change state of the third mutation on the deferred result of the first mutation.
- Matt: the most logical resolution of that to me seems like the server could choose to implement it so that it ignores the defer under the first mutation field but allows it on later ones. Even an implementation of defer within mutations that is "ignore the defer" is better than not allowing them.
- Michael: I agree, and that's what we do - ignore the defer. My concern is that the spec should make the behaviour/expectations more obvious.
- Brian: have you considered how this would work with a normalized or entity based cache? Is it last write wins?
- Matt: Relay is a normalized cache and implements defer
- Brian: what happens if you have two operations where operation 2 starts later but operation 1 completes later?
- Matt: it wouldn't matter for a query because a query is read only and we already have to deal with resolvers reading data that was updated after parent data was read.
- Brian: do you worry about tearing?
- Matt: you have to normalize with the variables.
- Brian: how do errors work with incremental delivery; errors can have paths - can errors come in before the deferred fragment comes in?
- Michael: each response is a complete GraphQL response with or without errors. E.g. if a non-null violation occurs you'd receive the data and errors
- Brian: Is the path relative?
- Michael: no, we apply using the path on the existing tree that's built up so the full path in the error will make sense against this.
- Brian: is this all reflected in the spec?
- Yes.
- We could add more non-normative notes, please raise these against the pull request.
- Benjie: My gut says that we should wait for the deferred action on one mutation payload to complete before executing the next mutation.