Closed LePips closed 2 years ago
Hey @LePips.
I have a couple of potential options on how to design it.
1. Using a factory method:
extension APIClient {
// Returns an API client preconfigured to work with JellyfinAPI
static func jellyfin() -> APIClient {
}
}
2. Creating a custom client class that use APIClient
behind the scenes.
As to the protocol, I'm not sure I fully understand how it helps in your case. I don't think it belongs to Get
because if it's added, it's not going to be used anywhere. And if someone is using CreateAPI
, they just to provide an API compatible with the Request
type. It doesn't have to be compatible with APIClient
.
Hi 👋 ,
My basic wrapper is option 2 with an APIClient
object behind the scenes, as I assume all wrappers are. This protocol just aids in matching the exact interface of the underlying APIClient
object and ensures compatibility over the evolution of the project. I assume that developers want the exact same interface for sending requests as I would, since for some calls I might set the delegate or edit the URLRequest
, and so on.
These wrappers with an underlying APIClient
object allow a more tailored experience for SDK creation and overall API interfacing.
This is not necessary (after all, there are only 5 methods) but I thought to pitch it as I will be implementing this pattern anyways. The "evolution of the project" might not also foresee much networking interfacing changes.
This can of course be ignored.
actor JellyfinClient: APIInterface {
private var _apiClient: APIClient
init() {
// manually control the APIClientDelegate on the underlying APIClient
// per developer reasons
_apiClient = APIClient( ... )
}
@discardableResult
func send<T>(
_ request: Request<T>,
delegate: URLSessionDataDelegate? = nil,
configure: ((inout URLRequest) throws -> Void)? = nil
) async throws -> Response<T> where T : Decodable {
try _apiClient.send(request, delegate: delegate, configure: configure)
}
// Continue all other method wrappers that send to underlying _apiClient below.
// APIInterface conformance ensures that I will match all APIClient methods
// now and in the future.
}
// Usage
let jellyfinClient = JellyfinClient()
// Assuming CreateAPI generation
let itemsRequest = Paths.Items.getItems( ... )
jellyfinClient.send(itemsRequest)
What about the "factory method" approach? It will ensure compatibility and there will be less maintenance for you in the long run.
These wrappers with an underlying APIClient object allow a more tailored experience for SDK creation and overall API interfacing.
Is there something missing in the existing configuration/delegate API?
factory method
A wrapper allows developers to specify behavior with their API via additional properties or methods.
Using my case as example: additional headers that include a unique device name, device identifier, and app version are required alongside the access token:
Example excerpt of header dictionary creation:
var header = "MediaBrowser "
header.append("Client=\"Jellyfin \(platform)\", ")
header.append("Device=\"\(deviceName)\", ")
header.append("DeviceId=\"\(platform)_\(UIDevice.vendorUUIDString)_\(String(Date().timeIntervalSince1970))\", ")
header.append("Version=\"\(appVersion ?? "0.0.1")\", ")
header.append("Token=\"\(accessToken)\"")
Per my API, these headers are required for proper usage. To create this, I would have the property JellyfinClient.accessToken
which would be set by my flow when signing in. By manually controlling the APIClient.delegate
in the underlying JellyfinClient._apiClient
creation, I can inject these headers to calls when they are necessary.
I haven't thought out the full architecture yet, but I could also add JellyfinClient.signIn/signOut
methods to manually handle this network flow, using the underlying JellyfinClient._apiClient
for the required API calls, and also specifying the other APIInterface
calls for all other requests throughout the application.
Is there something missing in the existing configuration/delegate API?
No, as I am simply creating another layer above APIClient
to provide additional functionality to specify behavior that I expect with my API. This behaviors are entirely expected and I would either have to tell other developers how to properly use it or I can create this behavior in the client itself. The latter is more convenient in my opinion from a development and maintenance perspective.
The layers are as such:
1 - JellyfinClient
/any client wrapper - communicates to developers how this API should be used. Uses underlying APIClient
for API calls.
2 - APIClient
- interface for general API calls, uses URLSession
for implementation.
3 - URLSession
- handles actual networking.
This comment is probably longer than it should be and moreso justifies wrapper implementations for an underlying APIClient
. So with this explanation, this protocol just ensures that if a developer is making a wrapper that they can be guaranteed to match the underlying APIClient
networking interface if they desire.
However again: this is a very small protocol and not necessary.
Later addition: the most "ideal" way to achieve what I desire would have been the ability to have JellyfinClient
subclass APIClient
. However we can't create sub-actors, so this protocol can also be thought being able to replicate that behavior for an underlying APIClient
.
I think the approach with a wrapper makes total sense, especially if you want to add more APIs to it, such as signIn/signOut
. I think people generally prefer hiding the external dependencies from their API to have control over it, so I'm not entirely sure why you want the opposite. APIClient
will continue to evolve and it probably has some APIs that your clients don't need as well. I would suggest hiding them so that you have control over your API evolution.
As for the protocol, as you said yourself, I don't see how it's necessary. I'm going to close this MR if you don't mind.
I don't mind at all, thank you for your time.
Original issue with proposal:
APIInterface
name is open for a great improvement. I'm not too familiar with Swift on linux so I don't know if the OS check is appropriate, but included anyway.