-
Notifications
You must be signed in to change notification settings - Fork 228
/
2022-09-01.md
381 lines (354 loc) · 22.6 KB
/
2022-09-01.md
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
# GraphQL WG Notes - September 2022
**Watch the replay:**
[GraphQL Working Group Meetings on YouTube](https://www.youtube.com/playlist?list=PLP1igyLx8foH30_sDnEZnxV_8pYW3SDtb)
## **Agenda**
1. Agree to Membership Agreement, Participation & Contribution Guidelines and
Code of Conduct (1m, Lee)
- [Specification Membership Agreement](https://github.com/graphql/foundation)
- [Participation Guidelines](https://github.com/graphql/graphql-wg#participation-guidelines)
- [Contribution Guide](https://github.com/graphql/graphql-spec/blob/main/CONTRIBUTING.md)
- [Code of Conduct](https://github.com/graphql/foundation/blob/master/CODE-OF-CONDUCT.md)
2. Introduction of attendees (5m, Lee)
3. Determine volunteers for note taking (1m, Lee)
4. Review agenda (2m, Lee)
5. Review previous meeting's action items (5m, Lee)
- [Ready for review](https://github.com/graphql/graphql-wg/issues?q=is%3Aissue+is%3Aopen+label%3A%22Ready+for+review+%F0%9F%99%8C%22+sort%3Aupdated-desc)
- [All open action items (by last update)](https://github.com/graphql/graphql-wg/issues?q=is%3Aissue+is%3Aopen+label%3A%22Action+item+%3Aclapper%3A%22+sort%3Aupdated-desc)
- [All open action items (by meeting)](https://github.com/graphql/graphql-wg/projects?query=is%3Aopen+sort%3Aname-asc)
6. TSC:
[Revise TSC meeting text w.r.t. WG meeting cadence](https://github.com/graphql/graphql-wg/pull/1098)
(5m, Benjie)
7. Meeting times changes (updates!) (10m Lee)
- [Discussion](https://github.com/graphql/graphql-wg/discussions/1051)
- [Updated Proposal](https://docs.google.com/spreadsheets/d/1sbfxf9m2lMgfQSeawUy4sz6oO7TLjGZLvR0mryOI3LQ/edit?usp=sharing)
8. Specifying 'extensions' property on requests (10m, Benjie)
- [RFC](https://github.com/graphql/graphql-spec/pull/976)
9. Defer/Stream update (30m, Rob)
- [Spec PR](https://github.com/graphql/graphql-spec/pull/742)
- [GraphQL-JS PR](https://github.com/graphql/graphql-js/pull/3659)
- [Discussion on Errors thrown parallel to deferred execution](https://github.com/robrichard/defer-stream-wg/discussions/45#discussioncomment-3486994)
10. Fixing ambiguity around when schema can/should be omitted from SDL (10m,
Benjie)
- [RFC](https://github.com/graphql/graphql-spec/pull/987/files)
11. Separate "IsSubType" from "IsValidImplementationFieldType" (5m, Yaacov)
- [Spec PR](https://github.com/graphql/graphql-spec/pull/977)
12. Clarify Current "ResolveAbstractType" Algorithm (15m, Yaacov)
- [Spec PR](https://github.com/graphql/graphql-spec/pull/973)
13. Call for Feedback for "Expanding Subtyping" RFC (5m, Yaacov)
- [Link to RFC](https://github.com/graphql/graphql-wg/blob/main/rfcs/ExpandingSubtyping.md)
14. Schema Metadata/Applied Directives (if time allows) (20m, Lee)
- [Discussion](https://github.com/graphql/graphql-wg/discussions/1096)
## Determine volunteers for note taking (1m, Lee)
- Benjie
- Yaacov
- Mike
## Review agenda (2m, Lee)
- No changes.
## Review previous meeting's action items (5m, Lee)
- [Ready for review](https://github.com/graphql/graphql-wg/issues?q=is%3Aissue+is%3Aopen+label%3A%22Ready+for+review+%F0%9F%99%8C%22+sort%3Aupdated-desc)
- [All open action items (by last update)](https://github.com/graphql/graphql-wg/issues?q=is%3Aissue+is%3Aopen+label%3A%22Action+item+%3Aclapper%3A%22+sort%3Aupdated-desc)
- [All open action items (by meeting)](https://github.com/graphql/graphql-wg/projects?query=is%3Aopen+sort%3Aname-asc)
- Clarifying problems that S.T.R.U.C.T. will solve
- Benjie has added a few use cases to his proposal, w.i.p. to make that more
crisp, so this action item will remain open.
- Regarding the “domain” of the spec;
- Roman: need more time on this to figure out the next step. Action item says
to ask people about what they think it is, but I think it is what it
_should_ be.
## TSC: [Revise TSC meeting text w.r.t. WG meeting cadence](https://github.com/graphql/graphql-wg/pull/1098) (5m, Benjie)
- Matt noticed some conflicts with the WG meeting changes and the existing TSC
attending member definitions. Benjie has made some minimal changes to the
definitions for “attending TSC” members to align this.
- Will be kept open for 72 hours, and then will presumably be merged.
## Meeting times changes (updates!) (10m Lee)
- [Discussion](https://github.com/graphql/graphql-wg/discussions/1051)
- [Updated Proposal](https://docs.google.com/spreadsheets/d/1sbfxf9m2lMgfQSeawUy4sz6oO7TLjGZLvR0mryOI3LQ/edit?usp=sharing)
- Lee: had a chat with Elisa (PM); she framed that our calendaring is a mess.
Fortunately we’re operated mostly through the GitHub agenda docs. There’s a
link in the agenda to a Google Sheet where we can try and figure out all the
events that we have going on and when they are and if we can fit them to a
common cadence.
- Lee: I was going to propose doing this meeting bi-weekly, but it gets messy
based on when the months have 4 or 5 weeks. So for now we’ll keep the 1st
Thursday and add the 3rd Thursday. We’ll also shift the time based on the
feedback; 10:30am-12 Pacific time. Then adding the 2nd Wednesday for Asia
Pacific timezone 3:30pm-5pm.
- Alternate Thursdays can be used for the other working groups, perhaps.
- Michael: would be good to have one calendar with all the events!
- Lee: Zoom account will be the master of these, then all the public stuff on
one calendar.
- Lee: blocked out time twice a month that subcommittees can book slots in.
- Ivan: for me it’s better because it finishes as 10pm not 11pm. About subgroups
I’ll update asynchronously.
- Lee: I’ll talk with Elisa and we’ll book in some timeslots for the
subcommittees.
- Benjie: I was talking about Elisa about having a list of our subcommittees
somewhere, and maybe putting it in the GitHub.com/graphql README
- Lee: I think the top of the WG README would work too.
## Specifying 'extensions' property on requests (10m, Benjie)
- [RFC](https://github.com/graphql/graphql-spec/pull/976)
- Benjie: In the GraphQL Spec, we detail that the response can contain
extensions. We reserve the top-level space so that extra features can be added
without any conflicts. So the GraphQL-over-HTTP spec says that for a response,
you can look at the shape of the response in the GraphQL spec. But when it
comes to requests, the GraphQL Spec doesn’t explicitly say what the structure
of the request is; so this is done in the GraphQL-over-HTTP-spec.
- Seeking an opinion: should this live in the GraphQL spec or in the
GraphQL-over-HTTP-spec.
- Lee: the GraphQL Spec defines the data structure of the response. To allow
customization, we added “extensions.” But execution requires a collection of
things that are necessary to execute the request, but we don’t specify a “data
structure” at all, and so we don’t have to specify that extensions should be
included.
- Benjie: When we come to document graphql over other protocols, we’ll need to
define this in each spec. Don’t want each spec to define this concept over and
over again.
- Lee: Would a non-normative note work for this?
- David: Apollo has used an extensions field for a number of years. We’ve
encountered issues with servers that reject requests that include the
extensions field. It’s important that we make clear that servers shouldn’t
respond with an error when present.
- Ivan: suggests describing the extensions field in a proper section. Sort of
like a spec in a spec. GraphQL over HTTP spec can reference it, for example.
Create a top level spec describing the request so other specs can reference
it.
- Lee: Agree with the broad strokes.
- Benji: Add an entry that speaks in a more generic way about extensions.
- Lee: Might be too presumptuous on the transport to suggest the data structure.
E.g. multipart requests.
- Ivan: But we suggest format already, for example section 7.2. We already use
Map as an abstract data structure.
- Lee: Yes, but I’m not seeing the value of adding a restriction.
- Roman: A known structure will make it easier for tools like GraphiQL to use it
(e.g. display it).
- Marc: Restricting at the transport level seems very restrictive. E.g. headers
would be a good place for extensions.
- David: headers have some drawbacks, FYI. e.g. CORS.
- Lee: agree, but there are some cases where a header might make sense. Subspecs
like GraphQL over HTTP could define specific transport.
- Ivan: whether or not we use headers, it seems to make sense to be clear about
what’s in extensions (i.e. a map).
- Lee: Benjie seems like you have two options: 1) extension may exist, be aware
of it 2) improve section and do a better job of defininging the input to an
execution.
## Defer/Stream update (30m, Rob)
- [Spec PR](https://github.com/graphql/graphql-spec/pull/742)
- [GraphQL-JS PR](https://github.com/graphql/graphql-js/pull/3659)
- [Discussion on Errors thrown parallel to deferred execution](https://github.com/robrichard/defer-stream-wg/discussions/45#discussioncomment-3486994)
- Rob: over the past couple weeks I’ve been updating the spec PR w.r.t. our
discussions. Ready for review. The GraphQL.js PR is merged into the 17-alpha
(main) branch. Benjie raised an issue which I’d like to discuss
- [screen sharing]
- [19:47]
- We discussed about nulls bubbling up inside stream/defer, but this is kind of
the opposite. A non-null field _next_ to a streamed field could throw causing
the streamed fields to be nulled out. Because the stream/defer is happening in
parallel, it’s not canceled out by this.
- I’ve written up a pretty quick implementation, but it’s a bit clunky; in
GraphQL.js we know a non-null error happened and that it bubbled up, but we
don’t collect the information about which fields were actually nulled out by
the error, so we look through the response and find the null fields and then
exclude stream/defer subsequent payloads that would be inside that. It’s not
optimal because we have to look at the response.
- Michael: didn’t we say that defer is null boundaries? Ah! But it’s happening
on the main request, so it leads to tasks being canceled.
- Rob: it could be on main payload or defer/stream that kicks off more
defer/stream.
- David: two issues: could be that we send something to the client that should
have been nulled.
- What if they happen at the same time such that they are in the same batch.
- David: client still need to know no matter what that something that’s sent
needs to be nulled out.
- Michael: isn’t it guaranteed that we’ll find the nulls and not confuse the
client? Same in sub-defers, we have payload ordering, so we’re guaranteed to
find them all.
- Even if they are happening at the same time, the server must hold them until
the main request into which they’re patched into is sent down.
- Lee: ideally the spec doesn’t take an opinion on when the streaming begins,
it’s more important to describe when they’re delivered and what the
constraints on them are.
- Michael: in Ivan’s example ({ ... @defer { a } ... @defer {b } }) it doesn’t
matter because defer is a null boundary, so we can guarantee that. We can just
null out the defer, and not worry about raising the null further. We don’t
need to worry about stuff that’s already sent because it’s a null boundary.
- Link:
[discussion 23](https://github.com/robrichard/defer-stream-wg/discussions/23)
- Rob: we must define that a null can’t bubble up higher, otherwise we’d have to
wait for everything to finish before we can send anything.
- Yaacov: we talked about enforcing order of delivery payloads. Makes it easier
for clients. This is one example. We did talk about potentially relaxing that
requirement (or adding an argument to do so) which could allow out of order
delivery and then the filtering might be unnecessary because we have other
problems.
- Michael: out of order delivery doesn’t nullify that - we talked about it for
lists, not for null boundaries.
- Yaacov: I don’t think we ruled it out completely.
- Rob: both defer and stream are ordered currently; if we change that then
clients would have to opt in because it would break assumptions - we can
discuss these concerns then.
- Lee: I see that there’s a proposal to wait to run the stream until we know
it’s safe to do it - but what matters is the perceived result, so you could
still start executing earlier as an optimisation so long as to the observer it
looks like the result is ran in a particular order.
- Lee: maybe you can make sure that you can’t be nulled out, and _then_ start
the stream. Then as an optimization put it where you want, but don’t break the
guarantees. Better performance for more complicated stream canceling behavior.
- Rob: with GraphQL.js being the reference and also widely used, would it be
weird for it to do the optimization.
- Lee: it’s not clear which would actually give the best performance - if the
odds of a neighboring field throwing is high then maybe the streams being
canceled is likely, so it’s cheaper to not start it. Default to the optimized
path but allow to de-opt, or vice-versa.
- Ivan: practical question - what should we do in GraphQL.js? Looking into the
data is not effective. Even if we wait for the entire payload to resolve. We
can track what was blown; previously everything we tracked internally on
errors we exposed publicly in the response (e.g. original path (where error
originated) is exposed as the path). Back to Rob’s PR; for performance reason
it makes sense to not check the data but to track what was blown by each
error - should we keep it as an implementation detail, or is it something that
we should expose?
- Benjie: I think the client can figure it out from data and path (and it’s
browsing the data anyway) so I don’t think it particularly valuable to track
it on the error - do it as an implementation detail.
- Ivan: so if an error happens client needs to browse through the data to find
out if a particular stream/defer was not even started.
- Ivan: in GraphQL.js we’ll do it as an implementation detail, and we could
think about exposing it on the client. I take my question back - we can add it
later.
- Rob: we could describe in the spec that a stream shouldn’t ever be returned to
the client because it’s inside something that was nulled out. The way I wrote
it was a consequence of how difficult it felt to keep track of where the null
bubbled out to. I’m concerned about stretching out the request duration longer
than it would have been without defer.
- Lee: one way to consider formatting this.
- There’s this constraint of there’s a path into the previous payload, before
I ship a payload I should check that the path exists.
- At the moment the error occurs, know what the affected streams would be, and
in that situation mark those as broken.
- The second one doesn’t feel reasonable because it doesn’t align with the way
that you’re doing execution. The way we’re collecting subsequent payloads is
that we’re collecting a mutable list that we append to. The other way to think
of it is steps returning a tuple of the data and the subsequent payloads; then
we can cancel the ones when a selection set blows up.
- Benjie: I’ve actually implemented both, and found that the first one was
better because it has less object allocation (no tuples) and it doesn’t have
to do any work if there are no errors.
- Lee: there’s approaches that you can do to optimize that. The mutative adding
of the entries into the list before you know if it’s successful or not is the
root of the problem, you could non-mutably make sure that this never happens.
- Rob: should we consider just making it so that clients have to deal with it?
- Michael: we’re well equipped to not send it down, we save bandwidth.
- Lee: it holds us to the golden constraint: subsequent payloads are definitely
mergeable in order. I think it’s the right way to think about when the
previous one had a problem.
- Michael: it’s a good catch!
- Rob: I’ll look into this more, and think more about how to describe it in the
spec.
- Lee: props to you and Benjie for spotting this and digging into the weeds.
- Rob: I wanted to get clarification on what stage this proposal is. Stage 2?
The spec PR has the stage 1 tag. Where are we?
- David: I think we’re at least _very close_ to Draft status from the
definition.
- Lee: I think that’s about right. The mutable vs non-mutable list thing feels
big in terms of spec languages, but it relatively minor in terms of everything
else.
- David: the spec doesn’t have many examples, but I think you’re working on that
soon.
- Lee: we’d definitely want that! This is by far the most complicated addition
to GraphQL since its open sourcing, so there’ll need to be a lot of material
around it to help people use it.
- Lee: it’s stage 2!
- Roman: I submitted notes into the PR, have you seen them?
- Michael: yes, and you’ve got some responses.
- Roman: in respect of Lee saying it’s the most complicated addition to the spec
since OSSing it. Have you considered this or not… should it be in the main
spec, or should it have its own spec? It makes it much harder to read the main
spec, and makes defer/stream harder to read too. Would it be beneficial to
create a separate spec for it, and maybe we can do similar for other complex
features. No need to discuss it now, but something to think about.
- Lee: it’s a good point. I’d rather it doesn’t become a separate spec because
it would make maintenance of both specs challenging; but your point on
legibility is on point and I’ll be trying to ensure it’s sufficiently isolated
so you can skip over the stream/defer branches if you don’t want to support
them.
- David: I don’t see a realistic way that this could be separated from the main
spec because of its effect on execution. Perhaps it could be merged
incrementally - e.g. just defer and not stream. Or adding defer/stream but not
combining with subscriptions. Either way it’d be good to get it out!
- Lee: it’s a good point. “If it’s possible then it will happen” seems to happen
a lot in the GraphQL domain!
- David: we’re planning to ship @defer support at Apollo very soon, we don’t
want to diverge from a future spec - so not only do we do content negotiation,
but we’re planning to do a parameter in the accept flag that allows us to
support future versions. This should also help share more feedback.
- Lee: I’ll leave that for your crew to balance with opt-in versus subtle
changes / potential for technical debt.
- Michael: we support both payloads for defer/stream, so we’re already paying
that price!
- Lee: FWIW it does feel like we’re far closer to done than not done. There was
a lot of iteration and I feel reasonably confident that what we’ve landed on
should cover the cases we care about.
- Hugh: the community is excited!
- Lee: thanks again Rob for being torchbearer!
## Fixing ambiguity around when schema can/should be omitted from SDL (10m, Benjie)
- [RFC](https://github.com/graphql/graphql-spec/pull/987/files)
- Benjie: two paragraphs in the spec that I feel are not clear. Pointed out by
Roman. I’d like to clarify what they should say currently. Carefully crafted
the wording to be more clear.
- Lee: Change makes sense to me. Will mark as editorial.
- Roman: pointed out that he has made some suggestions here, asked the group to
read this thread and provide feedback. This second sentence doesn’t make much
sense without knowing the context.
## Separate "IsSubType" from "IsValidImplementationFieldType" (5m, Yaacov)
- [Spec PR](https://github.com/graphql/graphql-spec/pull/977)
- Please review the spec PR ^
- Lee: usually when we change algorithms in the spec, we want them to mirror
functions in the reference implementation. I’m pretty sure that we have a
function, and the implementation is exactly this?
- Yaacov: I think it might be called “is subtype of” but yes there is a
function, I’m not sure if it’s exact.
- Lee: that would be my one request before merging this - to ensure there’s
parity.
- Ivan: yes, and it has exactly the same name. The implementation PR happened
long before the spec PR!
- Lee: it’s slightly more complicated but for a different reason.
- Ivan: it’s because we’re doing caching - precompute stuff on the schema
itself, so it’s an implementation detail, because it’s used in execution.
Semantically it’s the same.
- Lee: I’m merging this as an editorial change because it makes it closer to the
reference implementation. That unblocks you to getting the next one into the
state you want, but I don’t see an issue with that either.
## Clarify Current "ResolveAbstractType" Algorithm (15m, Yaacov)
- [Spec PR](https://github.com/graphql/graphql-spec/pull/973)
- Please review the spec PR ^
- Took the GraphQL.js implementation and used that to clarify.
- Before we change the behavior we should clarify the current behavior.
- I’ve had some hesitations about how valuable those changes would be. I wanted
it to better match what should happen. You can’t just rely on the internal
method - you have to assert that it’s working right.
## Call for Feedback for "Expanding Subtyping" RFC (5m, Yaacov)
- [Link to RFC](https://github.com/graphql/graphql-wg/blob/main/rfcs/ExpandingSubtyping.md)
- Nothing much new to report, but I have written up linking together the
different topics and how they interrelate.
- Ivan: union implementing interfaces makes sense because you can use fewer
inline fragments to specify types. For the range of all the different
proposals, unions implementing interfaces makes the most sense from schema
design and client perspective.
- Yaacov: I wanted to raise awareness of the RFCs. Surprised there aren’t more
people being excited about interfaces being a part of a union (?).
## Schema Metadata/Applied Directives (if time allows) (20m, Lee)
- [Discussion](https://github.com/graphql/graphql-wg/discussions/1096)
- Benjie: shared presentation:
- Lee: makes a lot of sense, but challenges the way I’ve been thinking about
introspection to this point. Introspection used by developers at development
time. Many people who use GraphQL block introspection externally. This
practice would break a lot of the use cases described here.
- Yaacov: would there be a way to manage permissions?
- Benjie: I don’t think there’s any desire for that.
- Michael S: Moving introspection into the application space. Would be more
involved in building your application.
- Benjie: Whatever we do to solve metadata my hope is that it’ll solve both of
these problems. I’m concerned that we’re moving towards a solution that will
limit the solution I’ve proposed here.
- Lee: though we may not want to make this the way we’d suggest users do this,
we may not want to prevent it.