Skip to content

Commit b174772

Browse files
benjiebenjie
authored
Add 2023-01 WG notes (#1226)
1 parent 51d8c47 commit b174772

File tree

2 files changed

+212
-0
lines changed

2 files changed

+212
-0
lines changed

‎notes/2022/2022-12.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
# GraphQL WG Notes - December 2022
22

3+
**Watch the replays:**
4+
[GraphQL Working Group Meetings on YouTube](https://www.youtube.com/playlist?list=PLP1igyLx8foH30_sDnEZnxV_8pYW3SDtb)
5+
36
# Primary meeting
47

58
## Agenda

‎notes/2023/2023-01.md

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
# GraphQL WG Notes - Jan 2023
2+
3+
**Watch the replays:**
4+
[GraphQL Working Group Meetings on YouTube](https://www.youtube.com/playlist?list=PLP1igyLx8foH30_sDnEZnxV_8pYW3SDtb)
5+
6+
# Primary meeting
7+
8+
## Agenda
9+
10+
- Agree to Membership Agreement, Participation & Contribution Guidelines and
11+
Code of Conduct (1m, Lee)
12+
- [Specification Membership Agreement](https://github.com/graphql/foundation)
13+
- [Participation Guidelines](https://github.com/graphql/graphql-wg#participation-guidelines)
14+
- [Contribution Guide](https://github.com/graphql/graphql-spec/blob/main/CONTRIBUTING.md)
15+
- [Code of Conduct](https://github.com/graphql/foundation/blob/master/CODE-OF-CONDUCT.md)
16+
- Introduction of attendees (5m, Lee)
17+
- Determine volunteers for note taking (1m, Lee)
18+
- Review agenda (2m, Lee)
19+
- Review prior secondary meetings (5m, Lee)
20+
- [WG secondary APAC](https://github.com/graphql/graphql-wg/blob/main/agendas/2022/12-Dec/wg-secondary-apac.md)
21+
- [WG secondary EU](https://github.com/graphql/graphql-wg/blob/main/agendas/2022/12-Dec/wg-secondary-eu.md)
22+
- Review previous meeting's action items (5m, Lee)
23+
- [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)
24+
- [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)
25+
- [All open action items (by meeting)](https://github.com/graphql/graphql-wg/projects?query=is%3Aopen+sort%3Aname-asc)
26+
- Updates on
27+
[Fragment Arguments](https://github.com/graphql/graphql-js/pull/3152) (15m,
28+
Matt)
29+
- [Updated Spec PR to include spec text changes](https://github.com/graphql/graphql-spec/pull/865)
30+
- [Updated graphql-js with working implementation and validation](https://github.com/graphql/graphql-js/pull/3152)
31+
- Discuss whether Fragment Arguments PR is ready for
32+
[Stage 2: Draft](https://github.com/graphql/graphql-spec/blob/main/CONTRIBUTING.md#stage-2-draft).
33+
- Review of pending approved PRs (15m, Roman)
34+
- making this regular part of agenda
35+
- PRs ready to go:
36+
- [974](https://github.com/graphql/graphql-spec/pull/974),
37+
- [975](https://github.com/graphql/graphql-spec/pull/975),
38+
- [979](https://github.com/graphql/graphql-spec/pull/979),
39+
- [981](https://github.com/graphql/graphql-spec/pull/981);
40+
- Calling for action to:
41+
- [983](https://github.com/graphql/graphql-spec/pull/983),
42+
- [984](https://github.com/graphql/graphql-spec/pull/984),
43+
- [986](https://github.com/graphql/graphql-spec/pull/986)
44+
- Default Value Validation Status Check (15m, Yaacov)
45+
- [Lee's original PR stack](https://github.com/graphql/graphql-js/pull/3049)
46+
- [Now rebased on main](https://github.com/graphql/graphql-js/pull/3814)
47+
- [Reference from 2021](https://github.com/graphql/graphql-wg/blob/6cb3298d3a4e4667bd5f85da9e9676d407ba4e55/notes/2021/2021-09-02.md?plain=1#L123-L128)
48+
- Updates on defer/stream (15m, Rob)
49+
- [Proposal to merge deferred fragments](https://github.com/robrichard/defer-stream-wg/discussions/52#discussioncomment-4479271)
50+
51+
## Determine volunteers for note taking (1m, Lee)
52+
53+
- Benjie
54+
- Hugh
55+
56+
## Review agenda (2m, Lee)
57+
58+
- TSC nominations have closed; we’re going to kick off voting soon (virtually).
59+
60+
## Review prior secondary meetings (5m, Lee)
61+
62+
- [WG secondary APAC](https://github.com/graphql/graphql-wg/blob/main/agendas/2022/12-Dec/wg-secondary-apac.md)
63+
- Discussion around legals of GraphQL scalars project - action on Lee to
64+
advance
65+
- Discussions around Roman’s various PRs (more on that later)
66+
- [WG secondary EU](https://github.com/graphql/graphql-wg/blob/main/agendas/2022/12-Dec/wg-secondary-eu.md)
67+
- Validation of variables
68+
- Adding style guides to the spec
69+
- Another step forward on defer/stream; to be discussed further today
70+
71+
## Review previous meeting's action items (5m, Lee)
72+
73+
- [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)
74+
- [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)
75+
- [All open action items (by meeting)](https://github.com/graphql/graphql-wg/projects?query=is%3Aopen+sort%3Aname-asc)
76+
- None ready for review and packed agenda, so leaving for next time.
77+
78+
## Updates on [Fragment Arguments](https://github.com/graphql/graphql-js/pull/3152) (15m, Matt)
79+
80+
- [Updated Spec PR to include spec text changes](https://github.com/graphql/graphql-spec/pull/865)
81+
- [Updated graphql-js with working implementation and validation](https://github.com/graphql/graphql-js/pull/3152)
82+
- Discuss whether Fragment Arguments PR is ready for
83+
[Stage 2: Draft](https://github.com/graphql/graphql-spec/blob/main/CONTRIBUTING.md#stage-2-draft).
84+
- Matt: Fragments don’t give you a way to reuse them in different contexts -
85+
e.g. a fancy component in React could have N of my friends in a list. In
86+
GraphQL I either have to push the logic up to where the query is or hard-code
87+
a value to decide how many friends I want to show. Relay has
88+
non-spec-compliant directives (`@arguments(nFriends: 10)`) - types that can’t
89+
be described in GraphQL’s type system.
90+
- Desire is to bring fragment arguments into the spec to get rid of
91+
non-spec-compliant directives system from Relay. I’d have been happy with just
92+
adding the AST nodes, but pushback was that the executor should understand
93+
them, so this RFC addresses this.
94+
- Define fragment arguments in similar ways to variables definition
95+
(`fragment FriendsList($nFriends:Int!) …`), but when you call them it’s like
96+
arguments to a field `...FriendsList(nFriends: $someVar)`.
97+
- Lee: it might be easier to think of them as “fragment variables” rather than
98+
“fragment arguments” - they behave like variables inside the fragment.
99+
- Matt: when dealing with validation, I wanted the AST to be more like “input
100+
values” - these are almost identical to variable definitions, except they have
101+
no dollar.
102+
- Lee: it’s kind of both - an argument is a property of how it is called.
103+
“Fragment variables operate like operation variables; you provide operation
104+
variables by passing them in the GraphQL request, you provide fragment
105+
variables by passing as an argument to the fragment spread”
106+
- Matt: scope of fragment variables is local to the fragment only - they are not
107+
inherited by child fragment spreads; variables can be shadowed.
108+
- Lee: variables defined in fragment are considered first, in the operation
109+
later. We don’t need to track a stack of variables.
110+
- Matt: this also prevents independent fragments being used in the same document
111+
from introducing new constraints on those fragments.
112+
- **ACTION** - Matt: switch from “argument definitions” to “variable
113+
definitions” (using args at the call site still) in a stacked PR
114+
- Ivan: will you be adding a validation rule in Facebook to prevent shadowing?
115+
Also, consider using a different symbol such as `$$` to make it clear it’s
116+
fragment local and to allow consistent syntax.
117+
- Matt: it’s not shadowing _within_ a function [...] Within Meta we would not
118+
add this kind of validation.
119+
- Ivan: if you’re expecting some more validations to come, could you add them to
120+
the document?
121+
- Matt: I feel this is close to RFC 2 now.
122+
- Lee: I think you probably are at Stage 2. Would be good to separate out the
123+
RFC doc from the spec edits to keep the PR clean.
124+
- Looks good, choices seem reasonable, would be nice to list out a TODO list of
125+
the open questions that still need resolving.
126+
- Lee: not quite stage 2 because we’ve not resolved all the challenges, but
127+
we’re very close!
128+
- Hugh: Apollo Client would love to work with you on this
129+
- Rob: same fragment spread in the same place with different arguments would be
130+
a validation error?
131+
- [discussion - consensus: TBD]
132+
- Roman: we should be forgiving and accept what the programmer tells us, do our
133+
best to merge
134+
- Matt: we’ve found that it’s almost always unintentional with fields -
135+
typically when two teams add different
136+
- **ACTION** - Matt: figure out and document what all the challenges are that
137+
need to be resolved
138+
139+
## Review of pending approved PRs (15m, Roman)
140+
141+
- making this regular part of agenda
142+
- PRs ready to go:
143+
- [974](https://github.com/graphql/graphql-spec/pull/974),
144+
- [975](https://github.com/graphql/graphql-spec/pull/975),
145+
- [979](https://github.com/graphql/graphql-spec/pull/979),
146+
- [981](https://github.com/graphql/graphql-spec/pull/981);
147+
- Calling for action to:
148+
- [983](https://github.com/graphql/graphql-spec/pull/983),
149+
- [984](https://github.com/graphql/graphql-spec/pull/984),
150+
- [986](https://github.com/graphql/graphql-spec/pull/986)
151+
- Roman: having PRs open for a long time after being accepted is frustrating -
152+
can we review them each month and get them merged?
153+
- Lee: editorial PRs shouldn’t need to wait on me; if something is obviously
154+
ready to merge and non-controversial then TSC members should be able to merge
155+
them.
156+
- Roman: the other items I’d like to draw your attention to.
157+
- Ivan: for non-editorial changes, having a champion is a good thing, we
158+
shouldn’t change the process here.
159+
- Lee: if a TSC member approves something that they believe is editorial then
160+
they can merge it, if they think it needs more discussion they can ping TSC.
161+
- When you approve, add a line of text detailing what the next action should
162+
be: are you waiting on more approvals? Does Lee need to review it? Etc. At
163+
least this way it will be clear whose fault it is for not taking the next
164+
action.
165+
- It’s entirely reasonable to want two sets of eyes on each thing.
166+
167+
## Default Value Validation Status Check (15m, Yaacov)
168+
169+
- [Lee's original PR stack](https://github.com/graphql/graphql-js/pull/3049)
170+
- [Now rebased on main](https://github.com/graphql/graphql-js/pull/3814)
171+
- [Reference from 2021](https://github.com/graphql/graphql-wg/blob/6cb3298d3a4e4667bd5f85da9e9676d407ba4e55/notes/2021/2021-09-02.md?plain=1#L123-L128)
172+
- Yaacov: I’ve rebased all your hard work, Lee. Last discussed Sept 2021.
173+
- Benjie [chat]:
174+
[https://github.com/graphql/graphql-wg/issues/879](https://github.com/graphql/graphql-wg/issues/879)
175+
- Benjie: there’s also the spec work that I think needs to be re-edited to
176+
mirror what the JS implementation does because I think you changed approach?
177+
- Lee: it’s waiting on me then.
178+
- Ivan: we’ll try and extract any non-spec changes to make the PR smaller. Last
179+
time I tried to understand it, it was pretty complex. One thing to make the
180+
review easier might be to see what Andi did in GraphQL-Java - did he encounter
181+
any issues? Did he do the GraphQL.js code or the spec text?
182+
- Yaacov: schema coordinates are tied into that, is that ready to go?
183+
- Lee: hopefully they can be separated.
184+
185+
## Updates on defer/stream (15m, Rob)
186+
187+
- [Proposal to merge deferred fragments](https://github.com/robrichard/defer-stream-wg/discussions/52#discussioncomment-4479271)
188+
- Rob: we had another breakout meeting shortly before the holidays. We’re
189+
meeting again on Monday
190+
- Recap:
191+
- Clients being unable to tell if a fragment was deferred or not is still the
192+
main issue.
193+
- Generally people aren’t happy with labels, so we’re looking for ways to drop
194+
it, if possible.
195+
- In GraphQL we can branch execution on fields via aliases, but we don’t have
196+
that same option for fragments (at least not yet).
197+
- General consensus: non-aliased fragments that are deferred should be merged.
198+
- Lee: are you completely removing label, or just making it very optional?
199+
- Rob: dropping it entirely, and it can be solved either via fragment aliases or
200+
by aliasing the parent field.
201+
- Michael: label was never enough for what we wanted to do anyway, so it feels
202+
not right to have the label there.
203+
- Lee: we can always add functionality that results in multiple payloads instead
204+
of one - it’s always better to start with fewer payloads, simpler. In future
205+
we might consider priority and other options, so I’m in favor with this
206+
merging.
207+
- Michael: the aggressive merging feels more GraphQL-y
208+
- Rob: complexity is not fetching things again just because they’re deferred.
209+
- Rob: you mentioned “id”, Lee, I’m not totally sold in either direction.

0 commit comments

Comments
 (0)