Spec edits for incremental delivery, Section 3 & 7 only #1124

robrichard · 2024-11-01T15:42:47Z

Extracted from the full PR (#1110) and targeting an integration branch to aid in review.

Helpful reference material:

Response format examples: New response format (June 2023) defer-stream-wg#69
GraphQL Conf talk: https://www.youtube.com/watch?v=LEyDeNoobT0

Keweiqu

Thank you so much @robrichard for this amazingly written edits! What a great progress!! See my inline comments.

And let me know if you want me to raise the error handling as a separate PR or a separate topic in WG discussion.

spec/Section 3 -- Type System.md

Keweiqu · 2024-11-07T19:37:39Z

spec/Section 7 -- Response.md

+last response of the stream. This entry must not be present for GraphQL
+operations that return a single response map.


"This entry must not be present for GraphQL operations that return a single response map"

Why? Seems like an unnecessary restriction.

We discussed this some time ago and landed on this: graphql/defer-stream-wg#57 (comment)

I think the main reason was a fear that clients may start to depend on hasNext: false being present in a single payload response, and this would break if they switch to a server that does not support defer/stream

Ohhhhh yeah now I remember!

I think the wording isn't quite right though. I think if the request uses @defer/@stream then it's okay to have hasNext: false but if the request did not use @defer or @stream then the result must not include hasNext. Does that differ from what we discussed before?

My intention was that servers should never include hasNext: false in a single payload response, even if a request uses @defer/@stream but the server did not defer anything. I agree that in this case we don't have the same risk of allowing it in requests without @defer/@stream, but it seems simpler to me to not make this distinction.

Keweiqu · 2024-11-07T19:45:22Z

spec/Section 7 -- Response.md

+Clients should expect the the GraphQL Service to incrementally deliver the
+remainder of the fields contained in the deferred fragment.


there is some ambiguity with this statement. When I read this it seems like with a fragment X {a, b, c} which is deferred, the GraphQL service can incrementally deliver {a, b} and then deliver {c}

I agree that it is ambiguous. I wrote this way because I was thinking that your example is technically possible. If a and b are in both a deferred and non-deferred fragment, a & b will be in the initial response and only c will be incrementally delivered. Or if a and b are in both deferred fragment X and deferred fragment Y, a & b might be delivered incrementally when fragment Y is complete, while c will be sent in a later payload.

Do you have a suggestion to clarify?

Keweiqu · 2024-11-07T19:49:11Z

spec/Section 7 -- Response.md

+The Incremental Object Result's `path` can be determined using the prior Pending
+Result with the same `id` as this Incremental Result. The Incremental Object
+Result may contain a `subPath` entry. If the `subPath` entry is present, The
+Incremental Object Record's path can be determined by concatenating the Pending
+Result's `path` with this `subPath`. If no `subPath` entry is present, the path
+is the same as the Pending Result's `path`.
+


what about index? Do you want me to raise this as a separate PR?

When we switched to the latest response format we decided not to put index in the incremental object. It's not strictly necessary since list items must be delivered in order. We were thinking it could be added in a future spec update if we want to support out of order or sparse responses. If this is an issue for you can you start a new discussion topic in https://github.com/graphql/defer-stream-wg/discussions?

spec/Section 7 -- Response.md

robrichard · 2024-11-08T17:00:00Z

@Keweiqu thanks for your review! I have addressed your comments and also moved the examples into a new appendix as discussed during the working group meeting.

martinbonnin · 2024-11-19T23:11:43Z

spec/Section 3 -- Type System.md

+GraphQL implementations are not required to implement the `@defer` and `@stream`
+directives. If either or both of these directives are implemented, they must be
+implemented according to this specification. GraphQL implementations that do not
+support these directives must not make them available via introspection. The


Can we clarify what is meant by implementation here? My current understanding is that an "implementation" is typically a library, such as graphql-js or graphql-java while a "service" is typically a server or a collection of servers, such as the GitHub or Shopify APIs.

If that's the case, the "not required" part could be confusing. If I'm writing the graphql-cobol library, can I decide to leave @defer out and still claim compatibility with the spec?

@martinbonnin thanks for reviewing. I updated this to say services instead of implementations as I was referring to servers here

Thanks for the clarification 🙏

Would it make sense to use language similar to @specifiedBy and @oneOf:

GraphQL implementations that support the type system definition language should provide the @defer directive if representing custom scalar definitions.

This is the closest to feature discovery we have in GraphQL and using common language pattern would make the spec more palatable. Or is there something fundamentally different about @defer and @stream?

I'm not sure this applies to @defer and @stream, because they are directives that apply to "executable locations", whereas @specifiedBy and @oneOf only apply to "type system location".

I think a GraphQL server could not support the "type system definition language" but still support @defer and @stream. Clients could determine if @defer and @stream are supported by using the introspection query:

query { __schema { directives { name } } }

I think the inverse could also be true. A GraphQL Server that supports the type system definition language may have no desire to support @defer/@stream and that's ok since we want this feature to be strictly optional.

It's possible I'm not thinking about this correctly, let me know what you think.

Apologies in advance, a bunch of nitpicking on the vocabulary/glossary is coming below.

a GraphQL server could not support the "type system definition language" but still support @defer

I don't think it makes sense for a GraphQL server to support the "type system definition language". A server reads request in and writes responses out. SDL is an implementation detail for the server.

But maybe not for its implementation (==lib?). A lib takes SDL in and generates a compliant service out of it. I always thought of implementation in the spec as being the "lib" but re-reading it, I'm not sure any more.

Regardless, if we use this service vs implementation distinction, I would rephrase like so:

A GraphQL implementation that does not support the "type system definition language" can still support @defer and @stream Clients can determine if @defer and @stream are supported by using the introspection query against a service. A GraphQL service may have no desire to support @defer/@stream and that's ok since we want this feature to be strictly optional.

I think this is the same for @oneOf and @specifiedBy:

A GraphQL implementation that does not support the "type system definition language" can still support @oneOf and @specifiedBy Clients can determine if @oneOf and @specifiedBy are supported by using the introspection query against a service. A GraphQL service may have no desire to support @oneOf and @specifiedBy and that's ok since we want this feature to be strictly optional.

That's why I'm tempted to put them in the same bucket of features even if @defer is obviously quite important for clients to be aware of.

@martinbonnin I am definitely open to changing this, just want to talk it through to make sure I fully understand.

Is this statement true for @oneOf and @specifiedBy?

A GraphQL implementation that does not support the "type system definition language" can
still support @oneOf and @specifiedBy

Since @oneOf and @specifiedBy are directives that can only be applied to grammar that is part of the "type system definition language", how can an implementation support this directive without supporting the grammar that is required to place the directive? I agree that specifiedByURL introspection field may be supported but this is not the same as the directive.

The same is not true for @defer and @stream because these directives are applied to the execution grammar.

A GraphQL service may have no desire to support @oneOf and @specifiedBy and that's ok since we
want this feature to be strictly optional.

Again, I could be wrong about this but my understand for @specifiedBy is that ideally all GraphQL servers would support it, but that's not possible for servers written before they were added to the spec, so RFC 2119 should is used. Additionally, we further restrict it to "implementations that support the type system definition language" since @specifiedBy can only be placed in the type system grammar.

For @defer and @stream we don't want to use RFC 2119 should/recommended language since it is optional and we expect some servers will choose not to implement it for reasons other than being behind on the latest spec changes.

my understand for @SpecifiedBy is that ideally all GraphQL servers would support it, but that's not possible for servers written before they were added to the spec, so RFC 2119 should is used.
[...]
For @defer and @stream we don't want to use RFC 2119 should/recommended language since it is optional and we expect some servers will choose not to implement it for reasons other than being behind on the latest spec changes.

Gotcha, thanks! So we've got:

RFC 2119 must for stuff that all services must support (@include for an example)

RFC 2119 should to allow older services to still be compatible but newer services should support (@specifiedBy for an example)

explicit language like here for stuff that is completely optional (@defer for an example)

Did I get that right?

If yes, I was missing the 2. vs 3. distinction, thanks for pointing that out 🙏

I'm still not 100% sold on it though. I kind of like that other directives all use the same sentence and feels a bit weird that @defer is not following the pattern (or maybe it's just my OCD...).

What about using RFC 2119 may to denote that it's "more" optional than @specifiedBy? Was that ever mentioned?

:: A _built-in directive_ is any directive defined within this specification. GraphQL implementations should provide the `@skip` and `@include` directives. GraphQL implementations that support the type system definition language must provide the `@deprecated` directive if representing deprecated portions of the schema. GraphQL implementations that support the type system definition language should provide the `@specifiedBy` directive if representing custom scalar definitions. GraphQL implementations that support the type system definition language may provide the `@defer` directive if they support returning response streams. GraphQL implementations that support the type system definition language may provide the `@stream` directive if they support incrementally delivered results.

I don't want to bikeshed this too much especially if it was discussed already. Is there any reading material and/or pointers I could catch up on?

A GraphQL implementation that does not support the "type system definition language" can
still support @OneOf and @SpecifiedBy

Since @OneOf and @SpecifiedBy are directives that can only be applied to grammar that is part of the "type system definition language", how can an implementation support this directive without supporting the grammar that is required to place the directive?

It can't but it's fine?

My reading of A GraphQL implementation that does not support the "type system definition language" is a code-first GraphQL implementation like graphql-kotlin for an example. It has nothing to do with supporting the grammar or anything like this. A code-first implementation can implement @specifiedBy.

Supporting the "type system definition language" is a feature of some GraphQL libs that has nothing to do with how the grammar is internally implemented.

I read all of these sentences as: "if a lib reads a SDL written by a user, then the lib must/should/may provide the built in directive definitions":

user SDL + built-in directives == final schema

The same is not true for @defer and @stream because these directives are applied to the execution grammar.

The way I see it the directive location (executable or type system) is irrelevant. These sentences are about the directive definitions that must/should/may be added to the resulting schema.

But again this is my very personal understanding of it and maybe PTSD from the full schemas discussion. Thanks for diving into this with me!

@martinbonnin I agree that we should probably rewrite this to use RFC 2119 may or optional to clearly state that it's an optional feature instead of recommended.

I still don't think we should include that support the type system definition language for anything related to @defer or @stream because they are features that are unrelated to the SDL.

I think a GraphQL server that creates everything programmatically fits the definition of an implementation that does not support the type system definition. My interpretation is that the spec is not providing any sort of guidance for the @specifiedBy directive for this type of implementation since it is excluded by the phrase GraphQL implementations that support the type system definition language ....

But for @defer and @stream it is important that this type of implementation follow this guidance regardless of SDL support so we should not include the same phrase.

I still don't think we should include that support the type system definition language for anything related to @defer or @stream because they are features that are unrelated to the SDL.

I see it now! Thanks for explaining 🙏

My interpretation is that the spec is not providing any sort of guidance for the @SpecifiedBy directive for this type of implementation since it is excluded by the phrase GraphQL implementations that support the type system definition language ...

Agreed here as well 👍 But I also think this may be a problem?

Do we have somewhere in the spec where we say what built-in directives are required in a valid full/final schema and which ones are not? I initially thought this was the place but since it excludes code-first implementations it can't reasonnably be it?

But for @defer and @stream it is important that this type of implementation follow this guidance regardless of SDL support so we should not include the same phrase.

Right 👍 I actually like the sentence as it is now because it applies to services. I think using service here is correct since SDL support is irrelevant for a service.

Finally, I'd say there should be a similar sentence for other directives? @include/@skip come to mind for an example because they are used in executable locations. Can a service ignore them? But I would probably do it for all directives for symmetry and consistency reasons.

@include/@skip come to mind for an example because they are used in executable locations.

For @include/@skip, the spec does have the following sentence at the beginning of this section

GraphQL implementations should provide the `@skip` and `@include` directives.

Do we have somewhere in the spec where we say what built-in directives are required in a valid full/final schema and which ones are not? I initially thought this was the place but since it excludes code-first implementations it can't reasonnably be it?

I do think this is the correct place, and with the above sentence for @skip and @include we are covered for all SDL and non-SDL directives. But even @skip and @include are not required since the spec is using RFC 2119 should.

My interpretation is that the spec is not providing any sort of guidance for the @specifiedBy directive for this type of implementation since it is excluded by the phrase GraphQL implementations that support the type system definition language ...

Agreed here as well 👍 But I also think this may be a problem?

Perhaps! Maybe it's worth a WG discussion to clarify?

benjie

I did not have time to review Appendix C and my review of section 7 was a bit rushed, but hopefully this feedback is helpful 🤞

spec/Section 3 -- Type System.md

benjie · 2024-11-28T14:25:34Z

spec/Section 3 -- Type System.md

+that ignores the `@defer` and/or `@stream` directives. This also applies to the
+`initialCount` argument on the `@stream` directive. Clients _must_ be able to
+process a streamed response that contains a different number of initial list
+items than what was specified in the `initialCount` argument.


Interesting, I didn't realise we had applied this to initialCount - I thought it was either non-deferred (i.e. give you the whole list) or deferred with initialCount supplied.

I think we should revisit this discussion, it's quite different to the general "don't defer" and "don't stream" optimizations in my mind - specifically if you specify initialCount: 2 I'd argue that at least 2 results should be supplied, or the entire thing should not be deferred (e.g. if there are fewer than 2 results). Skipping @defer / @include result in the client getting more data up front, but ignoring initialCount allows for the client to get less data up front, and that's a problem to my mind.

Happy to revisit this discussion. If I am remembering correctly, the arguments for this version were:

We are still "highly recommending" servers send the correct number of list items earlier in this paragraph

This allows the most flexibility, clients shouldn't make any assumptions about the length of the initial list

From a type perspective clients should already not be making assumptions about the length of the list

benjie · 2024-11-28T14:29:04Z

spec/Section 7 -- Response.md

+last response of the stream. This entry must not be present for GraphQL
+operations that return a single response map.


I think the wording isn't quite right though. I think if the request uses @defer/@stream then it's okay to have hasNext: false but if the request did not use @defer or @stream then the result must not include hasNext. Does that differ from what we discussed before?

benjie · 2024-11-28T14:31:37Z

spec/Section 7 -- Response.md

+### Path
+
+A `path` field allows for the association to a particular field in a GraphQL
+result. This field should be a list of path segments starting at the root of the
+response and ending with the field to be associated with. Path segments that
+represent fields should be strings, and path segments that represent list
+indices should be 0-indexed integers. If the path is associated to an aliased
+field, the path should use the aliased name, since it represents a path in the
+response, not in the request.
+
+When the `path` field is present on an "Error result", it indicates the response
+field which experienced the error.
+


Should we extract this and line 136/137 into a separate PR to merge sooner?

Opened #1129

spec/Section 7 -- Response.md

benjie · 2024-11-28T14:41:18Z

EDITED: My previous comment was incorrect, it's been edited here. Please see the edit history to make sense of the next couple comments.

One comment on this is that it quite often talks about multiple "responses", but I don't think that's really accurate. Subscriptions are the closest match, and they have a "response stream" which is a stream of responses; but the incremental payloads don't conform to response. I've put "payloads" in my edits, but I think the terminology we use here should be explored.

I noticed in section 7 you've referred to "result" as well. I think a "incremental result stream" consisting of multiple "payloads" of which the first is a "GraphQL response" payload and the remainder are "incremental result" payloads makes sense.

martinbonnin · 2024-11-28T15:04:51Z

One comment on this is that it quite often talks about multiple "responses", but I don't think that's really accurate.

Great point about terminology. I'm taking this opportunity to promote the Glossary. Whatever we decide we should add there.

Subscriptions are the closest match, and they have a single response which is a "response stream" which has _yields "events"

The spec can allow both interpretations I think?

A GraphQL service generates a response from a request via execution.

But then later on, in the Subscribe algorithm:

Return responseStream.

But then in the response format:

A response to a GraphQL request must be a map.

Can a response stream be a map? 🤔 Oh well...

benjie · 2024-11-28T15:07:24Z

Can a response stream be a map? 🤔 Oh well...

Now I come to think of it, subscriptions returns a stream of responses - each event on the stream causes an entire GraphQL response ({data, errors} - which is a map) to be produced; whereas defer/stream only produce one "response" payload, but then that's followed by incremental result payloads which are explicitly not conformant to a GraphQL response.

benjie · 2024-11-28T15:18:09Z

@martinbonnin I've edited quite heavily this comment above - WDYT?

martinbonnin · 2024-11-28T15:39:58Z

@benjie I agree that response is mostly associated with {data, errors} today and we should keep this as much as possible. Looks like we should extend it to account for {data, errors, pending, hasNext} with this proposal.

Re: payload, it's used pervasively in the Susbscription world where it's used either as generic data to be interpreted according to type (see graphql-ws) or as execution result (==response) (see graphql-sse) so agree we should keep payload as something holding data without mandating that this data has a specific shape.

I like incremental result but that introduces "result" in the mix. What's a non-incremental "result"? a response? If yes, then why not going with incremental response? Because an incremental response is not a response...etc.. naming is hard!

robrichard · 2024-12-03T17:36:36Z

@benjie @martinbonnin Regarding naming, I went back and forth on this many times and am still not totally confident the names I've chosen are best.

I've been using Response to refer to the entire stream, and payload to refer to each object that is sent in the stream. I think the distinction between subscriptions is important, so a query/mutation with defer/stream has one response with multiple payloads, while subscriptions have multiple responses.

I'm also using result to refer to the objects inside the pending, incremental, and completed arrays, i.e. Pending Result, Incremental Result, Completed Result. However, I noticed the spec seems to use Response and Result interchangeably:

The data entry in the response will be the result of the execution of the
requested operation ....
If an error was raised before execution begins, the data entry should not be
present in the result.

So maybe these should be renamed to Pending Object, Incremental Object, Completed Object or something else?

benjie · 2024-12-06T09:12:42Z

However, I noticed the spec seems to use Response and Result interchangeably:

I think the spec uses "result" very generically - it can be the result of anything. The GraphQL response is the result of a GraphQL request, but there's also results from fields, results from coercion, etc etc. I've raised #1130 to clarify the example you raised.

I've been using Response to refer to the entire stream

Currently I think the spec fairly universally uses the term "response" to mean the "data and/or errors" object that you get either for a query/mutation, or as the result of each individual event in a subscription.

I think the distinction between subscriptions is important, so a query/mutation with defer/stream has one response with multiple payloads, while subscriptions have multiple responses.

This is an important distinction and it helps to clarify that the intent of incremental/stream is effectively a "snapshot in time" rather than being another way of doing subscriptions.

Maybe instead of "response" being one thing, we should think of what we refer to as "response" currently actually being the standard response payload. Incremental delivery would then return an incremental response stream consisting of an initial response payload followed by one or more subsequent response payloads. We could trim the word "payload" from each of these. We would end up with:

query / mutation (without incremental) - yields a single "response"
subscription - yields a response stream, a stream of "response"s

A "response" is either:

a "standard response"
- data and/or errors
or an "incremental stream"
- a stream consisting of:
  - an "initial response"
    - (data and/or errors, pending, hasNext:true)
  - followed by one or more "subsequent response" (or maybe "continuation response"?)
    - (incremental and/or errors, pending?, hasNext)

This also factors in that subscriptions could, in future, also support stream/defer - their "response" is itself an "incremental stream".

I'm also using result to refer to the objects inside the pending, incremental, and completed arrays, i.e. Pending Result, Incremental Result, Completed Result.

This seems fine for now.

BoD · 2024-12-11T10:21:37Z

spec/Section 3 -- Type System.md

+) on FIELD
+```
+
+The `@stream` directive may be provided for a field whose type incorporates a


The same note should be there for the @defer section above

BoD · 2024-12-11T10:39:33Z

spec/Section 7 -- Response.md

+Service to incrementally deliver the remainder of indicated list field. When the
+Pending Result is associated with a `@defer` directive, it indicates that the
+response fields contained in the deferred fragment are not known to be complete.
+Clients should expect the the GraphQL Service to incrementally deliver the


Suggested change

Clients should expect the the GraphQL Service to incrementally deliver the

Clients should expect the GraphQL Service to incrementally deliver the

BoD · 2024-12-11T13:16:05Z

spec/Section 7 -- Response.md

+Result's `path` with this `subPath`. If no `subPath` entry is present, the path
+is the same as the Pending Result's `path`.
+
+Every Incremental Object Result must contain an `data` entry. The `data` entry


Suggested change

Every Incremental Object Result must contain an `data` entry. The `data` entry

Every Incremental Object Result must contain a `data` entry. The `data` entry

BoD · 2024-12-11T13:21:04Z

spec/Section 7 -- Response.md

+Result may contain a `subPath` entry. If the `subPath` entry is present, The
+Incremental Object Record's path can be determined by concatenating the Pending


Suggested change

Result may contain a `subPath` entry. If the `subPath` entry is present, The

Incremental Object Record's path can be determined by concatenating the Pending

Result may contain a `subPath` entry. If the `subPath` entry is present, the

Incremental Result's path can be determined by concatenating the Pending

BoD · 2024-12-11T13:26:19Z

spec/Section 7 -- Response.md

+GraphQL result. The Incremental Object Result's `path` will contain the path
+segments of the field this data is associated with.


Suggested change

GraphQL result. The Incremental Object Result's `path` will contain the path

segments of the field this data is associated with.

GraphQL result. The Incremental Object Result's path will contain the path

segments of the field this data is associated with.

May better convey that path is not a field in the result, but is found in the corresponding pending result.

BoD · 2024-12-11T13:30:16Z

spec/Section 7 -- Response.md

+An Incremental Object Result's data may contain response fields that were
+contained in more than one deferred fragments. In that case, the `id` of the
+Incremental Object Result must point to the Pending Result that results in the
+shortest `subPath`.


Not 100% sure, but should it be "the shortest path" instead?

BoD · 2024-12-11T16:37:45Z

spec/Section 3 -- Type System.md

+#### @defer Arguments
+
+- `if: Boolean! = true` - When `true`, fragment _should_ be deferred (see
+  related note below). When `false`, fragment will not be deferred. Defaults to


Should it be must not be deferred?

Spec edits for incremental delivery, Section 3 & 7 only

abafb76

robrichard force-pushed the incremental-integration-response branch from 6734003 to abafb76 Compare November 1, 2024 15:45

Keweiqu reviewed Nov 7, 2024

View reviewed changes

robrichard added 2 commits November 8, 2024 11:47

Move examples to Appendix

0310656

PR feedback

fcf898f

robrichard force-pushed the incremental-integration-response branch from 53cc60e to fcf898f Compare November 8, 2024 16:54

martinbonnin reviewed Nov 19, 2024

View reviewed changes

defer/stream is optional for GraphQL services

ffb8cad

benjie requested changes Nov 28, 2024

View reviewed changes

robrichard added 4 commits December 2, 2024 15:51

PR feedback

c7c3ee1

more PR feedback

2934a59

more PR feedback

5d72416

bubble -> propagate

03adfb9

robrichard force-pushed the incremental-integration-response branch from b6c35d7 to 03adfb9 Compare December 3, 2024 15:18

benjie mentioned this pull request Dec 6, 2024

Editorial: data and errors appear in the "response" #1130

Merged

BoD reviewed Dec 11, 2024

View reviewed changes

		last response of the stream. This entry must not be present for GraphQL
		operations that return a single response map.

		Clients should expect the the GraphQL Service to incrementally deliver the
		remainder of the fields contained in the deferred fragment.

	Clients should expect the the GraphQL Service to incrementally deliver the
	Clients should expect the GraphQL Service to incrementally deliver the

	Every Incremental Object Result must contain an `data` entry. The `data` entry
	Every Incremental Object Result must contain a `data` entry. The `data` entry

		Result may contain a `subPath` entry. If the `subPath` entry is present, The
		Incremental Object Record's path can be determined by concatenating the Pending

		GraphQL result. The Incremental Object Result's `path` will contain the path
		segments of the field this data is associated with.

Spec edits for incremental delivery, Section 3 & 7 only #1124

Are you sure you want to change the base?

Spec edits for incremental delivery, Section 3 & 7 only #1124

Conversation

robrichard commented Nov 1, 2024 • edited Loading

Keweiqu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrichard commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjie commented Nov 28, 2024 • edited Loading

martinbonnin commented Nov 28, 2024

benjie commented Nov 28, 2024 • edited Loading

benjie commented Nov 28, 2024

martinbonnin commented Nov 28, 2024

robrichard commented Dec 3, 2024

benjie commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrichard commented Nov 1, 2024 •

edited

Loading

benjie commented Nov 28, 2024 •

edited

Loading

benjie commented Nov 28, 2024 •

edited

Loading