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

Migrate to Instant from Date #150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sorikoff
Copy link

This supports recent major overhaul of code by replacing legacy Date with modern date and time API (Instant in this case). Older Android versions are supported with help of Java 8 desugaring.

@Jawnnypoo
Copy link
Member

Nice! We would want to really make sure this works with pre-23 devices before shipping, since I have seen cases where the code will compile and show no issues, but then when you get it on an actual device, it crashes at runtime.

@@ -15,6 +15,7 @@ android {
aarMetadata {
minCompileSdk = rootProject.ext.compileSdkVersion
}
multiDexEnabled true
Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I looked here. Do you know if it is wrong?
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see! I forgot this project was using minSdk 14. It should probably be bumped to 21 😅

@Jawnnypoo
Copy link
Member

This is a pretty large API change that affects all downstream users, so @kaushikgopal would need to really think about if this is worth the "breaking" change. I do like the benefits that Instant brings, and this might be the best opportunity to change over, with a 4.0 release.

@kaushikgopal
Copy link
Collaborator

thanks @Sorikoff thank you for the PR 🙏🏽 . some initial thoughts:

  1. multiDexEnabled & minSdk 14 - also cc @Jawnnypoo . I'm torn. I feel there might be many users out there wanting to use TrueTime with a lower SDK. Being a library, I wonder if we should be conservative and keep it. If anything, with the removal of Rx I wonder if we can push this even back.
  2. To Jawnny's point, I do worry that most folks haven't switched to Instant as being the default. For better or worse Date is the defacto usage 🫤. We wouldn't want people to import TrueTime and go through hoops to get the basic usage going.

Curious what you folks think

@kaushikgopal kaushikgopal mentioned this pull request Mar 29, 2023
@Sorikoff
Copy link
Author

@kaushikgopal 👋
Desugaring is available for 2+ years already, I think. And some solutions exist for even longer time... so with this major update it might be the right time to migrate 🤔

@Ke911
Copy link

Ke911 commented Apr 18, 2024

Hh

@Ke911
Copy link

Ke911 commented Apr 18, 2024

Vbdbjjdbjjsv I

@kaushikgopal
Copy link
Collaborator

I actually like tbr desugaring solution 🤔. Not to mention Kotlin provides a bunch of solutions too as part of their time library. I can revisit this again.

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

Successfully merging this pull request may close these issues.

4 participants