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

Allow passing URLRequest as PathConfiguration source #127

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

zoejessica
Copy link
Contributor

This non-breaking change introduces another case to the PathConfiguration.Source enum, a .serverRequest(URLRequest) so that clients can customize how the configuration file is loaded from the network.

This was prompted by our use in HEY – we'd like to be able to set a different cache policy on the request so that locally cached files are ignored and we always retrieve the latest possible file from the server. But I can see this being a reasonable use case for other clients too, while default behaviour is still provided by the .server(URL) source type.

@joemasilotti
Copy link
Member

Is it worth exploring a configuration setting on Path Configuration to replicate this caching behavior?

@zoejessica
Copy link
Contributor Author

That's a good question!

For the use case I describe, we need control of the NSURLRequest.CachePolicy which is available to be customized either by the URLRequest, or by the URLSession itself. So we could definitely introduce some options to the PathConfiguration to customize this behaviour.

public struct PathConfigurationLoaderOptions {
    /// If present, the ``PathConfigurationLoader`` will initialize a new `URLSession` with
    /// this configuration to make its network request
    let urlSessionConfiguration: URLSessionConfiguration?
}

(Naming a configuration struct for a Configuration class is hard!)

PathConfiguration's init method becomes

public init(sources: [Source] = [], options: PathConfigurationLoaderOptions? = nil)

and the options flow down to the PathConfigurationLoader which uses the config to instantiate a custom URLSession if needed:

        let session = options?.urlSessionConfiguration.map { URLSession(configuration: $0) } ?? URLSession.shared
        session.dataTask(with: url) ...

Was this the kind of solution you had in mind? It does set up a nice structure for further customization down the road, if there's appetite for that, but it's also a bit more complex of course.

@olivaresf
Copy link
Member

It does set up a nice structure for further customization down the road, if there's appetite for that, but it's also a bit more complex of course.

This structure would be a good idea to explore, but for our use case right now a minor update feels like the way to go.

@olivaresf olivaresf self-requested a review June 29, 2023 03:12
@zoejessica
Copy link
Contributor Author

@joemasilotti I'll leave this open until tomorrow in case you have feedback on the configuration option above.

@joemasilotti
Copy link
Member

Nope! If we keep it simple like this it looks good to me. 👍

This set up allows for future configuration of the loader and keeps the PathConfiguration.Source enum focused on the source of the config file rather than the specific method of obtaining the file.
@zoejessica zoejessica merged commit 39059e1 into main Jul 6, 2023
1 check passed
@zoejessica zoejessica deleted the path-config-request branch July 6, 2023 12:51
@joemasilotti
Copy link
Member

I thought we were going with the request and not full on configuration? What sparked the last minute change?

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

Successfully merging this pull request may close these issues.

None yet

3 participants