jellyfin / jellyfin-sdk-swift

Swift SDK for Jellyfin
https://jellyfin.org
48 stars 17 forks source link

Switch to CreateAPI #5

Closed PangMo5 closed 2 years ago

PangMo5 commented 2 years ago

The openapi-generator is of course a good tool, but it is difficult to use https://api.jellyfin.org/openapi/jellyfin-openapi-stable.json without modification due to problems of recursive objects such as BaseItemDTO. We're going to switch to CreateAPI, because the related issues are already solved, and I think it's more modern and more suitable for swift.

ddevler commented 2 years ago

After you switch can you start creating tags to match server version at export. So we don't have to import into SPM using master branch.

LePips commented 2 years ago

CreateAPI doesn't generate a lot compared to openapi-generator like helpers and a far more robust actual SDK, at least by my experience so far with it. I'm not really up for us changing all of this to CreateAPI at the moment since it doesn't obviously offer us any benefit.

Since you have been the one to generate this SDK, what are the problems that come from the spec regarding recursive objects? I see that you have to make some adjustments to the spec file.

I'll take some time to learn openapi-generator really quick to understand and probably make some changes if I find any.

LePips commented 2 years ago

I've been playing with openapi and our spec. What are the changes made to the template files for? As far as I can see in the diffs against the current SDK, the templates just ultimately rename the encoding container.

Ah okay I get the recursion problem.

LePips commented 2 years ago

Alright so with a CreateAPI generated SDK, just the syntax and stuff, and I do like the individualized APIClient object which is more my style versus a static client from openapi. Having to manually apply a path, even though they're through type safe paths, just honestly takes a lot longer and isn't worth it to me. There was still a manual fix that had to happen from the generated client and documentation comments had to be turned off because they weren't being commented out, just to note. We would also have to create a custom function for .get and other functions to manually inject the required authentication headers instead of repeatedly having to inject them and probably forgetting a lot.

I've realized the changes required in the spec for the recursive issues. This seems to be an issue also brought up in openapi itself so we can possibly look forward to it being fixed in the future. Openapi is also a lot more supported than CreateAPI, even for the purpose of Swift. We should also create our own generator in time for the sdk as most people do instead of using the default generation. This would most likely allow an API object that I like which can be safely injected into an app's environment. I am excited for async/await however I'll still have to play around with it. I have been creating Combine extensions for our use cases in my work and they have worked nicely. I could see myself writing a lot more code with async/await but again, I'll have to play around with it.

I'm fine with manually changing the spec for us right now and I prefer openapi against CreateAPI due to support, overall generation, and usage. CreateAPI does have a lot of customization options compared to openapi, so that is nice. It's also just too new for me against openapi right now.

Overall, I would have to play around with these SDK generations a bit more as right now I'm leaning more towards not changing it from openapi. No matter what happens, we will have to put in manual work to make a nice SDK and that's okay.

LePips commented 2 years ago

Sorry, for some reason this has taken my attention for the past bit and I should share what I am generating to come to my conclusions.

Both SDKs were also linted using SwiftFormat with the config file from Swiftfin.

OpenAPI AsyncAwait

My OpenAPI AsyncAwait generated library is at this branch using this command:

openapi-generator generate -i jellyfin-openapi-stable-edited.json -c openapi-config.yaml --type-mappings UUID=String

Code to make a call would look sort of like what's below, however I don't really like having to wrap everything into a Task but I will have to learn more AsyncAwait to see if this is correct. The try await might also get annoying with every call throwing an error.

JellyfinAPIAPI.basePath = "< server >"
JellyfinAPIPI.customHeaders = createHeaders(with: "< api key >")

Task {
    do {
        let systemInfo = try await SystemAPI.getSystemInfo()
        print(systemInfo)
    } catch let error {
        print(error)
    }
}

CreateAPI

My CreateAPI generated library is at this branch using this command:

create-api generate --output . --config createapi-config.yaml --package JellyfinAPI -s jellyfin-openapi-stable.json --entityname-template "%0Model"

Code to make a call would look sort of like what's below with the same issues as OpenAPI AsyncAwait regarding async/await overall. This does have the explicit APIClient object which I do really like instead of the single client in OpenAPI. As is obvious there is a bit more going into creating a request however it can probably be cleaned up with some extensions. Additionally, all comments had to be disabled from the generation per the issue you created with CreateAPI. I currently don't like how the API was generated due to the --entityname-template "%0Model" flag but I have created a feature request on CreateAPI to remedy this.

let client = APIClient(baseURL: URL(string: "< server >"))

Task.init {
    do {
        var request = JellyfinAPI.Paths.getSystemInfo
        request.headers = createHeaders(with: "< api key >")

        let systemInfo = try await client.send(request)
        print(systemInfo)
    } catch let error {
        print(error)
    }
}

Overall

Now that I've generated these SDKs and have been configuring them, I am actually coming around to CreateAPI. However, I don't think that it's ready yet. While I said it's "newer and less supported than OpenAPI" I guess I judged too fast before using these clients more. I'm ambitious in contributing to the project as well to overcome issues that we have and develop new features. Especially since OpenAPI is such a monolith and I personally never want to work with Java. Additionally, we would be using more frameworks from the author as he also made Nuke. I think that's pretty nifty.

I still need to learn more about async/await as I haven't used it at all and would need to learn more practices about it so that I am not as annoyed with the syntax or I get used to it.

Edit: Alright, been doing async/await stuff, gave CreateAPI a look over, and have already opened a PR for Identifiable onb entities. The only thing I want to wait for is Hashable for entities, my feature request regarding filenames, and the comment issue fixed and then we can generate!

PangMo5 commented 2 years ago

Thank you for your very detailed analysis! It's definitely a very early tool, but I think it's directly contributable and extendable, as you said. and modern and active. Also, The issue of comments you mentioned has already been raised and PR is open.

The async await is thought to be the future direction of swift, and we will soon get used to it. Utilizing async func can minimize Task usage, and we can also use it by creating a component such as AsyncButton.

kean commented 2 years ago

CreateAPI is currently managed by the team Cookpad and they've been doing fantastic job improving it so far. So I'm glad you enjoy and contribute to it.

re: Task

I suggest creating a completion-based wrapper to make it easier to use for people who haven't migrated to async/await yet, which is still new.

What you could also do it create a custom API client instead of using Get. It's what Antoine is doing in https://github.com/AvdLee/appstoreconnect-swift-sdk. That was before Get hit 1.0 though.

LePips commented 2 years ago

Thanks for the reference, we will be making some custom wrappers for Jellyfin headers and such.