-
Notifications
You must be signed in to change notification settings - Fork 194
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
TrueTime 4.0 : Kotlin & coroutines #129
Conversation
replace with https://issuetracker.google.com/issues/72050365 once released.
Super excited about this! |
Is there any timeline for this PR? |
@kaushikgopal do we any timeline for this PR? |
🙈 i really don't want to give timelines (for fear of jinxing it), but we're actively working on the revamp. i'd imagine something close in the next few months. I'm hoping earlier but past experience has taught me to say otherwise. |
// implementation project(path: ':library-extension-rx') | ||
implementation 'com.github.instacart.truetime-android:library-extension-rx:3.5' | ||
|
||
implementation "org.jetbrains.kotlin:kotlin-stdlib" |
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.
This shouldn't be needed, it gets added by default when you apply the kotlin plugin
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.
🤔 AS is complaining when i remove this. I'm guessing i've stripped/messed with the dependencies during the kotlin migration. i'm going to leave this one as is. once we have it merged, happy to try again and clean this up.
dependencies { | ||
repositories { | ||
mavenCentral() | ||
} |
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.
I don't think this should be needed, since its specified in allProjects
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.
same as #129 (comment)
import kotlinx.coroutines.coroutineScope | ||
import kotlinx.coroutines.selects.select | ||
|
||
class TrueTimeImpl( |
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.
Since this is the one people will use, I think it might be clearer to name this one TrueTime
and name the interface TrueTimeBase
or something similar, so that TrueTime
is the more discoverable one
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.
hmm... i prefer having TrueTime
be the interface mostly cause that's what people will be inclined to use and pass around more. It being an interface means they will use the interface in most places which is what we want 🙂. TrueTimeImpl
should probably just be used in one place and forgotten.
apple is nice and old with it's IP4 addresses
@vandac @stAyushJain : merged! you can take it for a spin and let me know what you observe |
Hello @kaushikgopal @Jawnnypoo, No more major overhaul of this library ? is it still supported or can be used ? or is it dropped entirely ? Thank you for your replies :) |
Starting a branch to move the implementation purely into Kotlin + coroutines
While @Jawnnypoo painstakingly migrated
TrueTime
&TrueTimeRx
to java, I wanted to keep those as is and rethink the api altogether. So keeping only the newer redesigned parts in Kotlin.Changes
1. (almost) 100% Kotlin
Instacart is strongly committed to a Kotlin future. Moving TrueTime to Kotlin will 🤞🏽 make the maintenance of this library easier possibly encouraging external contributions and the improvement of this library. Anecdotally many folks have mentioned that they would be more inclined to dive in and contribute to TrueTime if it was in Kotlin (vs Java).
Notably
SntpImpl
remains in .java. This was originally forked from AOSP and most other implementations you find online are pretty similar to the original AOSP implementation. For this reason, we chose to intentionally just keep this in Java. We have however cleaned it up and extracted a lot of the logic we'd previously nested in this class.2. Coroutines
Coroutines comes baked in with Kotlin. As Google continues to push Coroutines more heavily, there's a higher likelihood that an app would already have Coroutines than it would Rx. We want Truetime to be easily integrated into an app pulling minimal external dependencies along the way.
3. Isolating Android dependencies to a single package
We previously distributed two versions of TrueTime & TrueTimeRx. We're aiming for TrueTime to be pure .kt without any android dependencies/requirements. For distribution reasons, we're going to keep it simple and have it included with TrueTime.
4. Numerous Breaking API changes
The above changes were going to be a major overhaul. Given the inevitable breaking changes, we wanted to take this chance and rethink the API from scratch anyway, making it more nimble/modern. Will update the README with the newest changes.
TODOs: