-
Notifications
You must be signed in to change notification settings - Fork 7
feat!: Kotlin Multiplatform support #148
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
62a5911
to
eb1c6b1
Compare
Hey thanks for this, I will look at it as soon as I have a bit of time. I have played myself in kotlin with the java server sdk and it works well. |
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Trying to wrap my brains around this... This wouldn't just work with any js provider, right? It has to be a provider that is written to work with this SDK, a provider that is also multiplatform. |
Actually, it is technically possible to create wrappers around the java and js providers thanks to Kotlin's interop capabilities. The wrapper would proxy calls to the underlying implementation, and would convert back the return values and exceptions to KMP values/exceptions. I have seen something similar in action for OpenTelemetry in the Embrace repo I referred to on Slack in the thread we talked about this topic. As I mentioned on Slack, the current roadblock for such Java wrapper is a) the collision of package names and b) the static vs dynamic context paradigm issue you mentioned on Slack. a) we can easily solve in this PR even, if it's not too many breaking changes for a single PR. Regarding b) my thoughts are that it's incorrect to categorize Java as a server language and Kotlin as a client one, given that both can be used for both purposes. I'd much rather have Providers for both languages that support both modes of operation. But this is maybe a bit off-topic for this PR, I suggest that we switch to the Slack thread to discuss the question of the wrapper. |
This is a cool PR! :) To me, it makes sense to make clear that the Kotlin and also the Swift are implemented as client (static context) SDKs. If the rewrite is really that straight forward and the KMP version works for all platforms without additional effort, I think this would be great to get in. KMP would be only targeting static context (aka client) SDKs anyways right? I would only want to make sure that the effort maintaining the SDK does not dramatically increase if the SDK targets all KMP platforms. I guess we need solid test runs on all of the platforms then right? |
@lukas-reining I'm with you, the static-context (aka client) paradigm sounds very reasonable to me as the current focus for the Kotlin SDK. If the community around this repo grows, I could imagine an implementation for the dynamic-context paradigm as well, pretty much like for JS.
To JetBrains's credit so far it's been light sailing, I haven't needed to write a single line of platform-specific code 🎉 I believe it's not gonna get particularly complicated for maintaining this SDK, given that it contains more interfaces than actual implementations. The test infra will definitely need to be there, I think the current tests will make it for all platform given that I haven't added any platform-specific stuff. The browser test needs to be fixed by installing some browser in the CI, but that's not a big deal either. For the Providers, on the other hand, it might be challenging to support multiple platforms based on how they are implemented:
Ultimately, the authors of the Providers can decide, which platform they are targeting, it's not a requirement by any means for them to target all platforms supported by the SDK. The best thing we can do on the SDK side is to support as many platforms as we reasonably can, allowing the Provider authors to target all the platforms they like. |
I'll be AFK for the rest of the week, but I'm planning to come back to this topic next week to make sure that all parts are up to speed before merging the PR. |
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
a9e2723
to
1a76c44
Compare
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
While I support this PR I am having problems building this PR with Java SDK 11 while it is working for Java SDK 17. I would also like us to think if we are willing to keep go to some extra length to provide (publishing to maven central) the |
Yes, since AGP 8.0 JDK 17 is a build time requirement. |
@realdadfish that requirement is related only to the Gradle JVM, and not to the Toolchain JDK as far as I understand. I have checked some popular Android libraries regarding the JVM settings, all the ones I checked are targeting JDK 8:
So I'd not increase the |
@nicklasl since this PR is already bringing in some breaking changes (renaming of the base package, the replacement of So instead, I'd suggest relocating the artifact on Maven Central like this: |
Yes, I agree. I tried it out using the code below inside `publishing` and did `publishToMavenLocal` and it seems to be legit.// --- Relocation publication for the OLD 0.4.1 artifact ---
// This will publish a POM for dev.openfeature:android-sdk:0.4.1 that redirects.
create<MavenPublication>("relocationForOldAndroidSdk0_4_1") {
// Define the OLD coordinates for this publication
groupId = "dev.openfeature" // The old groupId
artifactId = "android-sdk" // The old artifactId
version = "0.4.1" // The specific old version being relocated
pom.withXml {
asNode().appendNode("distributionManagement").appendNode("relocation").apply {
// Point to the NEW coordinates
appendNode("groupId", "dev.openfeature")
appendNode("artifactId", "kotlin-sdk")
// Point to the specific version of the new artifact you want users to use
// when they try to resolve the old 0.4.1. In your case, this is 0.5.0.
appendNode("version", "0.5.0")
appendNode("message", "This artifact has been relocated. Please update your dependency to dev.openfeature:kotlin-sdk:0.5.0.")
}
}
} |
Signed-off-by: Bence Hornák <bence.hornak@gmail.com>
If there is interest, I'm happy to finalize these changes, as it is a dependency for the newly set up |
I was curious how much effort it would be to extend this SDK to multiple KMP platforms. It was surprisingly straight-forward, there are only a few references to the JDK in the project.
This PR
java.util.Date
withkotlin.time.Instant
and rename class accordingly fromValue.Date
toValue.Instant
(breaking change)dev.openfeature:android-sdk
todev.openfeature:kotlin-sdk
(breaking change)dev.openfeature.sdk
todev.openfeature.kotlin.sdk
in order to eliminate the conflict with the Java SDK (breaking change)Related Issues
Fixes #147
To do
android-sdk
tokotlin-sdk
Android
toKotlin
(the URL already says/kotlin
)dev.openfeature.sdk
todev.openfeature.kotlin.sdk
Add a multiplatform example app-> not that simple as I thought, would dedicate a separate PR for thisHow to test
Prerequisite for browser tests: Install Google Chrome (or Google Chrome headless)
To run tests on all supported platforms: