Adds RequestContext to the RequestChain/Interceptor pipeline #3198
Conversation
👷 Deploy request for apollo-ios-docs pending review.Visit the deploys page to approve it
|
There was a problem hiding this comment.
Thanks so much for the contribution! I don't have a problem with this approach at all, though I would like to get input from @calvincestari on this before merging in. He works on the networking layer a lot more than I do. He is out until the 27th, so I'll get his input when he's back.
| /// - resultHandler: [optional] A closure that is called when query results are available or when an error occurs. | ||
| /// - Returns: An object that can be used to cancel an in progress fetch. | ||
| func fetch<Query: GraphQLQuery>(query: Query, | ||
| cachePolicy: CachePolicy, | ||
| contextIdentifier: UUID?, | ||
| context: RequestContext?, |
There was a problem hiding this comment.
The only concern I have is that this is technically a minor breaking change. Anyone who is implementing the ApolloClientProtocol (which I would think is usually just for mock objects in unit tests), will have to add the new parameter. It's not a huge change, but it will not be backwards compatible.
I'm not really sure that there is a great method for deprecation of the old signatures though. So I think this falls under acceptable minor breaking changes.
There was a problem hiding this comment.
Yeah, I couldn't think of a way to do it without creating a breaking change. Glad to hear you're likely ok with it!
|
Oh! This PR also needs a couple simple unit tests that just ensure that the |
|
Added the tests that were requested :) |
There was a problem hiding this comment.
I had thought about suggesting that we attach the requestContext to the GraphQLOperation instead of passing it through the client functions, but that would require it to be added as a field on all the generated operations, which I don't think is a better solution.
With the added tests, I'm good with merging this PR in! @calvincestari, do you have any other opinions?
| @@ -0,0 +1,82 @@ | |||
| import XCTest | |||
There was a problem hiding this comment.
These tests are awesome! Nice work.
|
What about if we make the request context part of My original thought was that two requests with different contexts should be recognized as different requests, i.e.: not equatable. I don't know yet where that comparison would be useful but I feel it's still technically correct. I'm unable to create a branch on your fork so I've put the changes into a patch file which you can apply and see the difference in implementation. I think it creates less of a breaking change too. |
Unfortunately my original goal of the difference in equality is still not possible due to |
|
I actually really like that idea @calvincestari! The This also feels like a logical place to store it and ensures it is retained until the request is complete and deallocated. @danieltiger Do you mind making that change? |
Correct. |
Simply applying the patch should give you exactly that. |
|
Sorry, I haven't had a chance to take a look yet, but I will tomorrow morning! |
|
Ok. PR is updated. This approach is great. I had briefly considered it when I started, but pivoted for reasons I no longer remember ;) Anyway, I updated all the comments everywhere and made sure the context is being passed into the upload request as well. We should be good to go now! |
|
@AnthonyMDev are we ready to merge this? :D |
|
Merged! Thanks so much @danieltiger! We're going to get a minor release out to include this today. |
|
Amazing! Thanks so much for helping this get done! |
|
I have achieved the same by subclassing query classes. Then in the interceptor I'd downcast to check whether I need what I am after. Since we are forced to use downcasting the RequestContext as well I wonder if this approach has more merits ? Also worth mentioning that I am using query subclasses to differentiate operations in the |
There is sometimes a need to do things in an interceptor that relies on information not contained in the operation that is passed in. An example of this might be wanting to do custom headers on a per-request or pre-request-type basis.
As a solution, this PR adds a new marker protocol called RequestContext, that can be used to pass anything that a consumer might want/need through the request chain, making it available in any custom interceptor.
This was intentionally left as only a marker protocol, not any kind of specific context implementation. That's to allow the maximum flexibility for users of the chain, and to not try to solve for the probably endless needs they might have.