Skip to content

Commit 79a6cea

Browse files
benjiebenjie
authored
Add notes for August WG (#1368)
1 parent 7c6cab4 commit 79a6cea

File tree

1 file changed

+156
-0
lines changed

1 file changed

+156
-0
lines changed

‎notes/2023/2023-08.md

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
# GraphQL WG Notes - August 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
7+
8+
Agenda:
9+
[https://github.com/graphql/graphql-wg/blob/main/agendas/2023/08-Aug/03-wg-primary.md](https://github.com/graphql/graphql-wg/blob/main/agendas/2023/08-Aug/03-wg-primary.md)
10+
11+
### Determine volunteers for note taking (1m)
12+
13+
- Benjie
14+
15+
### Action items:
16+
17+
- Will be closing a number of them in September, so please comment on them if
18+
they're still relevant
19+
20+
### CCN:
21+
22+
- Error handling and null propagation are the main concerns, but they seem like
23+
they're not specific to CCN alone - they have overlap with other proposals
24+
such as the fragment modularity proposals.
25+
- Our proposal is to strip the CCN proposal right back to just the `!` syntax
26+
and simple enforcing of non-nullability.
27+
- Reason for being here: gather feedback on the idea of stripping CCN back to
28+
the beginnings.
29+
- Anthony: there was a lot of discussion around the open issues, and came to a
30+
realization that a lot of this isn't tide to CCN and can be addressed in other
31+
ways. We're proposing keeping `!` but skipping the `?` for now, but I'm not
32+
sure there's consensus.
33+
- What we seemed to have consensus on was around the behaviors: removing the
34+
discussion around bubbling/catching/etc by just using the existing logic.
35+
- Calvin: RFC is just the `!` and the spec will follow. Lee's original reason
36+
for raising the `?` was to mirror the `!`, which we believe has value. We
37+
don't want to keep the "catching" behavior right now.
38+
- In my opinion `?` is best as a nullability modifier, not null propagation
39+
boundary.
40+
- Martin: I'm excited about `?` because it means we can add more `!` to the
41+
schema which is more pleasant by default; but I'm happy so long as we're not
42+
preventing it from a future update.
43+
- Anthony: we're now past the experimentation phase and we've learned a lot.
44+
Discussions are around the behaviors of edge cases, specifically when `?` just
45+
changes the nullability or whether it's as a catch for an error raised by a
46+
`!` (like a try/catch). The fact we couldn't come to a determination on this
47+
is what's encouraged us to drop the `?` for now and just keep the `!`
48+
modifier. My thought is that we could add an `@catch` directive or some other
49+
approach in future and just have `?` and `!` be simple modifiers leveraging
50+
the existing algorithms.
51+
- Ivan: I like the simplest approach with `!` and `?`, but one problem was that
52+
Relay will strip them and send the query to the server, the same will happen
53+
for a lot of proxies. If the proxy removes `?` before sending the query
54+
upstream that would mean the proxy would have to implement the semantics (and
55+
it can't?). `!` is stricter, but `?` cannot be stripped because the error may
56+
already have been thrown.
57+
- Anthony: so are you arguing for adding `?` from the start to help address this
58+
feature as early as possible?
59+
- Ivan: yeah, I think it's needed.
60+
- Someone talked about Relay stripping `!` and `?`
61+
- Benjie: I think Relay need to strip `!` because of the lack of fragment
62+
boundaries, but they wouldn't need to strip `?` I think?
63+
- Ivan: clients don't know if server supports the syntax
64+
- Benjie: yes, the SDL and introspection must indicate that the syntax is
65+
supported.
66+
- Martin: can we handle this in the upstream schemas by making everything
67+
nullable?
68+
- Ivan: I don't think that's feasible.
69+
- Ivan: I think feature detection could be a solution. Proxy wouldn't support
70+
`?` until every downstream schema supported `?`.
71+
- Anthony: I'm concerned about large stacks of feature flags. Is it better to
72+
version the schema? That way Oct2023+ with CCN would always support the syntax
73+
- Benjie: there's a few reasons not to do this, but the biggest is that it would
74+
prevent MAYs being indicated. I think these feature flags would only be
75+
required where things cannot be detected in other ways: types, fields or
76+
directives on the introspection schema.
77+
- Michael: I agree, we also have to deal with servers that don't allow aliasing!
78+
- Benjie: I think we MUST have it indicated in SDL and introspection in order
79+
for clients to support code generation, or editors like VSCode to know whether
80+
the `!` or `?` is specifically allowed or not.
81+
- Alex: I disagree. If I need a new thing in GraphQL, I ask the guy next to me
82+
and then I implement it on my client. It's not that complicated - I just ask
83+
the guy if CCN is supported before I try and use it. It seems unnecessary to
84+
block all clients/servers from using CCN just to support these big complex
85+
clients/servers.
86+
- Ivan: Proxies can strip `!` before sending to upstream, so no need for feature
87+
probing. Not the same for `?` though. So `?` is blocked until we can indicate
88+
feature flags, but I don't think `!` is.
89+
- Anthony: whether or not clients can work with specific servers in your
90+
company; the spec states "this is valid legal syntax" and having a client use
91+
that and the server reject it (because it doesn't support it) seems like a
92+
violation of the spec.
93+
- Benjie: this would also be needed for fragment arguments.
94+
- Matt: I don't mind if the server doesn't support it - it can just fail.
95+
- Ivan: GraphQL is like TypeScript: if you write something you get immediate
96+
feedback if it's valid or not, but you don't get that in JS or Python. You
97+
shouldn't need to run the query to know if it will work, but it's a developer
98+
experience thing.
99+
- Matt: it's not a blocker for getting it into GraphQL.js; it's only a blocker
100+
for getting it into the spec.
101+
- Ivan: strippable features and non-strippable features are different.
102+
- Matt: a client could compile (maybe) the query to remove all the fragment
103+
arguments.
104+
105+
### SDL Omissions (Martin)
106+
107+
- Working on Apollo Kotlin. Widely used.
108+
- User writes queries in `.graphql` files, and puts the schema into the codebase
109+
(`.graphqls`). When we do so, we get errors:
110+
`'include' type tried to redefine existing directive 'include' type`.
111+
- This is because IntelliJ assumes that the SDL will come without the built in
112+
directives, without the introspection types, etc.
113+
- We need this information at Apollo for good reasons, but some tooling requires
114+
that it's not present.
115+
- The spec states that the built-in scalars MUST be omitted for brevity from the
116+
SDL.
117+
- We have the same but MAY for directives.
118+
- But we don't have the same for introspection types.
119+
- I propose a small edit so that tools like IntelliJ will know to handle this
120+
situation.
121+
- Matt: either you include none, or you include ALL. This would allow you to
122+
have a GraphQL server that doesn't support Floats.
123+
- Matt: this would help with the `@specifiedDirective`
124+
- Benjie: String _has_ to be supported (for the introspection schema to work),
125+
so we could say that if String was present then you've opted into the verbose
126+
SDL format. Currently the introspection types don't appear because `__schema`
127+
and `__type` don't get returned from introspection, and thus there's no
128+
entrypoint to them in the SDL.
129+
- Ivan: do we have to print `__typename` everywhere though?
130+
- Ivan: we could have explicit fields, implicit printable fields, and implicit
131+
non-printable fields.
132+
- Matt: Relay has added `__id` field which normally aliases to `id` but not for
133+
special things. The `__id` is also implicit. We have a cacheKey type field and
134+
a fetchKey field which aren't necessarily the same (e.g. fetchKey might
135+
require hundreds of bytes).
136+
- Ivan: can we merge this as is? Otherwise everything is snowballing!
137+
- Benjie: I think it's a MUST that the introspection types are not included in
138+
this proposal, but we should have a separate RFC that allows printing the full
139+
schema including introspection fields, built-in scalars, etc. I think there'll
140+
be a lot of prose in this latter one but not a lot of algorithm changes.
141+
- Ivan: GraphQL.js `buildSchema` ignores the introspection types and just adds
142+
our own, but really there should be two functions, one for building a client
143+
schema that has everything the SDL indicates and nothing more.
144+
- Even if we change the spec and validation, the implementations would still
145+
need to have additional changes.
146+
- Martin: the reason we need the full SDL is so we can tell if the server
147+
supports certain introspection fields.
148+
- Matt: path 1 is to have a full description of the schema that we can validate
149+
against. Path 2 is "for every new feature in the spec we add something to
150+
SDL/introspection to indicate this".
151+
- Ivan: feature flag for syntax is easy because there's no interdependency.
152+
- Michael: can you not solve this by using a config file based on the features
153+
detected. We use a heavily annotated schema where we add schema directives to
154+
indicate which features were supported.
155+
- Martin: every implementation would then have it's own directives potentially,
156+
whereas SDL is standard and well defined.

0 commit comments

Comments
 (0)