Skip to content

Latest commit

 

History

History
313 lines (274 loc) · 17.3 KB

2023-02.md

File metadata and controls

313 lines (274 loc) · 17.3 KB

GraphQL WG Notes - Feb 2023

Watch the replays: GraphQL Working Group Meetings on YouTube

Primary meeting

Agenda

Determine volunteers for note taking (1m, Lee)

  • Benjie
  • Hugh

Review agenda (2m, Lee)

  • [no comments]

Review prior secondary meetings (5m, Lee)

  • WG secondary APAC
  • WG secondary EU
  • Main advancement: Scalars website is now live.
  • Secondary advancements: the increased cadence has helped increase the velocity of stream/defer and fragment arguments discussions.
  • Review of fragment arguments would be useful now, so please have a read!

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

TSC: Election Results (5m, TSC)

  • Successful election with 10 nominees and only 1 person who put themselves up for reelection.
  • Congrats to the new TSC members, and to Andi on re-election!
  • Thanks to everyone who nominated themselves.
  • Best thing about TSC is we mostly defer to the WG, so everyone can take a leadership role.

TSC: Management of build and publish infrastructure, secrets, etc (20m, Benjie / Rikki)

  • This meeting is being recorded, so be mindful of the audience - we can take some of these communications private if need be
  • GraphiQL's last publish was partially unsuccessful due to token issues: graphql/graphiql#2997
  • GraphQL Foundation should have a more robust process for handling planned or emergency token revokation for individuals
  • Rikki has proposed a solution
  • Thomas is stepping in for Rikki
  • Matt: Ideally two or three owners. TSC members? Require 3 of these users to do a minor version bump and release (even a NOOP), just to test. If we don't exercise it, there's no guarantee it will work.
  • Benjie: this partly relates to the auto-deployment from CI, which has security implications around tokens and two-factor authentication tokens.
  • Lee: GitHub Actions auto-deploy is the best balance between security and convenience. Local project maintainers just need to do the mechanics of a version bump, they don't have to worry about tokens/MFA/etc.
  • Benjie: what's the next steps? GraphiQL is currently broken.
  • Lee: there's the right way, which I'll take ownership of. Is there something we can do today to unstick GraphiQL?
  • Thomas: short term would be to have a token with all the necessary permissions, even a personal token.
  • Benjie: I believe the issue is that even TSC members don't have access to the @graphiql/* package
  • ACTION: @leeb - address this.

Fix ambiguity around when schema definition may be omitted (10m, Benjie)

  • RFC; previously discussed in September
  • Aim: merge this (mostly) editorial change
  • Lee: this is a change in behavior but it does seem to align with the intent.
  • Lee: lets check this with the implementations to make sure there's no issues there. It may be that when printing the schema out there is an issue where the old behavior was followed.
  • Benjie: I think the code block in the top comment can be used as a test case - read the code in, generate the schema from it, and print it out - it should be the same (i.e. no mutation operation)
  • ACTION - Benjie - tag implementors and check that this won't cause issues.

Advance argument name uniqueness to Stage 3 (10m, Benjie)

  • RFC
  • GraphQL.js implementation is merged
  • Issue still remains; generally agreed this needs to be addressed
  • Champion hasn’t been at the WG’s, so what are our next steps here; what is the process for moving this forward
  • Someone should claim champion (Ivan did on the code side; PR was merged a while back)
  • Spec needs to be updated to agree with the reference implementation now
  • Looks straightforward - folks agree it’s ready to advance
  • It’s ready to merge … merged, YAY!

@defer: fully deduplicate response (30m, Ivan)

  • [RFC](robrichard/defer-stream-wg#65 (comment))
  • GraphQL.js implementation: TBD
  • Agreed at the previous working group that deferred fields that also exist in the parent (non-deferred) selection set should be omitted since they're already satisfied.
  • Then we agreed that sibling @defers should also merge, so ... @defer { a b } … @defer { a c } should become ... @defer { a b c }
  • Rob has come up with some challenging examples to see what would happen.
  • For some examples we can solve them statically and they don't cause any issues.
    • A defer directly inside a defer (... @defer { … @defer { ??? } }) is a bit controversial, and it's hard to extract the intent that the user had when writing the query
    • The incremental delivery WG generally agree that all the defers at the same "path" (independent of selection sets) should be merged together, so ... @defer {a {b} … @defer {a { c } } } should be equivalent to ...@defer { a { b c } }.
  • For more complex examples, we cannot determine statically how to fully deduplicate because e.g. two defers can come in either order. One option is to deduplicate at runtime and handle it at the transport layer.
  • Matt: we shouldn't have incremental payloads that don't satisfy at least one @defer - i.e. a defer should not be split over multiple payloads.
    • Matt [chat]: I feel weakly that there should be at most one incremental payload at the same path, and if there’s response duplication or fewer payloads than defers, that’s preferred.
    • Matt [chat]: I think product developers are expecting at least a complete set of data for the path they’re asking for and can’t actually use an incremental payload that isn’t yet completed, at which point there’s no use to splitting the payloads beyond once per path
  • Ivan's slides: https://docs.google.com/presentation/d/1hlq7sLCyxm4DQbi-7rciRFRZePmdrbgXD4dB5NL_DaM/edit?usp=sharing

Default Value Validation Status Check (5m, Yaacov)

  • Lee's original implementation PR stack at graphql-js
  • Now rebased on main
  • Benjie's Spec PR
  • Yaacov: call to action: review the implementation PR and how it aligns with the spec PR.
  • ACTION - everyone: review these PRs!
  • Matt was asking about what would happen if a scalar input value could be serialized to undefined. Does this lead to surprising behavior? Okay to just throw an error? Hopefully not a breaking change.
  • Ivan: not a breaking change to the spec. If necessary we can make a breaking change to GraphQL.js in a major version. I'd release this as a major release of GraphQL.js anyway. GraphQL.js aims to be replicable in other languages (e.g. GraphQL python is a direct copy) and most languages don't have a difference between null and undefined.
  • Benjie: every other language that implemented the old behavior should also release this under a major version bump because it concretely changes behavior of edge cases.
  • Lee: we should be clear about this because the difference is subtle.
  • ACTION - Matt / Yaacov - tag Lee at the relevant line of code to review.

Secondary (APAC)

Agenda

Determine volunteers for note taking (1m, Lee)

  • Benjie

Review agenda (2m, Lee)

  • Looks like a short one, so I can use remaining time to address the GraphiQL publishing permissions issues.

Review prior primary meeting (5m, Lee)

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

  • [skipped]

Fix ambiguity around when schema definition may be omitted (10m, Benjie)

  • RFC - currently stage 1, looking to advance
  • GraphQL.js implementation
  • [summary]
  • Have you had a chance to look at it in GraphQL Java?
  • Donna: yes, I had a look earlier and there is indeed an issue where the schema appears to have a Mutation type when it should not; we'll fix it.
  • Great! Basically ready for RFC2, the only issue is there's a test in GraphQL.js that I don't know why it exists and I had to do a special case handling for no root operation types in printSchema to handle it.
  • Issue is that if you have a schema consisting of the type type Query {foo: String} but that is not the root query type (the schema is invalid and has no operation types) then when you print it, either it looks valid (because Query is a default operation type name) or you print it as schema{} type Query{foo: String} but that cannot be parsed. I worked around it, but I'm not sure why this test exists or what it's trying to enforce.
  • Matt: could be related to removing the query type?
  • Lee: we should make the print-parse-print loop consistent
  • Matt: it is, it'll take an invalid schema and make it valid but then it'll be consistent from there on out.
  • Matt: at Meta we split the schema types into different files and parse them into ASTs and merge them to build the schema.
  • [changed to Matt's topic, then circled back]
  • Benjie: thinking about this, you cannot add anything to the SDL type Query{foo: String} to make it have no root types. type Query{foo: String} schema {} won't currently parse. So this is an invalid schema that cannot be represented in SDL; when we attempt to represent it via SDL we get what appears to be a valid schema, and then the print-parse-print loop is happy and consistent. So I think this is the right solution.
  • Lee: I'm concerned about the ?? null in the code, is that needed?
  • Benjie: yes, I think when you do query.getQueryType() it will return null if there is no query type, however if you do query.getType('Query') then it will return undefined (or maybe the other way around); so the ?? null means we consistently deal with null which makes the later queryOperationType === queryType comparisons easier.
  • Benjie: is there an action for me to take at this point, or do we just need people to review it?
  • Lee: I'll spend some time reviewing it.

Fragment Arguments (10m, Matt)

  • graphql-js PR is waiting for review
    • Do we need a flag? It's difficult to evaluate usage without graphql-js syntax support: can we can merge as-is without a flag?
  • Relay has PR enables the new syntax over directives
    • Blocker for Relay adoption within Meta is graphql-js, and therefore prettier, syntax support.
  • No pushback from Relay so far.
  • Before any servers have the new syntax we will require the tooling to support the new syntax.
  • There's internal questions over how it works, but the syntax seems well established and no-one is questioning it.
  • Lee: once something is released, people will start using it and it removes the ability to iterate if need be.
  • Matt: might make sense to change the name of the flag ("fragment variables"). Parser would just not understand things unless it has a flag. Alternatively the flag could be in the validation rather than the parser.
  • Lee: keeping the flag but renaming it seems reasonable; enables you to write tests, test corner cases, Relay to use it, etc whilst still ensuring we can change our minds if we need to.
  • ACTION - Matt: rename the flag and add it to the parser (that's what we're doing with Relay anyway)
  • Matt: Main question remaining is around scoping behavior.