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

APIInterface Protocol #49

Closed
wants to merge 1 commit into from
Closed

Conversation

LePips
Copy link
Contributor

@LePips LePips commented Aug 8, 2022

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.

@kean
Copy link
Owner

kean commented Aug 8, 2022

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.

@LePips
Copy link
Contributor Author

LePips commented Aug 8, 2022

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)

@kean
Copy link
Owner

kean commented Aug 8, 2022

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?

@LePips
Copy link
Contributor Author

LePips commented Aug 8, 2022

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.

@kean
Copy link
Owner

kean commented Aug 10, 2022

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.

@kean kean closed this Aug 10, 2022
@LePips
Copy link
Contributor Author

LePips commented Aug 10, 2022

I don't mind at all, thank you for your time.

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.

None yet

2 participants