Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: introducing interceptors #232

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gitrema
Copy link
Contributor

@gitrema gitrema commented Mar 29, 2023

Implement the interceptor patterns to allow users of the SPTDataLoader to provide their own set of interceptors.

@gitrema gitrema force-pushed the add-interceptors branch 4 times, most recently from 9b486ea to 0bad779 Compare March 30, 2023 12:02
Copy link
Collaborator

@kmcbride kmcbride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable overall.

Conceptually it seems like an authorizer is an interceptor, but I don't know that it would be possible to combine the two without breaking existing functionality… perhaps in a future release.

Comment on lines +28 to +29
- (nonnull instancetype)init NS_UNAVAILABLE;
+ (nonnull instancetype)new NS_UNAVAILABLE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: nonnull can be removed since this is within the macro's scope

Comment on lines +27 to +28
- (SPTDataLoaderInterceptorResult *)decorateRequest:(SPTDataLoaderRequest *)request;
- (SPTDataLoaderInterceptorResult *)decorateResponse:(SPTDataLoaderResponse *)response;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I've written an Objective-C API 😄 but would this be more idiomatic?

Suggested change
- (SPTDataLoaderInterceptorResult *)decorateRequest:(SPTDataLoaderRequest *)request;
- (SPTDataLoaderInterceptorResult *)decorateResponse:(SPTDataLoaderResponse *)response;
- (nullable SPTDataLoaderRequest *)decoratedRequest:(SPTDataLoaderRequest *)request error:(NSError * _Nullable *)error;
- (nullable SPTDataLoaderResponse *)decoratedResponse:(SPTDataLoaderResponse *)response error:(NSError * _Nullable *)error;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe but they are easier to be rewritten in Swift like that ;)

Comment on lines +240 to +243
SPTDataLoaderInterceptorChain * chain =
[[SPTDataLoaderInterceptorChain alloc] initWithInterceptors:self.interceptors];
@synchronized(self.requestToInterceptorChain) {
[self.requestToInterceptorChain setObject:chain forKey:request];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that self.interceptors is immutable and the chain does not store information unique to each request, is it necessary to be creating a chain for every request?

It seems like self.interceptors and self.requestToInterceptorChain could be replaced with a single self.interceptorChain property.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can as it is right now. I was thinking ahead by adding Cancellation at some point and I thought to cancel the single interceptor that is running for a given request (given multiple ones can be in a different stage of the interceptor chain). On the other hand all the interceptors can be cancelled at the same time regardless if they are in use and demand to the implementation to properly handle it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants