Apollo-ios: some thoughts on Interceptors

[RolandasRazma]

Feedback

I just update from 0.33.0 to 0.36.0 and want to share some thoughts on Interceptors.

• confusing what does proceedAsync, handleErrorAsync, retry and kickoff do. If I have error in response and want to retry do I call retry? or kickoff?
• RequestChainNetworkTransport but you pass response Interceptors to it (as request and response interceptors are the same list)
• confusing that same call used for request and response, only way to check for response == nil in interceptAsync
• TokenAddingInterceptor from docs adds Authorization header to request not depending if its request or response. Is that how it should be?
• RequestChainNetworkTransport have additionalHeaders - shouldn't this be interceptor?
• there is AutomaticPersistedQueryInterceptor and also autoPersistQueries on RequestChainNetworkTransport what should I use?
• There is LegacyInterceptorProvider but something not "legacy" (NetworkInterceptorProvider) only mentioned in documentation. How do I create custom provider where I add only my interceptors and leave "default" ones untouched? NetworkInterceptorProvider from docs have 10 interceptors, LegacyInterceptorProvider have 7. Should I inherit from LegacyInterceptorProvider or should I copy one from docs and constantly track if new Interceptor will just appear in the list?

Versions

Please fill in the versions you're currently using:

• apollo-ios SDK version: 0.36.0
• Swift version: 5.3

[RolandasRazma]

comment about TokenAddingInterceptor is invalid as it is inserted in first place (so it can't be response)

[designatednerd]

confusing what does proceedAsync, handleErrorAsync, retry and kickoff do. If I have error in response and want to retry do I call retry? or kickoff?

• proceedAsync is called anytime you want to have the chain proceed to the next interceptor
• handleErrorAsync is called anytime you want to have the chain return an error to the original caller
• kickoff should only be called by you when you want to start the chain from scratch.
• retry should be called if you want to retry a request - it will reset the interceptor index under the hood and call kickoff to restart the chain. You should use this method rather than trying to reset the index yourself. Basically, don't call kickoff on the chain from within an interceptor, call retry.

RequestChainNetworkTransport but you pass response Interceptors to it (as request and response interceptors are the same list) confusing that same call used for request and response, only way to check for response == nil in interceptAsync

I'm not totally clear on what the question here is - is it "Why are we using the same interceptors for preparing a request as handling a response?" If yes, please confirm, if not, please clarify. Thanks.

TokenAddingInterceptor from docs adds Authorization header to request not depending if its request or response. Is that how it should be?

Yes - the idea of that is that you put it first so that it is the first change applied before a request goes out.

RequestChainNetworkTransport have additionalHeaders - shouldn't this be interceptor?

These are intended to be additional headers that must be added to every single request, and which do not change (for example, an API key or a language setting). The interceptors are intended for things which may change (for example, a user's authentication token).

there is AutomaticPersistedQueryInterceptor and also autoPersistQueries on RequestChainNetworkTransport what should I use?

The short answer is both - autoPersistQueries tells you whether auto-persisted queries should be used at all, and the interceptor does the work of checking for APQ-related errors and auto-retrying when needed. Essentially, autoPersistQueries tells you if you should send APQ hashes outgoing, and the interceptor handles incoming responses to tell if you need to retry due to an APQ failure.

There is LegacyInterceptorProvider but something not "legacy" (NetworkInterceptorProvider) only mentioned in documentation. How do I create custom provider where I add only my interceptors and leave "default" ones untouched? NetworkInterceptorProvider from docs have 10 interceptors, LegacyInterceptorProvider have 7. Should I inherit from LegacyInterceptorProvider or should I copy one from docs and constantly track if new Interceptor will just appear in the list?

You _can_ inherit from LegacyInterceptorProvider, and override the method providing the array of interceptors. This is easier if you're putting interceptors at the beginning or end of the array rather than interspersing them throughout. If they're interspersed, it's likely better create your own implementation of InterceptorProvider and using the array provided by LegacyInterceptorProvider as a starting point.

I will call out in release notes if any changes are made to the LegacyInterceptorProvider, particularly new interceptors - the intent is that the core interceptors should not change.

[Nealsoni00]

I was going through a similar thinking process as @RolandasRazma yesterday doing the same 0.33->0.36 migration. Would be super helpful if you added these explanations to the documentation for other people.

[designatednerd]

Have y'all had a chance to read through the updated client creation documentation, particularly the section on how the request chain works? I would be particularly interested in where you feel like that's not clear enough. Thanks!

[RolandasRazma]

I did, I think for me most confusion would have been averted if:

• there would be 2 lists: pre server call and post server call having different call signature (no reply in "pre" ones)
• there would be no "Legacy" in class names as it made not clear what to use
• don't feel feel comfortable of just copying list of "default" interceptors as that list might change without the realising

[designatednerd]

there would be 2 lists: pre server call and post server call having different call signature (no reply in "pre" ones)

One of the reasons I went with a single chain is we've had repeated feature requests to have the data associated with what was being requested passed through to the completion handler. I will be taking a look at some options based on feedback here, I think maybe there could be some other options that might make NetworkTransport unnecessary and allow the user to send data through any transport (ie, send it with a websocket if you really feel like it).

there would be no "Legacy" in class names as it made not clear what to use

OK. I believe I tried to explain "Legacy" vs "Codable" as "What we've been using" vs "What we will be using in the future" in those docs, but it sounds like that didn't help. I'll try to clarify that.

don't feel feel comfortable of just copying list of "default" interceptors as that list might change without the realising

I mean, that's going to be a problem no matter what if you need to intersperse different interceptors - if you're subclassing hte default list and relying on the default order in terms of where to insert your interceptors, that's going to cause problems too. Again, I can certainly commit to making sure I make it very clear in release notes when any of this changes.

[designatednerd]

@RolandasRazma @Nealsoni00 I've just opened a PR with updated docs. Would love your feedback on #1484.

[RolandasRazma]

One of the reasons I went with a single chain is we've had repeated feature requests to have the data associated

Everything could still work as it does now under the hood. 2 list would allow to not have "response" in pre list

I mean, that's going to be a problem no matter what

that's not necessary true if there would be 2 lists as in most cases you need "pre action" and "post action"

I already migrated and now it's more clear what's going on so I'm not advocating for a change. Most likely all new users will do better as they have no expectations

[designatednerd]

2 list would allow to not have "response" in pre list

That's fair.

I'm not advocating for a change.

I think if we're gonna make one it's better to do it sooner rather than later, honestly. I also need to think a few things over, to be honest.

[designatednerd]

OK, I've merged #1464. I will continue to think about what could work to make it easier to deal with when an interceptor will be called - pre or post response - and see what I can come up with. However, I wanted to check - do you feel there's anything else here that still needs to be directly addressed before we can close this issue out?

[RolandasRazma]

I will close it

[designatednerd]

@RolandasRazma thank you!