Skip to content

Latest commit

 

History

History
203 lines (190 loc) · 22.8 KB

2021-09-02.md

File metadata and controls

203 lines (190 loc) · 22.8 KB

GraphQL WG Notes - September 2021

Watch the replay: GraphQL Working Group Meetings on YouTube

Agenda

Determine volunteers for note taking (1m, Lee)

  • Benjie

Review agenda (2m, Lee)

__fulfilled Meta Field RFC (10m, Matt)

  • Matt: Fragments are becoming more and more how client developers interact with GraphQL at Facebook. To access fields you have to access an accessor named based on the fragment in order to see those fields.
  • The __fulfilled field is designed to tell us whether a particular fragment has been retrieved or not so that we know whether clients are able to access that fragment or not.
  • There was discussion (on Discord) of using "keyed fragment spreads" in some form - this would match exactly with how we do fragment spreads at facebook.
  • The __fulfilled meta-field says that "this selection set that I'm on has been fulfilled"
  • Lee: it's a meta-field that always returns the value true.
  • Matt: a long-term vision of why we might want something like this is the rules required to inspect on an entire GraphQL document to find out if a fragment is valid in that document could be eliminated so you could know directly at the fragment spread level whether or not it's valid.
  • Michael: but if I add it in two selection sets I don't know which one is fulfilled.
  • Matt: yes... this RFC might be ahead of itself because maybe we want to allow canonical named fragments. Requiring a label for the fulfilled field would ensure that you know which fragment it was.
  • Michael: it'd work if you used an alias
  • Matt: yes, exactly.
  • Michael: it gives the client an easier way to see if the server has delivered on a certain fragment. You can test it in a complex way but it makes it simpler.
  • Ivan: it seems a bit too generic for the task at hand. If you do aliases on a field then maybe you can have a field that returns a list of strings - the field fragments. If you can nickname aliases, you can nickname fragments. I don't see this feature being useful for anything else.
  • Benjie: why can't you already do this with __typename and aliases?
  • Matt: you can; but if we start doing this in many many places then the cost of doing this in the response is relatively high. One reason to use __fulfilled rather than "keyed fragments" is that the fulfilled field is almost always smaller in terms of the overall response size because any duplicated fields in the response would not be duplicated whereas keyed fragments would be. With typename you can do it but it'd be more complex - you're asking "if this field exists" but you're getting a string back which feels wrong. This is actually what we're doing today though.
  • Lee: a field that's inexpensive to query [...]
  • Matt: one point is that the response doesn't have to be in JSON - it could be using flat buffer formats/etc. __fulfilled is trying to solve keeping a JSON response small whilst simulating keyed fragment responses. Given that I don't think this RFC has fully explored the solution space for the problem far enough. It's not ready to be pushed beyond a strawman.
  • Michael: are you already using this?
  • Matt: we're experimenting with a canonical response format (using a similar field), but clients use alias format. We've broken the response format in this experiment.
  • Lee: you want to understand which fragments were/were not included in execution of a query (also useful for @stream and @defer). How much is this useful for humans to add rather than tooling.
  • Matt: I expect 98% to be build tooling; and the 2% being people emulating build tooling.
  • Lee: we should optimise for the smallest response possible.
  • Matt: GraphQL not being a wire format spec leads me to believe that we should have an optimised GraphQL wire format spec somewhere. That'd give us more leeway to give us keyed fragment spreads - not optimised in the JSON response format, but the actual cost for optimised response would be low. A way to translate to/from the optimised response.
  • Michael: e.g. like flattening the JSON.
  • Matt: yeah, and there's probably a really large pool of potential solutions.
  • Lee: there are wire protocols/derivitives of JSON that let you do this kind of thing. E.g. representing a DAG vs a tree. I don't know that we have enough metadata to even produce something like that. Maybe we're missing something to be able to do that.
  • Matt: server could return data in the Relay store format that the data will be serialized to.
  • Stephen: normalized transport flat-buffer/thrift/etc - are there prior arts of people doing that in production, it seems like a natural thing that people would want to do but I've not seen it.
  • Matt: there was an effort at Facebook to use flat buffers around the time GraphQL was being open sourced; but you only start seeing results when you have incredibly deeply nested complex responses, but Facebooks gnarliest query was about 30 queries deep in the tree back then. It's significantly worse now. Unless you're hitting that scale having JSON with duplication isn't going to be a big deal.
  • Lee: I think gRPC and protocol buffers have smarts that help you represent a DAG.
  • Michael: is there a reference to the Relay store format.
  • Benjie: there's been some requests for adding strings/etc to response and maybe an __identity field that returns its argument back out again?
  • Lee: I like the idea of querying it as an identity function. Aliasing the fragment itself is also a cool idea - knowing if the field is in the response body or not.
  • Matt: this is exactly what we do for our clients right now; I just didn't realise that's what I was doing when I built the client.
  • Lee: speak some more about the label argument - what's the purpose?
  • Matt: the reason I included it is that in a world with canonical responses then response does not have aliases - even if you have an alias it's "my field with arguments", the label is a way to differentiate those fields as an alternative to aliases. In an alias-based world it doesn't have much value.
  • Lee: it's a runtime concept rather than a store concept - you want to ensure you don't lose information. The server won't need the alias because it's an alias-based world?
  • Matt: likely correct if we didn't have the label argument.
  • Matt: the other thing is that assuming we have this field does the naming make sense? Is there a better name for __fulfilled and/or label?
  • Lee: good question - I'm going to push for alternatives because it's hard to come up with a name that clearly indicates what's going on. You'll never get false but it says type is Boolean - this is inherently confusing from a semantics point of view.
  • Lee: in terms of intent, the names seem right. "What are the fragment that were fulfilled?" - makes sense.
  • Benjie: an obvious advantage of having label: String! as an argument is that it basically forces you to use aliases otherwise the fields will refuse to merge (due to different arguments).
  • Lee: good point, avoids footgun.
  • Lee: the problem is interesting - how do you know what fragments were included in the execution result. I think we need to explore alternative solutions.
  • Matt: agreed - the next step should potentially be more strawmans.
  • Stephen: The “Falcor JsonGraph” that I mentioned https://netflix.github.io/falcor/documentation/jsongraph.html :)
  • [18:41 GMT]

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

  • Ready for review
  • All open action items
  • [18:42 GMT]
  • #477 - GitHub infra for managing WGs
    • Lee: what was the context?
    • Ivan: it was related to Input Union and other RFCs - every time we need to give commit access there's pressure on people not to screw things up in spec repo. There was suggestion we move RFCs to WG repo and make it easier to obtain commit access there - change anything in the folder of your RFC. Should we limit how long they have commit rights/etc?
    • Michael: I remember this discussion; but maybe we should have a graphql-rfcs repo?
    • Ivan: I'd advise against adding more repos. RFC processes are inherently related to WG repos/agendas/etc, so I think they should live with WG. Also there's no real risk of messing things up on the WG repo.
    • Lee: thanks for the reminders; I've added notes to the issue itself. I'm going to move the RFC folder to the WG repo - having notes and RFCs together makes sense. That should allow us to add plenty more people with commit rights to that repo.
    • I'm going to add details to the README; we also need to overhaul the spec repo README now we have a website/etc.
    • [ACTION - Lee] - complete this.
  • #543 - Update agendas to recommend adding GitHub username
    • Lee: I will do this!
    • Lee: I'll just make my name a hyperlink; what do you think?
    • Benjie: prefer if you make an @leeb because it makes it easier to reference/copy/etc.
    • Lee: better idea
  • #488 - License missing in scalars subproject
    • Lee: who's maintaining this reproject?
    • Michael: Andi.
    • Michael: the problem with this project is it's not sufficiently promoted - it only has Datetime currently. Adding JSON scalars or similar would be valuable.
    • Morris: it doesn't even have the Datetime spec though does it?
    • Michael: graphql-scalars.com is the official one. There's another one the Guild are maintaining.
    • Ivan: we started a separate WG on this, there was a lot of bikeshedding but we got stuck on how to give proposals unique URLs - I was following unique numbers like RFCs, others have other ideas e.g. using schemes, etc. Andi and others gave the initial push but we lost it in the small details. Maybe it's worth having concrete goals like getting Datetime somehow.
    • Benjie: I think it should be scalars.graphql.org rather than graphql-scalars.com
    • Morris: we want to allow different companies to have their own subfolders and publish their own scalar paths.
    • Michael: there should be a central place owned by the GraphQL foundation; but not every scalar spec has to have the same quality as the GraphQL spec.
    • Morris: agree - each person can have their own directory under the main directory - it's a central place to look for them not an arbitrator of what's good/what's bad.
    • Michael: then tooling can support it. This is hindered because no-one knows about it - we reference it frequently but it doesn't have much there.
    • Lee: we'd like to breathe some life into this project but it's just a README - lets mark this issue as done and try and give it some maintenance love.
    • Lee: also note that the license for the repo does not have to be the same as the license for the individual scalar specs (like npm).
  • #477 - GitHub infrastructure for managing WGs
    • Ivan: Idea was to create GraphQL.js WG and make it easier to create more WGs, so we wanted a template repo for creating WGs. GraphQL-over-HTTP, GraphiQL, etc etc. This part is not done.
    • Lee: I think we can close it - we're not creating WGs often enough to justify this, seems overkill.
  • #590 - Options for alternative meeting times
    • Lee: seems every time we discuss this we find there's never a good time. We did consider adding a time that was suitable for EU/Asia but not US. I'd be happy with this but I won't organise it because it'll be midnight my time.
    • Ivan: it's late my time but it's manageable right now. It seems like we have the most optimal time right now without splitting into alternative times for different timezones.
    • Lee: closing for now.
  • #637 - Share stories related to GraphQL spec release
    • We wanted to release blog posts around the same time; but I think a waterfall approach would be better - I'll release my blog post this month with the spec release and then we can use the GraphQL account to boost the signal on blog posts where others want to write about these features.
  • #643 - Add note GraphQL Request !== HTTP request
    • Not done
    • [ACTION - Benjie] Raise initial definition PR?
  • #645 - Implementors - ensure Default Value Coercion Rules change is compatible
    • Benjie: this was related to the change in defaults - it seems that though the spec change is small the actual implementation change is large for some projects so we want to ensure that if it's not going to be possible then people need to let us know now.
    • Lee: Spawnia voiced support - it's challenging to implement but he's seen people bit by this.
    • Lee: Evan talked about it in the Ruby stack and was discussing with the maintainer but haven't heard anything since then.
    • Lee: I'm going to suggest closing this 6 month old issue - we did ping the implementors - I'll do one last ping on the actual change because I'm hoping to come back to this PR and get it ready to land.
    • Michael: I'll have a look at it and see what it means for us.
  • Only stuff open since March now!
  • #646 Add prose to explain why scalar fields must have same type when composing fragments
    • Will require deep dive
  • #648 - Implement oneof in GraphQL.js
    • Benjie: I've not got around to that yet
    • Lee: regarding advancing your RFC?
    • Benjie: yes
  • #661 - Should we apply to GSoC / GSoD
    • Regarding GSoC / GSoD
    • Assuming the deadline is long gone. Will bring this up with Brian later today, but will close it for now.
    • Ivan: just wanted to know what to do - do I apply or not? Since then I've not got time.
  • #662 - scan for outdated copyright notices
    • Brian suggests someone from Facebook should change the license. Seems unlikely Facebook'll sue GraphQL Foundation!
    • [ACTION - Lee] I'll follow up after the meeting.
  • [19:17 GMT]
  • #694 query level nullability
    • He's working on the JS implementation
  • #695 - Raise a GraphQL.js PR for preventing @skip/@include on subscriptions
    • Ivan: I can do this Benjie, ping me
    • [ACTION - Benjie] Ping Ivan about this

Relax rule on Field Selection Merging to allow merging covariant fields Spec PR (15m, Ivan)

  • [19:20 GMT]
  • graphql/graphql-spec#883
  • Apollo Federation suffers from this issue due to a transform; it was discovered by a client.
  • Michael: it's one of the most complex rules in the spec.
  • Ivan: indeed; I think people may have discovered it and just looked for a workaround.
  • Michael: Sacha from Twitter wants to relax the rules and this would simplify this part of the spec. Lee explained why they made it so strict, but with clients now using __typename so much it's not so important.
  • [19:27 GMT]
  • Lee: you want to be able to accurately model the type of the response shape. Languages which don't have the capability of union types: they can say this is a string, this is an int, but they can't say this is a string or an int. Modern languages can handle this, but older languages cannot.
  • Michael: but the question is can we push this requirement onto the client that has these restrictions rather than making it a requirement for the spec itself?
  • Lee: I think that's interesting and it'd simplify this. There's another argument to be made - a simplified understanding of the response. If you're using an untyped language then a human has to make a guess as to what the type is - e.g. Ruby/Python, given a query, given the shape, can I rely on it? With overlapping fields of different types they might end up doing integer math on strings/etc.
  • Lee: this PR is a very specific change to only the nullable union type, which most languages handle in a different way (and can handle).
  • Michael: in C# a nullable int is different from a non-nullable int, so they are compile-time different. But I'm not against this change.
  • Lee: part of the intent in this for the human legibility is that you should be able to look at the field in the query and know what type you're going to get back in the response in that position. A human might look at this and see Port.country_code is non-nullable so country_code cannot be null but actually if you get a Warehouse.country_code it could be nullable -> confusion.
  • Stephen: is it worth the complexity?
  • Matt: any time you have a fragment spread where you may or may not get the spread fulfilled then every field in it kind of has to be treated as nullable. Something indicating what fragments are fulfilled would help here ;)
  • Michael: the RFC for changing the nullability of fields from the client could be helpful.
  • Ivan: keyed fragments is more of a feature request, whereas this PR is more of a bug fix. If we decide to replace this mechanism at all then there should be no reason not to temporarily patch it. If we recognise it as a bug then we can fix it in a reasonable timeframe rather than doing something vendor specific/finding a complex solution.
  • Michael: what if it's a union? Different fields can have different meaning - client might not think it's confusing.
  • Lee: if I alias two fields to have the same alias where one is String and the other String! should that work?
  • Lee: the actual spec change proposal here has nothing to do with interfaces, it's about relaxing the response shape algorithm to allow nullable/non-nullable to be treated the same.
  • [19:40 GMT]
  • Ivan: restricting this to interfaces would make the validation rule more complex.
  • Matt: I don't think it should be limited to interfaces. My argument is that clients already have to deal with this with fragment spreads where one has a field and one doesn't so the field access has to be nullable in some way - it might not be included in the response. "It might be included in the response but null" doesn't seem like a meaningful difference.
  • Benjie: it is in JavaScript!
  • Lee: and JSON.
  • Matt: sure.
  • Benjie: seems like this strips the nullability comparison deeply even through lists, but is that what we do currently for field compatibility in interfaces?
  • Ivan: yes, interfaces make [A] and [A!]! compatible.
  • Lee: nods
  • Michael: the GitHub schema makes these kinds of changes - nullable on the interface but non-nullable in the implementations.
  • Lee: going back to your question: "is this a bug?" - I don't think so. It's a surprising constraint, but not a bug. Querying a field on an interface and querying a field on a subtype of that interface does not give you the "same type" due to covariance. It's certainly unintuitive - we're not patching something not working as advertised; it's working as advertised but it's not clear. Paths forward:
    • null case: no change
    • response shape is too strict, make it more covariant response shape - make nullability not part of "same" comparison
    • make specific rules for two fields coming from the same interface
  • Ivan: I think not recognising this as an issue would add it to the list of non-intuitive things.
  • Lee: I loosely favour the first option: make no change. Primary impact this will have on downstream tools doing type generation - this'll loosen an assumed constraint that these tools may have which could lead to NPEs for existing code.
  • Ivan: queries that use this feature would be invalid right now so it won't break existing queries.
  • Matt: the concern is more existing clients on new queries.
  • Lee: "I assume the query is valid because a tool upstream would have invalidated it; so given it's valid I know this will be <type X>" and may make an assumption that it gets wrong. Those bugs can be fixed; but it's not a "cleanly safe" change because it involves fixing assumptions in other software.
  • Michael: is the change sufficiently impactful that we must do it?
  • Lee: how frequently does this confusion point surface, how complex is it to resolve? This isn't the first time it's been discussed (ref Sacha's conversation).
  • Michael: I'd also like to go in that direction - exploring what makes sense to change, widen it further, come to a decision but not in a rush.
  • Ivan: when I was presented with this I was certain it was a bug, then I thought the interface implementation rules should match merging - without context on how spec was written I thought it was intentional they were so close to each other, thought it must be a bug. But now I realise it's not a bug which makes things harder to decide direction.
  • [19:55 GMT]
  • Lee: I thought it was a bug when I first saw it too, but when I looked in more I realised it was a constraint working as expected. But it is unintuitive which is why I'm only loosely favouring no change. Michael's guidance here seems smart - rather than "there is a bug with how we think about inline fragments for implemented interfaces" think "what if we generalised changes to the shape compatibility" [paraphrased].
  • Ivan: should I go really far down Sacha's route and fully generalise, or go step by step?
  • Lee: I'd be dubious of completely removing it, whereas your proposed change here is sharp and smart and specifically narrows in on nullability being a different class of response shape definition. What's the range of removing the constraint - what does it involve now? Doing one doesn't prevent us from doing the other later.
  • Ivan: for next time I'll prepare a GraphQL.js PR and speak to people working on clients - it's easier for me to do this now because Apollo have Android and iOS clients.
  • Ivan: can we move it to Stage 1 to agree this is a problem to see if we want to see how to fix.
  • Lee: yeah, I think that's okay.
  • [19:59 GMT]
  • Lee: that wraps us up almost completely on time, I'm going to drop that last agenda item but recommend that you check out the discussion on Discord.