Support for Kotlin Multiplatform

maxspencer commented 5 years ago

Hi there, this is my first time contributing to this project, but I hope I can add some value 👋

Introduction

Here at The Guardian we have a tool called Ophan to which we send analytics data using Thrift. I've recently been working on a research-heavy project where I've been trying to write a cross-platform client library for this tool, Ophan, using Kotlin Multiplatform.

I have been able to do this 👉 multiplatform-ophan 👈 and Thrifty, with some important modifications, was an important part of the solution. I'd like to investigate whether there's a way we can incorporate those modifications back into this repo so that Thrifty can be used in other Kotlin Multiplatform projects.

Requirements

Briefly, Kotlin Multiplatform projects consist of some "common" code which can be compiled for, and run on, any of the supported platforms, as well as platform-specific portions of code which are only used on a single platform. Common code is Kotlin, but with only acces to the "common" parts of Kotlin's standard library and none of the Java standard library (for example there is no java.util.LinkedList) because it must be able to run in non-JVM environments. This obviously limits not only the code you can write yourself, but what your common code can depend upon.

If we consider Thrifty specifically, there are two key requirements for making it usable in this situation:

The Thrifty compiler must output Kotlin code which does not import any classes which are not available in the common Kotlin standard library, and
The Thrifty runtime must abide by the same restrictions.

Proposed solutions

Requirement 1

This is actually really easy to solve with the existing compiler options!

java -jar thrifty-compiler.jar
    --lang kotlin
    --omit-generated-annotations
    --list-type=kotlin.collections.ArrayList
    --set-type=kotlin.collections.LinkedHashSet
    <other options/params>

I've used this successfully in multiplatform-ophan here:

https://github.com/guardian/multiplatform-ophan/blob/master/build.gradle.kts#L86

My only question here is whether it's worth adding a new --lang kotlin-multiplatform option which sets some of those other values automatically?

Requirement 2

This is trickier. Essentially you have to take the existing thrifty-runtime source and modify it to first of all be Kotlin and secondly, use only valid common Kotlin dependencies.

I have done this, here:

https://github.com/guardian/multiplatform-ophan/tree/master/src/commonMain/kotlin/com/microsoft/thrifty

but I have only included the CompactProtocol and I have not created tests for this code. However, I hope this does provide a solid basis for a thrifty-runtime-ktx-multiplatform module in this repo.

Next steps

Really up to you as the project maintainers to decide the best way to proceed. I hope this can be of some use to this project and I look forward to hearing from you.

benjamin-bader commented 5 years ago

Oooh, this is interesting! Thank you for sharing what seems to be a considerable amount of good work. At first blush, multiplatform seems like a clear win. I've never used kotlin multiplatform, so I'll need to dig in to the code.

In theory as long as the transports (the only real platform-dependent types we have) are good, then the rest of the protocols should just work as-is.

The only reservation I have is backwards-compatibility - I'd prefer not to break existing Java consumers. I've been reluctant to push a kotlin dependency on the Java runtime, but given that okio already does, we may as well follow suit. Given a port to Kotlin, it seems like the only remaining questions are: how to achieve code reuse between

It feels like we could integrate multiplatform and preserve Java consumers by putting your code into the thrifty-runtime-ktx module. Ideally we wouldn't have to duplicate implementations of tricky things, but given that the protocols are basically stable it wouldn't be the end of the world to have two copies of the runtime.

TL;DR I'd want to see (or see a path leading to) the following before we ship this:

[ ] Backwards-compatibility for Java-based consumers;
[ ] Integration testing (could probably just be another conformance test module for the runtime, but I'm not sure how we'd handle the Android and especially iOS bits. Note that we're constrained to use Travis CI here, for various reasons)

benjamin-bader commented 5 years ago

Despite the wall of text, I want to stress that this is exciting work that I would like to see land in the project - the only questions are "how to", not "should we".

microsoft / thrifty