RFC: GraphQL @defer
#3093
RFC: GraphQL @defer
#3093
Conversation
✅ Deploy Preview for apollo-ios-docs canceled.
|
|
This is ready for review and comment. The section on the generated models is the weakest of the RFC right now, I will put more time into that when I get back. If you have comments/ideas for that though please comment on it and I will address it. |
| .fragment(EntityFragment.self, deferred: true), | ||
| ] } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Just for a bit of context, here's how it works on Apollo Kotlin:
- fragments spread/inline fragments generate a dedicated field in the generated model
- before
@defer, this field could be nullable if:- its type condition means the fields won't always be returned
- or it has a
@skipor@include(unless it's trivial)
- after
@defer: now there's a 3rd case where this field can be nullable: if it has@defer. If it'snullit means you haven't received this fragment yet.
Design/3093-graphql-defer.md
Outdated
|
|
||
| The most simple solution is to change the deferred property type to an optional version of that type. This hides detail though because you wouldn't be able to tell whether the value is `nil` because the response data has been received yet (i.e.: deferred) or whether the data was returned and it was explicitly `null`. It also gets more complicated when a type is already optional; would that result in a Swift double-optional type? As we learnt with the legacy implementation of GraphQL nullability, double-optionals are difficult to interpret and easy lead to errors. | ||
|
|
||
| I explored Swift's property wrappers but they suffer from the limitation of not being able to be applied to a computed property. All model properties are computed properties because they simply route access the value in the underlying dictionary data storage. It would be nice to be able to simply annotate fragments and fields with some like `@Deferred` but it doesn't look like that is possible. |
There was a problem hiding this comment.
Not pressing: Investigate macros for this purpose? I haven't played much with macros outside of an iOS 17 dummy target, so I can't say what their overall level of backwards-compatibility would be. I hope that as a language feature it should be compatible, but who's to say. 🤷🏽
There was a problem hiding this comment.
Macros might be a way to obscure the implementation in the generated code so the generated models wouldn't need to be regenerated if the underlying implementation changed but I don't think macros can provide a mechanism to expose the 'state' and values of deferred fragments?
There was a problem hiding this comment.
Yeah, I thought about this a lot. The problem is there really isn't any amount of generated code that could really make this work. We actually want to be able to have a "metadata" accessor on a property. The only way to do this is through the projectedValue of a property wrapper.
We could wrap the value in some struct that had the metadata and the value, but the 99% of the time you just wanted to value and not the "state" metadata, you'd need to call myProperty.value which is definitely not what we want here.
We just wanted to be able to use that sweet swift syntactic sugar that only exists with property wrappers, but they have another limitation that prevents us from using them right now.
Design/3093-graphql-defer.md
Outdated
| } | ||
| ``` | ||
|
|
||
| Another way which may be a bit more intuitive is to make the `server` case on `Source` have an associated value since `cache` sources will always be complete. The cache could return partial responses for deferred operations but for the initial implementation we will probably only write the cache record once all deferred fragments have been received. |
There was a problem hiding this comment.
The cache could return partial responses for deferred operations but for the initial implementation we will probably only write the cache record once all deferred fragments have been received.
Just thinking on edge cases here. Let's imagine this scenario:
- User triggers a network request.
- One of the response fields is
@deferredas it takes, let's say 5 seconds to resolve. - The user navigates away from the view prior to receiving the
@deferredfield, ending all ongoing network requests for that view.
In this case, there would be no cache write at all, despite the majority of data being retrieved (and valid!). This perhaps could lead to the unintended effect wherein a partial fetch that should have updated the values of various entity types across the cache don't. Perhaps fine for an MVP implementation, but something that should be communicated out & documented.
There was a problem hiding this comment.
Yes I can see that use case. I think this is something we'll iterate towards still though instead of making it operate that way from the first release.
There was a problem hiding this comment.
Great call out @Iron-Ham. I think we're all aligned this is something we want to implement, just not for the MVP. And yes, we should clearly document this behavior.
|
I'm going to be creating issues/tasks for the parts of the RFC that are less contentious and need to happen to be able to effectively support the |
|
@AnthonyMDev / @Iron-Ham - do you have any feedback on the completion handler changes? Working on the defer protocol parser today I realized there is the |
Thinking about this more today I don't believe there is any value in passing back the label to the user. Users work with models and models are typed which is a much stronger 'reference' than a label string. If anything it should maybe be a property on deferred fragment instances. |
|
@Iron-Ham, I've updated the Generated Models section with an option (4) that would make fragments optional. I'd love to get your thoughts on it when you get a moment - thanks. |
| } | ||
| ``` | ||
|
|
||
| 4. Optional fragments (disabling field merging) - optional types are only needed when fragment fields are merged into entity selection sets. If field merging were disabled automatically for deferred fragments then the solution is simplified and we only need to alter the deferred fragments to be optional. Consuming the result data is intuitive too where a `nil` fragment value would indicate that the fragment data has not yet been received (i.e.: deferred) and when the complete response is received the fragment value is populated and the result sent to the client. This seems a more elegant and ergonimic way to indicate the status of deferred data but complicates the understanding of field merging. |
There was a problem hiding this comment.
I think this is probably the best approach we have. I do believe there is one specific situation where we already allow optional fragments. I think it had to do with having a conditionally included fragment with a @include/skip directive. We should double check that and make sure that we maintain consistency and clarity here.
There was a problem hiding this comment.
+1 -- I think this is a brilliant approach.
| // proceed ahead on the request chain | ||
| } | ||
|
|
||
| let errorHandler: (() -> Void) = { |
There was a problem hiding this comment.
We'll just need to make sure that we have adequate testing around this to ensure these closures don't cause retain cycles. Easy to capture self here and cause memory leaks.
| case complete | ||
| } | ||
|
|
||
| public let response: Response |
There was a problem hiding this comment.
Oh, this is what I was referring to above! :)
I don't like calling this Response though. It's not actually the response data, it's just indicating if the response is partial/complete. Think we just need some consideration on naming here.
There was a problem hiding this comment.
I do like the idea of providing this data to the user through an enum. As for naming, yea just Response maybe isn't the best, thinking about what we are trying to represent which is the status of the result, maybe something like ResultState? ResponseResult doesn't really feel right, and ResultData would make me think it is data and not a state representation.
There was a problem hiding this comment.
The naming we can settle on in Slack or the PR. It seems like we're aligned on the state being available at the root of GraphQLResult and not only on the server source case.
| } | ||
| ``` | ||
|
|
||
| 2. Another way which may be a bit more intuitive is to make the `server` case on `Source` have an associated value since `cache` sources will always be complete. The cache could return partial responses for deferred operations but for the initial implementation we will probably only write the cache record once all deferred fragments have been received. This solution becomes invalid though once the cache can return partial responses, with that in mind maybe option 1 is better. |
There was a problem hiding this comment.
Agreed, ultimately would be great to be able to get partial responses from cache but for this implementation I think option 1 is fine.
I'm not sure about this. There may be some valuable functionality that this could unlock, but I don't think it needs to be part of the MVP either way. We can see if we get any user feedback requesting us to expose this later. |
I initially thought there might be value too until I went back and read the RFCs description/purpose of the
|
|
I've updated the RFC with decisions on the preferred option where there were multiple and cleaned up some of the text based on review comments. In summary:
|
Read the formatted RFC here.
Codegen
@defersupport to the IR #3139@deferif not opted into #3186@deferlabel validation #3187hasDeferredFragmentsproperty and associated logic #3277@deferdirective in schema matches GraphQL spec definition #3287Execution
PossiblyDeferredtype #3144Networking
deferSpectoAcceptheader for deferred operations #3143deferSpecresponse parser #3146GraphQLResult#3148Caching
General
@deferdocumentation #3177