Merged
Conversation
…funnel to callback queue
…on of custom subclasses of `HTTPRequest`.
…lace that acutally uses it and it can now be subclassed.
Update how requests are created so custom subclasses of HTTP requests can be used
The problem is: the `additionalErrorInterceptor(for:)` function is never called, if the `InterceptorProvider` class in use is a subclass of any `InterceptorProvider` that *does not* declare the `additionalErrorInterceptor` function.
For example, consider this `CustomInterceptor` type:
```swift
class CustomInterceptor: LegacyInterceptorProvider {
// Note that we don't use `override` here.
func additionalErrorInterceptor<Operation>(for operation: Operation) -> ApolloErrorInterceptor? where Operation : GraphQLOperation {
ErrorInterceptor()
}
}
```
Its superclass, `LegacyInterceptorProvider`, does not provide an implementation of `additionalErrorInterceptor(for:)`, relying instead on the default implementation that returns `nil`.
Since its superclass does not implement this function, the implementation in `CustomInterceptor` is not overriding it, but declaring it anew. For some 🤯 reason, this means that the implementation that provides the `additionalErrorInterceptor` is never called. The default implementation in the extension of the `InterceptorProvider` protocol is called instead, returning `nil`.
I suggest 2 small changes to solve this issue, and remove this trap.
- Remove the default implementation of `additionalErrorInterceptor(for:)` from `extension InterceptorProvider`. This will force base classes to implement it; the library expects the user to subclass one of the provided InterceptorProviders anyway, so I think it's acceptable.
- Implement `additionalErrorInterceptor(for:)` in both `LegacyInterceptorProvider` and `CodableInterceptorProvider`, so that subclasses will have something to override, but keeping the nice `nil` default.
These changes make for a pretty smooth solution, with only one small breaking change: `InterceptorProvider`s that did implement `additionalErrorInterceptor(for:)` before this change will have to add the `override` keyword, if they inherited from one of the provided `InterceptorProvider`s.
This was referenced Sep 25, 2020
|
hi @designatednerd, I'm wondering if you have an estimate release date for a new version including this PR? |
added 2 commits
September 26, 2020 14:46
…n providers. Added missing `override` keyword.
Contributor
Author
|
@heyzool - Hoping for monday evening if nobody finds any showstoppers |
…-additional-error-handler-bug [Networking Stack] `additionalErrorInterceptor(for:)` doesn't work when subclassing `LegacyInterceptorProvider`
Adding the operation type to the request headers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Future changes to the networking stack will get merged into this until it comes out of beta (hopefully soon). Opening a PR to get Netlify to auto-generate updated docs that can be referenced here.
Please note:
Networking Beta:- this will help me narrow down where things are going wrong more quickly.CHANGELOGfrom this branch for updates.new-network-betabranch.