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

Networking Beta: Convenience constructors for HTTPRequest and JSONRequest #1404

Closed
knox opened this issue Sep 21, 2020 · 8 comments
Closed

Comments

@knox
Copy link

knox commented Sep 21, 2020

In my use case i have to append to the URLRequest.query before a request is actually sent and that was fairly straight forward with the old network stack using HTTPNetworkTransportPreflightDelegate.networkTransport(_:willSend:)

With the new network stack things have gotten a bit more complex but i was able to come up with a solution involving a custom ApolloInterceptor that turns a given JSONRequest into a custom subclass which overrides toURLRequest().

So to make implementation of custom HTTPRequests more easy i suggest you to add some convenience constructors for HTTPRequest and JSONRequest that take a single argument of their own type. This would let me get rid of this bunch of constructor arguments like this:

        let customRequest = CustomJSONRequest<Operation>(jsonRequest)

instead of this:

        let customRequest = CustomJSONRequest<Operation>(
                operation: jsonRequest.operation,
                graphQLEndpoint: jsonRequest.graphQLEndpoint,
                contextIdentifier: jsonRequest.contextIdentifier,
                clientName: jsonRequest.additionalHeaders["apollographql-client-name"] ?? "",
                clientVersion: jsonRequest.additionalHeaders["apollographql-client-version"] ?? "",
                additionalHeaders: jsonRequest.additionalHeaders,
                cachePolicy: jsonRequest.cachePolicy,
                autoPersistQueries: jsonRequest.autoPersistQueries,
                useGETForQueries: jsonRequest.useGETForQueries,
                useGETForPersistedQueryRetry: jsonRequest.useGETForPersistedQueryRetry,
                requestCreator: jsonRequest.requestCreator
                )
@designatednerd
Copy link
Contributor

The override is the correct way of doing this - I'm not quite clear why the super init, which has a bunch of default parameters, isn't the way you're doing this, though. Can you go into a bit more detail on that?

@knox
Copy link
Author

knox commented Sep 21, 2020

When looking for a way to get my CustomHTTPRequest with its overriden toURLRequest() into place i learned about the concept of ApolloInterceptor in the new network stack architecture.

Therefore i have implemented a custom interceptor to replace the original HTTPRequest instance with one of CustomHTTPRequest as a substitution. Basically what i'm trying to do here is to create a copy of the request with whatever particular values its properties have but extend its original functionality.

If there is something i have misunderstood or a better approach to achive this i'm glad to get enlighted.

@designatednerd
Copy link
Contributor

OK! I think I see what the problem is here: While the RequestChain can take an arbitrary subclass of HTTPRequest, there's no way to say "You should use this specific subclass" from the level of the RequestChainNetworkTransport, so as of right now you're having to recreate the request at the interceptor level.

It sounds like the issue is that there needs to be a way to specify the request type that's customizable at that level, so that you can actually use the custom type you've created.

Does that sound accurate? I'm gonna poke at some ideas on this.

@knox
Copy link
Author

knox commented Sep 21, 2020

Indeed, if subclassing HTTPRequest is the intended way to go, a more simple way to place that type into the processing is greatly appreciated.

@designatednerd
Copy link
Contributor

@knox Please see #1405 . You may have opened a trapdoor to a bigger change but it's a good one 😄

@knox
Copy link
Author

knox commented Sep 22, 2020

Wow i didn't mean to ask for moving mountains but opening RequestChainNetworkTransport and constructRequest(for:cachePolicy:contextIdentifier:) seems to perfectly fit my needs.

@designatednerd
Copy link
Contributor

Haha, this worked as designed: You pointed out that I hadn't provided an access point to something, so I added it. Then by adding it I realized something else that was annoying me could change. 😇

@knox
Copy link
Author

knox commented Sep 23, 2020

With 0.34.0-rc.2 i was able to implement a clean and simple solution to my use case.

@knox knox closed this as completed Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants