Watch the replays: GraphQL Working Group Meetings on YouTube
- Determine volunteers for note taking (1m, Lee)
- Review agenda (2m, Lee)
- Review previous meeting's action items (5m, Lee)
- 🗳 2023 Technical Steering Committee (TSC) Elections 🗳
- Defer/Stream discussion (60m, Rob)
- Next steps for
@oneOf(5m, Hugh)- Is the
@oneOf proposalstill being considered, or has it been superseded by the Struct proposal?
- Is the
- Benjie
- Bobbie
- Move
@oneOfitem above Stream/Defer since it's quick
- Ready for review
- All open action items (by last update)
- All open action items (by meeting)
- #796 - Still worth investigating; chat to Ivan if you're interested to
investigate
- Relates to relaxing a constraint on field selection merging; spec change is slight, but ramifications are significant so we want to test it in the reference implementation to see what the effects are
- #1099 - Still thinking about the strategy around this
- #1100 - Will discuss during
@oneOftopic - #1055 - discussion still taking place
- #1057 - "Everyone" task; Rob confirms the related topic is marked as resolved, so we're closing it
- #1056 - GraphQL-over-HTTP spec is getting good feedback, we can close this
- #973 - CCN still floating opened, but the action itself seems to have been dealt with, so closing.
- #879 - still needs addressing
- TSC Elections process & issue
- Self Nomination form
- Lee: Nomination opening date slipped past me, so I've pushed the entire schedule back by one month. Please look at the links above. IMPORTANT: nominations are open and will be open through until the end of this month. Please apply if you want to be a member of the TSC.
- What does it mean to be on the TSC vs in the WG?
- Not a tonne.
- Some things explicitly require votes, the TSC runs those voting processes (e.g. how to issue technical grants, approve TSC members, etc)
- Also administering GitHub - write permissions across all repos.
- Went smoothly last year.
- Nick, Dan and Rob are not planning to re-nominate themselves, so there's a healthy appetite for people to nominate themselves.
- We'll probably know the results sometime early January.
- Lee: gut check meeting cadence?
- Roman: is there a better way to sign up for the meeting?
- Lee: you can edit the files through the GitHub UI which is less bad than having to clone everything/push a PR.
- Roman: Google Doc?
- Michael: prefer the history in GitHub
- Hugh: sign up and adding agenda items being linked in one place is nice
- Yaacov: maybe we could introduce a bot to automate this?
- Michael: we also have the CLA bot
- Lee: that's important - it's a requirement you've done that. Anything anyone discusses here is open and deemed a contribution so we need to ensure everyone has agreed not to go rogue.
- Lee: there's merge request conflict issues, so this is definitely not the cleanest process. Maybe we can do auto-PR merging so long as COC/CLA is signed?
- Roman: maybe we can email just one person?
- Benjie: lots of people merge these PRs, I'd rather it was distributed than on just one person.
- Denis: could automate from issues.
- Lee: cadence-wise seems silence.
- Hugh: it's good this is the most popular meeting and the others have more space for longer discussions.
- Lee: we should add a standing agenda item to report on major things discussed in the other meetings.
- Benjie: would be good to automate uploading to YouTube.
- Lee: Zoom has a feature to stream to YouTube.
- Michael: it works well. Almost zero setup. Stream key from YouTube into Zoom, and it creates the video on YouTube automatically - live streamed and then persisted.
- Roman: it's a video of our homes, so better to record and then upload rather than to live stream.
- Lee: certainly at a minimum we'd want to make sure everyone was comfortable
- Benjie: we can always remove the video
- Is the
@oneOf proposalstill being considered, or has it been superseded by the Struct proposal? - Benjie: Not mutually exclusive, not clear we need both. Jumped on this because he wanted STructs, not solving input unions problems. Can address input unions problem. With oneOf pattern. Question if we need both of these if we introduce struct. Question is which is better, union or oneOF, so they are tied together, which is the reason for delay.
- Lee: we had counterproposal to oneOf that was similar to struct, the reason
not to pursue that in favor of oneOF is that many existing schemas already
have structure that mirrored what oneOf was doing without tools to guarantee
it and ability to leverage the constraint.
To roll out input unions, a new kind of hting like a struct would not be viable (migratino path from what is already there to something like struct) would be painful. The application of oneOf since this is already the modeling they are using, would be easier. So to keep in mind, future uses of graphQL that can start schemas from scratch struct might be more useful, but existing schemas that want to incrementally adopt the new features, would be easier… earlier on Lee had favored struct approach, but this line o thinking mad ethe directive easier to adopt. Might be a reason to pursue enthusiasm around oneOf.
Michael: The impact of oneOf is small to the spec. People already have immediate value from it with new validation rules. Struct will take more time to get into the spec to consider all the edge cases. Michael favors getting oneOf now and then later struct - Benjie: We have also been having discussions with metadata and with that we may not need oneOf to be a spec, could be its own thing like custom scalar which would reduce the problem of having too many forms of polymorphism in GraphQL. If we solve metadata, people can use oneOf through introspection without having to go too deep into the spec right now. Benjie thinks that struct and oneOf work quite well together, so is ok with moving both through, but does not want to do it in exclusion of struct.
- Lee: hunch is having it in primary spec will allow it to be most useful rather than having to cross-reference things. Nice to have a definitive version of what it does (single source of truth). If it takes wind out of the sails of struct, then that is a sign of value add, but he doesn’t think this is the case. (a useful pressure test).
- Michael: if we want to solve only the input case, or if we also want to apply the output case. Last take is to only work on the input side as that’s the problem area.
- Roman: does not feel “it” needs to be in the spec. Application Developer, building model, can use UNION, problem with UNION is it returns something and client does not know it has to guess. Can create the type that contains fields with variation, and make both fields nullable and for each variation has a tag. Have preceded it by field name and made it clear which is equivalent. Only thing missing is you have to make all fields nullable, and can return the thing with all fields null. Inconvenience, oneOf can add to this additional validation. Simple attribute that can be implemented as a custom attribute, should we standardized, Roman does not feel like it should be standardized, required by every implementation to have it.
- Roman: custom implementation can handle, should it be standardized? Does not think it should be required. Can miss fields on input, still compliant if nullable, only thing missing is schema suggests that all can be null. oneOf suggests one and only one always.
- Lee: thinks it is useful and feels strongly that it should be standardized. Feedback from greater community is that this is a critical missing piece, and we see it in schemas with people struggling to implement this. Solving this root problem is quite important, either with struct or with oneOf eneds to be in the specification.
- Roman: is valuable, but would vote for oneOf instead of input Unions. Structs is a much more advanced case. oneOf is a clear straightforward case.
- Hugh: oneOf is still something up for conversation, and enthusiasm for it.
- Clients can't reliably distinguish if defer was inlined or not
- Rob: Last meeting we discussed #52 - client's can't always determine if a fields was deferred or not. Applies to both defer/stream.
- See: graphql/defer-stream-wg#52
- Suggest adding "pendingPayloads"
- Benjie [chat]:
- What if you have
- { list { list { …@defer { stuff } } } }
- That could be HUGE pendingPayloads for very trivial data
- Michael: this seems complex, and fragment modularity could solve that? Label might not even be needed. This feels more complex/error prone.
- Michael: we're experimenting with @stream/@defer a lot; If you use it with
relay then you can go very easily from 20 patches to 3000 patches. The best
way to work around that right now is the hack that you suggested using
__typename - Matt: since that hack is proliferating… Really the hack is placing in the response itself a location that says "yes, we did this, so don't look at pendingPayloads/incremental/etc". The server should be able to take that 3000 deferred items response and collapse it down into 3 deferred items; but you need a way to know that happened - and it feels like sticking a value into the response itself that's unique to that defer is necessary.
- Michael: I find the labels to not be useful. On the backend side optimizing the queries we lose that.
- Matt: "fullfilled defer" should exist, but rather than it being true it should be the identifier for the pending payload, or a tuple of identifier, fulfilled.
- Lee: should this be opt in? I.e. do we only care about it in a few defers, or all of them?
- Matt: if we always put deferredPayloads in incremental then this is not necessarily a problem. If you know they're all in the initial response, not really a problem. It's when there's a mix that there's a problem. Not sure how you'd have a client correctly handle this situation without some inline metadata or somewhere you can go to look.
- Michael: metadata or fragment aliasing would work. With fragment aliasing you lost a bit in fragment merging optimizations but it feels worth it.
- Michael [shares screen; sharing Banana Cake Pop]
- Streaming nodes from a connection, and then deferring a fragment on the result. 20 patches
- Add a stream into inner nodes; suddenly 3000 patches.
- Now backend has to do lots of work, AND client gets terrible performance.
- We allow server to optimise this - do complexity analysis.
- But we need metadata/fragment aliases so that we can know what has happened.
- With
foo: …PriceDetails @defer- an aliased fragment - you get alignment between the client structure and the GraphQL query.
- Matt: we're "splitting out" the aliased subtree into its own independent subtree. Having 3000 responses doesn't change the fact that you'll have them as separate subtrees. There's an argument that even with fragment aliases you may want to collapse and merge. But that adds complexity on top of complexity.
- Lee [chat]:
- Yeah I’m concerned about tracking the metadata being more expensive than the value of having deferred the field in a non-trivial set of cases
- Lee: I'm concerned the medicine is worse than the disease.
- Michael: I'm personally in favour of aliased fragments.
- Lee: if the default mode is one where you don't get tracking, you just get the sense that there's more stuff to come, is that a reasonable default.
- Matt: the key thing to know is: when we're about to render something, am I in
1 of three states
- made the request, waiting on deferred data
- have the deferred data
- there was an error, never going to get the data
- Need a way to differentiate between these three states on render. This is independent of which client you're in.
- Roman: I agree. So long as you get a signal to tell you when it's done. Isn't it up to the server to choose to batch them and send the batch?
- Michael: you can get the server into trouble with that, because the incremental batches can get huge - building huge data in memory.
- Roman: why not cut them?
- Matt: we're trying to figure out what mechanism we can use to collapse the 3000 payloads into 20 or whatever and yet know which of the 3000 items are fulfilled.
- Michael: currently we discourage optimization of this in the server. Naive implementation of the spec introduces a security risk into your GraphQL server, and this shouldn't be the case.
- Yaacov [chat]:
So far solutions include: (1) __fulfilled field (2) reference proposal which acts like fulfilled, but syntax-wise in the query is positioned like an alias (3) true fragment modularity where the alias wraps the deferred fields- (4) variations of reference that Matt proposed in a comment
- Benjie [chat]:
__fulfilledworks for@deferbut not@stream, right? (You can't know the last item in that list is done?)
- Michael: even creating the backend tasks cause branching - e.g. for 3000 items you might branch 3000 times. It's typically more efficient to drop the @stream/@defer directive and just fold it in instead of creating the context branching and starting the processing. (This is closely related to the spec text that we have at the moment.)
- Rob: we say you SHOULD unless you have a good reason not to. I think we should update this language and have clearer guidelines on helping the server not be DOS'd by a client.
- Michael: we should fix this in the spec if the DOS is possible.
- Roman: just let the server decide whether to defer or not
- Rob: but the question is about indicating to the client what the server has done.
- Hugh: what about Ivan's suggestion?
- Ivan: as Benjie points out we have similar problems: is it inlined? Is the
stream finished or not? Two separate problems. This solution solves both
without introducing a new concept - instead of pendingPayloads we add
booleans. If it's inlined, nothing adds to incremental. If not inlined, you
get label, path, and
hasNext: true- you don't repeat in next payload, you just send the data when you get it. Should work well for defer and stream, and has two benefits. Helps remove the DOS problem with huge pending payloads - no need to repeat pendingPayloads. Indicate if rejected/if error happens. Removing it from array is boolean flag - if it gets rejected, sendhasNext: false- don't wait any more. - Rob: this still has the same issue Benjie raised with
{ list { list { …@defer { stuff } } } } - Yaacov: if something is inlined, where is that indicated in the payload?
- Benjie: I don't think it would be, that's the point.
- Ivan: to avoid the explosion is to make the inlining implicit and non-inlining explicit. Otherwise we risk denial of service.
- Rob: do we think better guidelines around ignoring/not ignoring defer/stream would help with the DOS issue?
- Michael: we should not discourage people from dropping a defer. Need guidelines on how to estimate when you should drop defers
- Benjie: applies on a per item basis, can inline what data you have. A defer with a label, the entire label is deferred or it isn’t. This allows it to be granular. Effectively a list of promises. Once this is above a threshold, maybe you inline everything from there onwards. Can do this on a discoverable basis as you walk through resolving the data.
- Rob: Like Benjie said last meeting, good default is ignoring a defer stream that is inside another defer/stream
- Matt [chat]:
- I have to drop off, but if we don’t do inline keys of some sort, I think we should focus on refining Ivan’s solution. Though I’m happy to also jump into figuring out what some form of inline-key solution might look like
- Lee: A real problem to be solved here. If we have fragment aliases would thi snot be a problem at all? Does this ideal solution speak to a missing piece rather than working around what we arledy have. WE have not hit on a resolution forward, but good input to continue …
- Rob: breakout group to discuss. (was decided at the last meeting but didn’t happen due to holidays).
- Michael: suggesting list of problems written down/small document to guide such meeting.
- Separate between 2 issues: 1. when to ignore / not ignore and 2. What info to give the server to handle response.
- Agree to Membership Agreement, Participation & Contribution 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)
- Licensing & final legal sign-off for GraphQL Scalars project (5m, Donna)
- Review a few active approved simple PRs and merge 974, 975, 980, 981, 982, 983, 985
- Benjie
- Matt
- Ready for review
- All open action items (by last update)
- All open action items (by meeting)
- Benjie: No updates on action items
- Donna: Thanks to whoever connected the URL! Still need to sort out the license
- Table to end of meeting
- Need to assign task/action item onto Lee to deal with
- Donna: if there’s a way for me to help out, I’m happy to pitch in.
- Benjie: open PR, don’t actually merge, and have Lee push the button
- P30: looks good to Benjie
- 981: Need to make an editorial decision for the spec capitalization (CREATE ACTION ITEM)
- 983: Action item on Matt Mahoney: review and help improve wording so we don’t impose “this is really hard” emotion
- 985: The sentence is not as accurate as it needs to be, but to fix it we should address all instances of "parallel" in that section.
- ACTION: Matt - provide specific suggestion on how to address 985
- 984: [long discussion around why this is a selection set and not a mutation
operation]
- Benjie: this defines serial execution of a selection set, rather than explicitly execution of a mutation operation.
- Matt's example shows an inline fragment spread, to show it's the selection set not the operation that's important. \
mutation M($newBirthday: Int, $newAddress: Int) {
... {
changeBirthday(birthday: $newBirthday) {
month
}
}
changeAddress(address: $newAddress) {
street
}
}- Matt: Maybe runnable examples in the spec would be helpful?
- Benjie: perhaps we should add a title or caption or a non-normative note that strongly indicates this is a selection set, not an operation/document (and maybe gives an example of a full operation). \
- Matt: "selection set" is meant to imply "part of operation"
- ACTION: Roman didn't read this as a selection set but instead as a document and felt that "mutation" is missing - we need to make it crisper and clearer that we're talking about selection sets here.
- Matt: we use selection sets throughout the documentation; this is where it jumped out to you, but if we fix it here we need to fix it everywhere where selection sets are referenced.
- * BRING TO JAN WG: process for getting TSC eyes/approvals on small PRs that have at least 1 approval.
- Nominate yourself!
- At least three of the five currently-being-elected seats are not going to be filled by incumbents
- Votes early in January
- Agree to Membership Agreement, Participation & Contribution 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)
- Field error resulting from insufficient validation of variables (15m, Benjie)
- Add a style guide to the GraphQL specification (15m, Benjie)
- Update from stream/defer breakout group and new proposal (20m, Rob and Benjie)
- Hugh Willson
- spec#1002: Issue with reproduction
- If passing null the variable default doesn’t kick in
- Can ultimately lead to a runtime error
- Seems like it should be a validation error instead
- We thought this was fixed, but we only sort of fixed this (and kinda backwards)
- It sounds like we missed something here
- The motivation to get this right was to explicitly disallow nulls from making it through a well typed boundary; could be a security risk if we allow them through
- The code in the issue was evaluated from the point of view of the spec and from a running test case, so it’s a real issue
- Is there a straightforward solution here - when providing null it’s interpreted as not provided; default values could override null
- Or, we move the assertion to be later in the process
- Solution here could be breaking
- Queries that were previously valid would no longer by valid; validation rule would complain, so this is probably a non-starter
- What was previously erroring, feeding null in, does what it says and coalesces into the default value
- If we coerce to a default value it becomes impossible to actually pass null in so this could be breaking as well
- Breaking change that relates to the validation rule seems like the better choice here, but this might only be fixable when we’re ready for a version bump
- We have a note about this in the spec: http://spec.graphql.org/draft/#note-ba9c8
- Details: http://spec.graphql.org/draft/#sec-All-Variable-Usages-are-Allowed.Allowing-optional-variables-when-default-values-exist
- Origin: graphql/graphql-spec#418
- It should throw a field error but not a null pointer runtime exceptional; we need to prevent that from happening
- This situation seems like a bug in the spec; we’re enabling clients to give us what appears to be a valid request, when it could lead to problems
- We last worked on this problem 4 years ago
- Could introduce a new validation rule with backwards compatibility saying this validation is a should not a must
- Could be enabled/disabled via an option flag in graphql-js for example
- Nervous about changing the semantics of what it means to have a default value; lots of issues trying to get this right in the past
- General preference to introduce things as SHOULD
- Can we describe what we want here and work backwards?
- Not opposed to adding an appendix about this in the spec
- What is the text of the field error from the reproduction - added here: graphql/graphql-spec#1002 (comment)
- It seems like this line is the one in question and needs the change, but this will need to be confirmed: http://spec.graphql.org/draft/#sel-IALbLDFCFCAACEYoxc
- Action: having default mode be the happy path which will require to spec text changes
- PR
- Inconsistencies in styling
- Especially around capitalization
- Providing a style guide here could help
- We most closely match the AP style guide
- Propose we follow the rules there
- Have started a basic new style guide and updated things to follow
- Chicago style guide is great as well, but decided not go with it because of prepositions / conjunctions no matter how long they are, whereas AP only applies to preps / conj under a specific length
- Love anything we can just lint rule away
- AP style guide is nice and simple
- How do we preserve this going forward; add a lint tool to apply this moving forward?
- Looks like we could use something like: https://vale.sh/explorer/ap/
- Next steps: All folks think this is a good idea; Benjie is going to run with it!
- Proposal
- The above is the outcome of a separate breakout session earlier this week
- This is the leading proposal so far to solve the issue
- Details and examples are broken out in the discussion thread so not repeating them here
- Definitely an interesting proposal
- Question around labels and reasons and how useful they will be
- Reason shouldn’t be required, nice bit of extra information, but not necessary
- Labels and path are necessary to uniquely identify what triggered things
- We did consider getting rid of label entirely but decided to keep it because we might want to defer things separately and not have them be merged
- Modeling these as promises / async interables
- Path to know where you’re merging data in, but need the id in case you’re merging multiple things at that path
- What do labels and reasons solve for clients? Do they help or hinder?
- Are labels redundant if deferred fields can already be identified separately, meaning they can be pulled back in on the client and merged effectively without the label
- Reason is just noise right now; but may be useful in future
- If labels are optional what is the situation that requires a client developer to add a label?
- Labels can be seen as identifying deferred fragments as separate promises, conceptually
- Use case of a label to distinguish between logically different defers that can’t be merged makes sense
- Less clear as to the advantage of client developers having to worry about an array of labels coming back
- Also concerned about adding a bunch of extra stuff in deferred responses; could reach a point where all of the extra response overhead negates the benefits of defer
- Not sure about merging of the labels; now there is some specific meaning behind if you want things to be merged or not that developers need to worry about (the client library can’t hide all of these decisions from end users)
- Proposal is super interesting but we need to figure out the label part
- Next steps: We have a follow up breakout meeting scheduled for Dec. 19th