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

Kotlin multiplatform and flatbuffer support #12

Open
DadiBit opened this issue Dec 25, 2023 · 11 comments
Open

Kotlin multiplatform and flatbuffer support #12

DadiBit opened this issue Dec 25, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@DadiBit
Copy link
Member

DadiBit commented Dec 25, 2023

Is your feature request related to a problem? Please describe.
The project is currently on halt, since kmp support for flatbuffers is not yet fully working.

Describe the solution you'd like
Either:

  1. stop waiting for kmp support --> work on flatbuffer support, and later on rewrite everything as KMP
    a) less work now, more later
    b) a bit stupid IMHO, since java library already exists, and it can be imported in kotlin, making this one "redundant"
  2. manually implement kmp flatbuffer classes --> rewrite from scratch everything already
    a) more work now, less later
    b) I honestly don't have much time during the week, but I could cut out some time on weekends to work on it
    c) If I work too slowly and the flatbuffer compiler gets kmp fully implemented then all the work is going to be wasted
  3. work on the actual flatbuffer compiler and then come back to this project
    a) it would be a ton of work for me, since I have absolutely no idea of how the flatc compiler works
    b) would benefit other devs as well

Additional context

  • Google flatbuffer repo has a few PRs waiting to be merged
  • Main issue is silent since September, but was already moving slow before
  • Java SDK already implemented in the repo

PS: I know this is more like a personal note, but feel free to comment on wtf I should do (or what you could do to help 😄)

@DadiBit DadiBit added the enhancement New feature or request label Dec 25, 2023
@DadiBit DadiBit self-assigned this Dec 25, 2023
@DadiBit
Copy link
Member Author

DadiBit commented Dec 26, 2023

Please also see #40 on the website repo for original issue and brainstorming ideas + working code snippet in jvm (?) Kotlin @LeoColman

@LeoColman
Copy link
Contributor

The original issue has a very rich discussion! I think I'll be learning wtf Flatbuffer means 😂

My opinion on your suggested solutions:

stop waiting for kmp support --> work on flatbuffer support, and later on rewrite everything as KMP
a) less work now, more later
b) a bit stupid IMHO, since java library already exists, and it can be imported in kotlin, making this one "redundant"

a) less work now, more later

I agree. We would need to rewrite stuff to the KMP version. I think the tests and the logic would remain the same, so I'm not sure if this 'more work later' is big or small

b) a bit stupid IMHO, since java library already exists, and it can be imported in kotlin, making this one "redundant"

I'm not sure if it's completely stupid. I don't think it would be redundant, as Kotlin users typically rather have a Kotlin binding than using the Java binding. Even if they both use the Java SDK.

What we could do is import it to the Kotlin-SDK until something comes out for MPP


manually implement kmp flatbuffer classes --> rewrite from scratch everything already
a) more work now, less later
b) I honestly don't have much time during the week, but I could cut out some time on weekends to work on it
c) If I work too slowly and the flatbuffer compiler gets kmp fully implemented then all the work is going to be wasted

This looks what I would call in PT-BR some cornojob. I don't think we should work on reimplementing Flatbuffer for KMP. We probably don't have enough knowledge neither in Flatbuffer nor in KMP to do this with good quality, and as you said: It will eventually be thrown away.

If we want to do something like this, I'd rather create a library that does it than adding it to this project.


work on the actual flatbuffer compiler and then come back to this project
a) it would be a ton of work for me, since I have absolutely no idea of how the flatc compiler works
b) would benefit other devs as well

Thanks Apollo 😂
I'll ask around Kotlin's slack to see if people have solved this problem or the ETA for when it will be ready.

@LeoColman
Copy link
Contributor

IMO, a good first approach would be

Use Java bindings in this library, make it Kotlin-JVM only, but prepare the API and system design using Kotlin-Common

When other platforms (ie Kotlin for Flatbuffers) are ready, we can just include them as supported platform.

This would make this project
1. KMP-Ready
2. Kotlin-like (Real Kotlin DSL, real Kotlin project)

@LeoColman
Copy link
Contributor

LeoColman commented Dec 26, 2023

One thing to take into consideration: Part of Open-Meteo API is still not available in the SDK as FlatBuffers. We can ignore flatbuffers and use http until both the SDK and Kotlin are 100% flatbuffer-compatible.

We can generate the API from the OpenAPI definition instead https://github.com/melix/open-meteo/blob/main/openapi.yml

@DadiBit
Copy link
Member Author

DadiBit commented Dec 29, 2023

IMO, a good first approach would be

Use Java bindings in this library, make it Kotlin-JVM only, but prepare the API and system design using Kotlin-Common

When other platforms (ie Kotlin for Flatbuffers) are ready, we can just include them as supported platform.

This would make this project 1. KMP-Ready 2. Kotlin-like (Real Kotlin DSL, real Kotlin project)

Lol I didn't think about this way of solving the issue, thanks! I got a first implementation of the common sub-package up and running in kmp 🎉 ; the only features missing are: Date/Time stuff, URL encoding and http client (maybe ditching the one I implemented in java with something more sustainable like KTOR could solve these last two points)

I need to do more testing. Actual implemetation of all APIs should happen after the http client thing is solved: possibly flatbuffer implementation could be available in KMP by then, and even if it's still a kotlin/jvm only thing we can implement it just for that.

One thing to take into consideration: Part of Open-Meteo API is still not available in the SDK as FlatBuffers. We can ignore flatbuffers and use http until both the SDK and Kotlin are 100% flatbuffer-compatible.

Yep, currently the sdk was using protobuf, which initially was planned to be used in the other apis as well, but since flatbuffer was picked it's probably going to be used across all APIs when it's going to be finalized.
Sidenote/fun fact: csv and xlsx formats are also available to download.

We can generate the API from the OpenAPI definition instead https://github.com/melix/open-meteo/blob/main/openapi.yml

Unluckily this doesn't always reflect the latest API, as opposed to flatbuffers, which use a single source of truth (the .fbs schema files) across the server and the sdk 😄

Again, thank you for the brillant idea @LeoColman 🙇

@DadiBit
Copy link
Member Author

DadiBit commented Dec 30, 2023

After some more testing I decided to create a dedicated kmp branch and push my "tests" to it.
Currently I tested the Ktor OkHttp client, with Json parsing. Protobuf should work fine too. Flatbuffers currently throw an error as "not yet implemented" (probably should use a @UseExeperimentalFlatbuffers annotation or something).
The logic is still the same: using interfaces, which allow "mixins", as opposed to classes, which let you to have only one parent class at a time, which lead to SuperVeryLongClassNames. When I get to Flatbuffer implementation I can start implementing all the response stuff.

@DadiBit
Copy link
Member Author

DadiBit commented Jan 4, 2024

Update 2024-01-04

Date/time problem: kotlinx-datetime should work just fine for us. I still need to test TimeZones (especially "auto" implementation) 👍

Flatbuffer: added a git submodule to the google flatbuffer library + soft link to the main targets source code (google's repo tests require some generation in the gradle script, but I couldn't understand where the hell to place the generated output to make it so commonTest would recognize the folder; didn't link them, for now).
By adding the google/flatbuffers repo as submodule we also get a working copy of flatc (by compiling it, tho) that can generate kmp source code. If we were to add the main sdk repo (removing it when this one is merged into it) we could generate the response parse "automagically" 🪄

Tested targets: my tests pass on jvm, linux, node.js & chromium.

Apple targets: I do not own any macOS device, but I do have a tethered-jailbroken iPhone; to my understanding, I would need XCode, maybe in a macOS VM, so for the time being I removed all apple stuff. Just in case, for future reference, @LeoColman do you have any macOS device to run the library tests on? It could be a big time saver, honestly 😄

@LeoColman
Copy link
Contributor

I don't have any mac :(

Only Github can run with mac for me 😂
https://github.com/kotest/kotest/blob/master/.github/workflows/release_macos.yml

@DadiBit
Copy link
Member Author

DadiBit commented Jan 21, 2024

Update 2024-01-21

I got some interesting stuff working on https://github.com/DadiBit/Open-Meteo-KMP

I decided to start from scratch with a "temporary" repo to experiment with gh actions and more.

I still need to figure out how to properly implement multiple location queries, but the main deal of content fetching & decoding is done. I do not plan to touch it any more1 🎉

I think I could also start trying to work on a forked repo from the official open-meteo/sdk one to get everything in the kotlin subdirectory.

Footnotes

  1. Yeah I know, I know, I need to get tests done, please have mercy

@LeoColman
Copy link
Contributor

experiment with gh actions

I suggest experimenting with https://github.com/typesafegithub/github-workflows-kt . Kotlin all way down :)

That repo is pretty interesting! 🎉

@DadiBit
Copy link
Member Author

DadiBit commented Jan 28, 2024

Thank you again for the support! It looks interesting, I'm going to have a look later on this week.

Quick update: I got the multiple location thing working, but it's not super dev-friendly. I would like to be able to pass both lists and single entries, and perhaps even add a thing that "unzips" each entry.

Meanwhile, I got my hands dirty with the flatbuffer kmp compiler, and I'm probably going to make a PR from DadiBit/flatbuffers, since, right now, the compiler doesn't use enum class for enums... It would make the code cleaner and overall more intuitive. If you have any C++ knowledge I would love some feedback on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants