fix: Remove managedSelf from InterceptorRequestChain
#3070
Conversation
| @@ -4,6 +4,8 @@ import ApolloAPI | |||
|
|
|||
| /// A protocol to set up a chainable unit of networking work. | |||
| public protocol ApolloInterceptor { | |||
|
|
|||
| var id: String { get set } | |||
There was a problem hiding this comment.
Identifiable is only available on iOS 13+ and macOS 15+, we support lower versions of both. This imposes the same 'identifiable' requirement for interceptors.
There was a problem hiding this comment.
- It would be really helpful to document this method and explain what the purpose and scope of this identifier is, so that custom implementers can decide what the best identifier would be for their specific use case.
- In order to avoid the breaking change introduced by this, have you considered a default implementation of this requirement using a protocol extension?
There was a problem hiding this comment.
It would be really helpful to document this method and explain what the purpose and scope of this identifier is, so that custom implementers can decide what the best identifier would be for their specific use case.
Documentation can be added to clarify it's purpose, I'll get that done. As long as it's unique to other identifiers and a String it shouldn't matter. This was required as a workaround to the interceptor architecture while fixing the memory leak and maintaining the ability to support HTTP-based subscriptions.
In order to avoid the breaking change introduced by this, have you considered a default implementation of this requirement using a protocol extension?
Yes I did, but Swift extensions cannot hold stored properties and the identifier needs to be constant during the lifetime of the interceptor. Static constants don't work here either.
There was a problem hiding this comment.
Documentation can be added to clarify it's purpose, I'll get that done.
Thanks! Perhaps the scope is the interceptor instance? What does "other identifiers" mean concretely? Unique within the set of installed interceptors active during the processing of one request?
Swift extensions cannot hold stored properties and the identifier needs to be constant during the lifetime of the interceptor.
Ahh, makes sense. Perhaps an alternative might've been an id { ObjectIdentifier(self) }, but it would incur a protocol requirement for class-semantics by the implementer.
There was a problem hiding this comment.
What does "other identifiers" mean concretely? Unique within the set of installed interceptors active during the processing of one request?
Correct. The interceptor provider passed into RequestChainNetworkTransport during initialization is used to build the request chain for each request. They need to be unique amongst that array of interceptors.
| ) | ||
| } | ||
|
|
||
| private func proceedAsync<Operation: GraphQLOperation>( |
There was a problem hiding this comment.
The shared logic of proceedAsync so the other two public functions are purely for interceptor lookup.
|
@AnthonyMDev, @BobaFetters - this solution is generally ready for review. It needs some tests for the index sync between the two |
| ) | ||
|
|
||
| // when | ||
| requestChain.kickoff(request: request) { result in } |
There was a problem hiding this comment.
We don't care about the response in these tests, there are other tests for that. These purely ensure that the interceptors are executed in the correct order.
…ql#3070) # Conflicts: # Tests/ApolloInternalTestHelpers/InterceptorTester.swift # docs/source/config.json
Fixes #3057 (supersedes #3065)
This PR removes
managedSelffromInterceptorRequestChaingoing back to the memory management model that was used before the introduction of HTTP-based subscriptions.The gist of this solution is to deprecate the current
proceedAsyncfunction, adding a new one that requires the interceptor to identify itself. It's the interceptor identifier that allows us to get rid ofApolloInterceptorReentrantWrapperandmanagedSelf.