Skip to content

Commit 5b44894

Browse files
authored
Add notes from 2023-02-08 secondary (APAC) meeting (#1265)
1 parent 694cf62 commit 5b44894

File tree

1 file changed

+119
-0
lines changed

1 file changed

+119
-0
lines changed

notes/2023/2023-02.md

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,3 +192,122 @@
192192
of edge cases.
193193
- Lee: we should be clear about this because the difference is subtle.
194194
- **ACTION** - Matt / Yaacov - tag Lee at the relevant line of code to review.
195+
196+
# Secondary (APAC)
197+
198+
## Agenda
199+
200+
- Agree to Membership Agreement, Participation & Contribution Guidelines and
201+
Code of Conduct (1m, Lee)
202+
- [Specification Membership Agreement](https://github.com/graphql/foundation)
203+
- [Participation Guidelines](https://github.com/graphql/graphql-wg#participation-guidelines)
204+
- [Contribution Guide](https://github.com/graphql/graphql-spec/blob/main/CONTRIBUTING.md)
205+
- [Code of Conduct](https://github.com/graphql/foundation/blob/master/CODE-OF-CONDUCT.md)
206+
- Introduction of attendees (5m, Lee)
207+
- Determine volunteers for note taking (1m, Lee)
208+
- Review agenda (2m, Lee)
209+
- Review prior primary meeting (5m, Lee)
210+
- [WG primary](https://github.com/graphql/graphql-wg/blob/main/agendas/2023/02-Feb/02-wg-primary.md)
211+
- Review previous meeting's action items (5m, Lee)
212+
- [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)
213+
- [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)
214+
- [All open action items (by meeting)](https://github.com/graphql/graphql-wg/projects?query=is%3Aopen+sort%3Aname-asc)
215+
- Fix ambiguity around when schema definition may be omitted (10m, Benjie)
216+
- [RFC](https://github.com/graphql/graphql-spec/pull/987) - currently stage 1,
217+
looking to advance
218+
- [GraphQL.js implementation](https://github.com/graphql/graphql-js/pull/3839)
219+
- Fragment Arguments (10m, Matt)
220+
- [graphql-js PR](https://github.com/graphql/graphql-js/pull/3835) is waiting
221+
for review
222+
- Do we need a flag? It's difficult to evaluate usage without graphql-js
223+
syntax support: can we can merge as-is without a flag?
224+
- [Relay has PR](https://github.com/facebook/relay/pull/4214/commits/104d1eec479b4be9b378542a246c2d5159efa2cc)
225+
enables the new syntax over directives
226+
- Blocker for Relay adoption within Meta is graphql-js, and therefore
227+
prettier, syntax support.
228+
229+
## Determine volunteers for note taking (1m, Lee)
230+
231+
- Benjie
232+
233+
## Review agenda (2m, Lee)
234+
235+
- Looks like a short one, so I can use remaining time to address the GraphiQL
236+
publishing permissions issues.
237+
238+
## Review prior primary meeting (5m, Lee)
239+
240+
- [WG primary](https://github.com/graphql/graphql-wg/blob/main/agendas/2023/02-Feb/02-wg-primary.md)
241+
242+
## Review previous meeting's action items (5m, Lee)
243+
244+
- [skipped]
245+
246+
## Fix ambiguity around when schema definition may be omitted (10m, Benjie)
247+
248+
- [RFC](https://github.com/graphql/graphql-spec/pull/987) - currently stage 1,
249+
looking to advance
250+
- [GraphQL.js implementation](https://github.com/graphql/graphql-js/pull/3839)
251+
- [summary]
252+
- Have you had a chance to look at it in GraphQL Java?
253+
- Donna: yes, I had a look earlier and there is indeed an issue where the schema
254+
appears to have a Mutation type when it should not; we'll fix it.
255+
- Great! Basically ready for RFC2, the only issue is there's a test in
256+
GraphQL.js that I don't know why it exists and I had to do a special case
257+
handling for no root operation types in printSchema to handle it.
258+
- Issue is that if you have a schema consisting of the type
259+
`type Query {foo: String}` but that is _not_ the root query type (the schema
260+
is invalid and has no operation types) then when you print it, either it looks
261+
valid (because `Query` is a default operation type name) or you print it as
262+
`schema{} type Query{foo: String}` but that cannot be parsed. I worked around
263+
it, but I'm not sure why this test exists or what it's trying to enforce.
264+
- Matt: could be related to removing the query type?
265+
- Lee: we should make the print-parse-print loop consistent
266+
- Matt: it is, it'll take an invalid schema and make it valid but then it'll be
267+
consistent from there on out.
268+
- Matt: at Meta we split the schema types into different files and parse them
269+
into ASTs and merge them to build the schema.
270+
- [changed to Matt's topic, then circled back]
271+
- Benjie: thinking about this, you cannot add anything to the SDL
272+
`type Query{foo: String}` to make it have no root types.
273+
`type Query{foo: String} schema {}` won't currently parse. So this is an
274+
invalid schema that cannot be represented in SDL; when we attempt to represent
275+
it via SDL we get what appears to be a valid schema, and then the
276+
print-parse-print loop is happy and consistent. So I think this is the right
277+
solution.
278+
- Lee: I'm concerned about the `?? null` in the code, is that needed?
279+
- Benjie: yes, I think when you do `query.getQueryType()` it will return `null`
280+
if there is no query type, however if you do `query.getType('Query')` then it
281+
will return `undefined` (or maybe the other way around); so the `?? null`
282+
means we consistently deal with null which makes the later
283+
`queryOperationType === queryType` comparisons easier.
284+
- Benjie: is there an action for me to take at this point, or do we just need
285+
people to review it?
286+
- Lee: I'll spend some time reviewing it.
287+
288+
## Fragment Arguments (10m, Matt)
289+
290+
- [graphql-js PR](https://github.com/graphql/graphql-js/pull/3835) is waiting
291+
for review
292+
- Do we need a flag? It's difficult to evaluate usage without graphql-js
293+
syntax support: can we can merge as-is without a flag?
294+
- [Relay has PR](https://github.com/facebook/relay/pull/4214/commits/104d1eec479b4be9b378542a246c2d5159efa2cc)
295+
enables the new syntax over directives
296+
- Blocker for Relay adoption within Meta is graphql-js, and therefore
297+
prettier, syntax support.
298+
- No pushback from Relay so far.
299+
- Before any servers have the new syntax we will require the tooling to support
300+
the new syntax.
301+
- There's internal questions over how it works, but the syntax seems well
302+
established and no-one is questioning it.
303+
- Lee: once something is released, people will start using it and it removes the
304+
ability to iterate if need be.
305+
- Matt: might make sense to change the name of the flag ("fragment variables").
306+
Parser would just not understand things unless it has a flag. Alternatively
307+
the flag could be in the validation rather than the parser.
308+
- Lee: keeping the flag but renaming it seems reasonable; enables you to write
309+
tests, test corner cases, Relay to use it, etc whilst still ensuring we can
310+
change our minds if we need to.
311+
- ACTION - Matt: rename the flag and add it to the parser (that's what we're
312+
doing with Relay anyway)
313+
- Matt: Main question remaining is around scoping behavior.

0 commit comments

Comments
 (0)