Skip to content

Latest commit

 

History

History
263 lines (204 loc) · 24.7 KB

2020-02-06.md

File metadata and controls

263 lines (204 loc) · 24.7 KB
Raw

GraphQL WG Notes - Feb 2020

Video recording: https://www.youtube.com/watch?v=a0xwVVhH6rI

Agenda:

  1. Introduction of attendees (5m, Lee)
  2. Agree to Specification Membership Agreement and Code of Conduct (1m, Lee)
  3. Determine volunteers for note taking (2m, Lee)
  4. Review agenda (2m, Lee)
  5. Review previous meeting's action items? (Skipped due to agenda coverage) (5m, Lee)
  6. Update on Spec freeze and 2020 release schedule (5m, Lee)
  7. Custom Scalar Specification URIs Update (10m, Matt Farmer)
  8. Discuss deprecation of input fields. There was an older PR (graphql/graphql-spec/pull/525) which seems abandoned. Are there any blocker to move this forward? (15min, Andi)
  9. Discuss full unicode support. There was a PR (graphql/graphql-spec/pull/231) which was never merged. GraphQL Java and GraphQL Ruby and probably more implementation support full unicode support already. What are the challenges here? (15min, Andi)
  10. Discuss backwards compatible introspection: the spec is about to add two new introspection fields (isRepeatable and specifiedBy) to Introspection. Together with description it becomes increasingly difficult for clients to query the full Introspection data. Should there be a more general solution? (15min, Andi)
  11. Discuss @defer and @stream strawman proposal and questions in the comment (30m, Kewei, Jafar, Jennifer)
  12. I'd like to get an idea of what people want from the website redesign (10m, Orta)

Live notes:

Introduction of attendees (5m, Lee)

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

Determine volunteers for note taking (2m, Lee)

Review agenda (2m, Lee)

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

  • (Skipped due to agenda coverage)

Update on Spec freeze and 2020 release schedule (5m, Lee)

  • Lee: Proposal: end of today is the freeze for the spec.
  • Lee: Over the remainder of this month, everyone who’s interested should comb through the draft and look for irregularities
  • ACTION - everyone: look through the spec and look for irregularities (bugs, spelling mistakes, etc)
  • Lee: Bryan, our program manager will make sure whoever has contributed to the spec has the proper legal documents
  • ACTION - Bryan: make sure everyone who has contributed has the proper legal documents
  • Lee: I'll be writing up the release notes - particularly how this release is different to previous releases
  • Michael: review will be easier if we can tag what's changed
  • Lee: That’s a good suggestion. Have a bullet list, a high level explainer of the important changes. Different forms of release notes for different purposes.
  • Lee: We can tag all the merged PRs as Spec2020
  • Ivan: last time we released the spec, Lee asked people to write articles that we could link to, but no-one did this. If someone wants to work on this article they can start now after feature freeze and we can reference this article.
  • Lee: That’s a great suggestion. An important part is a press plan.
  • Lee: I'm happy to review anything, help with editing, or anything else that people need help with
  • Andi: I might write a blogpost about the interface hierarchy.
  • Lee: Mike has also written one
  • Andi: I am interested to know what happens after the freeze.
  • Lee: disallow non-breaking change is in the approved/accepted state - I plan to merge that today after I review it one more time. The reference implementation has been in place for months if not a year. Custom scalar spec is not quite ready yet; missing the cut is not a big deal - all of the reference implementations track the draft rather than a specific version of the spec.
  • Ivan: does this affect graphql-js? Can we keep merging stage 2 features?
  • Lee: graphql-js should keep on tracking draft
  • Lee: What the freeze means from a tactical standpoint is that PR outside of the RFC folder will not be merged for a couple of weeks.
  • Lee: The only thing it might affect is custom scalars, which will just delay it for a few weeks.

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

  • 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.
  • Seeking validation on how to handle use-case where a scalar is extended multiple times, which results in the directive being included in the typeExtensionsMap multiple times. Currently taking the last element.
  • Custom Scalar Specification URIs RFC Issue
  • Custom Scalar Specification URIs RFC PR
  • GraphQL.js PR
  • Matt: waiting on GraphQL 15 to be cut - on hold because of this.
  • Matt: the specified directive should only be present once; however if you extend a type then the directive might be used multiple times which would be invalid - how do we handle this - should we just take the last one?
  • Lee: My understanding is that the spec text forbids that.
  • Evan: Yeah, I believe it would be invalid at that point. If it is trying to override an existing directive.
  • Matt: so is that a bug in the GraphQL.js implementation?
  • Ivan: I think you added a test for this. It should be validated; I probably only need to check one thing - if you create the schema programmatically but then extend it with SDL.
  • Matt: in the agenda notes, there’s are two test cases and it sounds like the second one should have thrown an error but currently it doesn’t
  • ACTION - Matt/Ivan: check if this extra check is required, or if there is a bug.
  • Ivan: I encourage everybody to check the spec because we changed the wording “specified” to “specified by” (a number of similar changes)
  • ACTION - everyone: check the spec and implementation to make sure you're happy with these changes.
  • Lee: Matt, it sounds like you exactly have the right framing. The test case looks incorrect to me as well. The second one should cause a validation error.
  • Matt: My initial thought is that we should separate this into two different issues.
  • Ivan: I’ll try to review it. It is probably a bug. We added this validation pretty recently, a bit over a year ago, so there might be a bug in there.
  • Lee: I agree, Matt. We should consider treating this as a separate issue.
  • Andi: I started implementing it in GraphQL.js but we should provide guidance from an implementation point of view in the case that there are conflicting directives. For example, if someone updates and there is a new default directive, what should he/she do? This is a bit of an edge case.
  • Lee: This is kind of a grey area. We don’t describe how to be compliant when we are not compliant.
  • Andi: What should make a call one way or another. Nobody is intentionally against it. It’s just an edge case.
  • Lee: Technically it’s spec breaking. It is illegal.
  • Andi: Then we should just clarify that case.
  • Evan: it's implicitly spec breaking because you cannot have two directives with the same name, and the built in one is automatically included.
  • Michael: For example, the deprecated directive, it is a part of the SDL.
  • Andi: the spec states that you should include the deprecated directive.
  • Andi: we should state that it's forbidden to declare the directive "@specifiedBy"
  • Lee: I think that is a good suggestion. One thing we can do is make the spec more clear. We do this for scalar types. For example, we explicitly say that you cannot supply your own String type that behaves in a different way.
  • The intent was to do the same thing for the directives, but we were a little more terse with directives than we needed to be. See: http://spec.graphql.org/draft/#note-29c86
  • Ivan [chat]: http://spec.graphql.org/draft/#sel-FAHnBXFCAACCbvwP
  • Andi: I just think that we are introducing a new directive, that this issue is more apparent.
  • Pascal: I remember there was a best practice for prefixing directives.
  • Lee: It’s related but it’s not exactly what Andi is talking about.
  • Pascal: The thing is you have to prefix all directives. It feels weird that the custom directives have to be prefixed (because there are a lot more custom directives than built in)
  • Ivan: I'm not sure we need to add anything right now; but I kind of agree that forcing everyone to prefix all their custom directives is undesirable.
  • Lee: Yeah, that’s why this is a non-normative suggestion and not a rule. I think either stopping people from starting up their GraphQL server or allowing the override and continue to chug along the best we can are both possible routes. We just need to be clear about it.
  • Andi: we should be explicit in the spec about this
  • Lee: agreed; I will be adding this change
  • ACTION - Lee: add a change to the spec making this clear
  • jhusain: has this issue been considered in the context of namespaces before. I searched for it, and heard about Facebook talking about using namespaces in 2017 but couldn't find conclusions - has this been considered recently?
  • Lee: I think it has not been considered recently. There was discussion about this in 2017-2018. It can be kind of tricky to track with different namespaces. There was an idea to have all the built in types to be in their own namespace. You always need to resolve to the correct namespace and that ended up being very tricky. It just introduced more oddities. However, I don’t think this means that namespaces solution is a deadend.
  • Lee: "It looked like a solution looking for a problem."
  • jhusain: can you point us to these specific issues/places where this was discussed?
  • Lee: I think this was all discussed live, so check the notes. I believe it was also discussed in the GraphQL Spec issues and the GraphQL.js issues on GitHub.

Discuss deprecation of input fields. (15min, Andi)

  • There was an older PR (graphql/graphql-spec/pull/525) which seems abandoned. Are there any blocker to move this forward?
  • Andi: Deprecating arguments and input fields happens in the wild, it feels like a missing piece of GraphQL.
  • Ivan: I reviewed it today, and remembered the entire discussion. Someone created the PR for spec repo, but never attended the working group, we discussed it without him. Agreement was that after GraphQL.js PR is ready it can be fast-forwarded. It's my fault; solution is to ask the original author if they want to move it forward, if not then someone else can take over.
  • I reviewed it today; the only thing missing is the introspection part - we might need to add an extra flag. Actually... we might need two stage introspection for this.
  • ACTION - Ivan/Andi: ping the original author.
  • Andi: I’m happy to help out if the GraphQL.js is the only thing missing from the PR.
  • Lee: sounds good to me; it just needs a champion.

Discuss full unicode support. (15min, Andi)

  • There was a PR (graphql/graphql-spec/pull/231) which was never merged. GraphQL Java and GraphQL Ruby and probably more implementation support full unicode support already. What are the challenges here?
  • Andi: This is more in the realm of hygiene. Currently, there is a restriction to the Basic Multilingual Plane - covers all written communications but doesn’t cover emojis for example. In real life, our implementation allows all unicode in descriptions.
  • Andi: Lee opened a PR quite early (like 4 years ago) and there was discussion but then it was abandoned.
  • Lee: I’ll talk about it because it is my PR. I think I abandoned it because I didn’t have the time or context to take it to a conclusion. There were a lot of specific edge cases and I didn’t have confidence that the change I was trying to make was the right one. There are no objections to multilingual planes. However, the question is what happens if you are in an environment that doesn’t support multilingual planes.
  • Lee: JS historically had really bad support for this (but it's got better now); this was one of the reasons we got stuck.
  • Michael: .NET has recently added support for UTF8 JSON (previously we could only do it in UTF16); this is great for performance and compatibility.
  • Lee: That’s a good point. So there might be performance implications for supporting multilingual planes.
  • Andi: unicode stuff is similar to timezone stuff - edge cases get tricky and the common understanding of edge-cases is not there. I'm not a Unicode expert. One thing in the Unicode spec is that there are two different things - what unicode represents and how to encode these things. The [GraphQL] spec could be more more clear that these concerns live outside of our spec.
  • Andi: question: do we want to allow escape strings with more than 4 digits for codepoints outside of the BNP? This seems like the most critical thing. There could be performance implications if a system does not support it well.
  • Michael: in .NET a string is UTF16, which has space concerns if you're reading in a UTF8 string. We had to do a lot of work around UTF8. You're inflating strings by default if you state that GraphQL strings have to be UTF8.
  • Andi: This is not what I’m talking about. The spec doesn’t tell you how to encode anything.
  • Michael: the spec states that the string is UTF8
  • Andi: No, the spec does not say that it has to be in UTF8
  • Lee: This is just wrong. What Andi is suggesting is correct. We should not say anything about the encoding in the spec. Andi, if you want to champion this issue, here are some ideas:
  • ACTION - Andi: split this into separate pieces so that we can think about the implications of each. 1. comb through the spec and make sure that we're consistent between "character" and "codepoint". Whenever we say "character" we mean "codepoint"; whenever we say "UTF8" we mean "Unicode". 2. Change what codepoints are allowed. It's weird that some GraphQL implementations are non-compliant because they support characters outside of the BNP. 3. Multi-plane escape sequences.
  • Jesse: the null character is problematic, e.g. in PostgreSQL the null character is not allowed. This is a problem for interoperability.
  • Andi: this is kind of a hygiene thing, I'm happy that we're going to try and address this.
  • Full unicode support would be wonderful.
  • Lee: I'm really happy you're looking into this.
  • Ivan: one small note, I submitted a project for GraphQL-cats to Google Summer of Code - it would be great to have Unicode tests in this so we get half a year to make sure that all implementations agree on this.

Discuss backwards compatible introspection (15min, Andi)

  • "the spec is about to add two new introspection fields (isRepeatable and ~~specifiedBy~~) to Introspection. Together with description it becomes increasingly difficult for clients to query the full Introspection data. Should there be a more general solution?"
  • Matt [chat] Minor correction: specifiedByURL is what is currently being proposed to be added (no longer specifiedBy)
  • Andi: Fundamental challenge is related to the introspection query. You have to specify every field you want. We added 3 new fields with the soon to be released spec. The description to the schema, the http to scalars, etc. There is a general challenge of how to evolve introspection moving forward.
  • Ivan [chat]: https://docs.google.com/presentation/d/1ISS5QhF6D2ncD8ZGChoCv1LnOWj9jryZrJ2dG11i4Mc/edit?usp=sharing
  • Michael: what we do is we do 2-phase introspection - first query checks for all of the optional things - e.g. asking for the subscription type. From the results of this we then generate the introspection query. So for 2 phase introspection the first phase may change over time.
  • Ivan: We discussed it in a previous WG. Lee initially suggested multi-stage introspection. I don’t want to add more experimental or breaking stuff. After 15.0.0 release, we’ll do proper 2-phase introspection.
  • Ivan: we're using flags for each of the features currently.
  • Lee: I wonder if there is anything in the future that can replace introspection. I don’t want to set things up so that this isn’t something we can explore in the future. We’ve been talking about 2-phase introspection for forever. If introspection is going to be core, then it’s going to be replicated for every language, and we should clarify how to do it in the spec. Add it as a chapter to the introspection chapter.
  • Lee: If anyone thinks that their implementation of 2-phase introspection is really bullet-proof and future-proof; lets start writing it up!
  • ACTION - anyone who has implemented 2 phase introspection: volunteer to be involved with writing this up
  • Ivan: just to confirm, this would be a non-normative note?
  • Lee: I don't know if there is anything normative or not. Probably non-normative? Lets not get hung up on the details of it; if we agree that it's valuable to write this up separately from source code lets start doing that.
  • Andi: I think introspection is extremely critical to the GraphQL ecosystem. Even if it is a non-normative note, it is something we should discuss.
  • Ivan: Actually, I like the flexibility of introspection. That’s why I’m partial to keeping it a non-normative note.
  • Ivan [chat]: graphql/graphql-spec#319
  • ACTION - Michael: have a go at writing this up
  • Lee: rejected RFCs don't feel great, but they are a really good way of documenting process to explain why we've not followed a particular path. I just want to make it clear that this could still be a useful thing to do.
  • Lee: We have 1-phase introspection already specified in the spec (though you can ignore it if you want to) - making sure this stays up to date is really important, and there's space in the spec to add details about 2-phase here.

Discuss @defer and @stream (30m, Kewei, Jafar, Jennifer)

  • Strawman proposal and questions in the comment
  • Kewei: I submitted a PR that added content to Liana’s original RFC. Possible changes to improve latency, the difference between subscriptions and defer stream.
  • Kewei: Defer/stream are NOT to represent real-time changes.
  • Kewei: I have a lot of questions about what to include in the refer/stream spec
  • Kewei: Defer is a low level paradigm. Execute this piece of code asynchronously. Something that is more high-level might be more valuable. For example, let’s say there is a piece of data that I will need in the future. The way the server can handle it could be synchronously or asynchronously.
  • Kewei: What do people think about tabling the @defer part in favour of a higher level abstraction in the future?
  • Lee: Successful RFCs first describe a problem space but yields much higher quality results. It sounds like you’re already thinking about this in a broader perspective.
  • Kewei: I think we should save @defer discussion for the future
  • Lee: I think it might make sense to separate defer and stream. The original pitch is stream would operate on arrays of things and defer would operate on singletons. There is a parallel in React concur list, it allows you to wait for things in ordered arrays.
  • Kewei: I’ll definitely capture my thoughts in the RFC more. Basically the concern is if we standardize defer now, it might restrict new features in the future.
  • Kewei: The second question is that at Facebook, when we experimented with defer and stream, we also added a lot of other features like batched payloads. I was wondering which of these features would be valuable to include in the spec.
  • Lee: That’s a good question. Some things to consider. Does doing one thing make it harder to do another thing? It’s not about whether things are simple or complex, what’s more important is describing ideas in detail. The behavior should be very well defined.
  • Kewei: I think a lot of these are more detailed questions.
  • Andi: I am interested in how you implemented your overall system. Just want to make sure the overall idea makes sense.
  • Michael: There was a good talk from Robert Zhu.
  • Kewei: It is an attempt to represent promises and futures like in other languages. The connection is very short lived. Everyone can implement their own async iterators that represent the data to be returned, and the system pulls from these. It is not realtime - we do not use pub/sub for it.
  • Lee: If you or anyone is interested in doing a tech talk or a blogpost, I think there would be a lot of avid readers.
  • Lee: If we had to write an execution spec text, what would that look like? This is going to be the hardest part of defer and stream.
  • Kewei: The interesting thing about implementing defer and stream is that the two behave very differently. Stream resolver returns an async iterator. With defer, we had to change the executor logic, this is asynchronous code, we’re not going to await for it, we are going to store the deferred piece of code in the queue.
  • Kewei: We have a lot of additional context that would be useful to the end user of the defer and stream. They wouldn’t be helpful for the implementers of defer and stream. It feels it wouldn’t be appropriate to put it in the spec but where should this information live?
  • Lee: I think we can consider putting it in the spec. The FAQ should be mixed in with the spec. That way the spec is more than a description about implementing GraphQL, it is also a way to learn about it.
  • Kewei: Okay, we can first try to include information in the spec and if it’s too much, we can move it elsewhere.
  • Kewei: Is the current RFC considered the strawman?
  • Lee: I think right now it is.
  • Ivan: I think so too.
  • Kewei and Liliana will champion this idea and will talk to each other
  • Lee: I think it makes sense to move this to stage 1.
  • Action - Lee: move to stage 1
  • Kewei: sounds good!

I'd like to get an idea of what people want from the website redesign (10m, Orta)

  • Orta is not here today
  • Action: Move to next wg’s agenda