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

Basic Kotlin/Js support #78

Open
wants to merge 267 commits into
base: master
Choose a base branch
from

Conversation

seyedjafariy
Copy link

Added basic support for JS targets.

I call it basic because there are a ton of things to be improved. I mostly made it just work with the target.
The reason for going with the basic implementation is mainly because I'm Android dev and not sure how do JS devs prefer those Locale and TimeZone stuff.
With Moment-TimeZones we can add all the TimeZone db but I assume the size of the lib will come to concern.
I backed most of the expected functions with momentJS since I found it very popular.
Also With the help of FixedOffset type provided by Island-time, I bypassed all the TimeZone rules.

There are two incomplete parts with this PR: JsTest, JsMath.
The work that needs to be done to get these working is a bit trivial. I did not know if the PR will be accepted or not so I decided to let you guys review first and see what you think then if decided I can fix all of them before merging.
One note about tests though: In order to make JSTest work I have to change all of the commonTest function names and remove spaces from them. As spaces in function names are not supported for Kotlin/JS.

Thanks in advance for taking the time and reviewing

…ilitate alternate clock sources in the future (ie. kotlin stdlib)
erikc5000 and others added 17 commits May 18, 2020 22:12
* Fix random() behavior on ranges, add randomOrNull()

* Fix some edge cases with intervals

* Move parts of randomInternal() to separate, non-inline functions

* Refactor to further reduce code duplication
Fix publishing of atomicfu transitive dependency on native
* Add IDEA code style to VCS

* Restore 120 column width
* Remove duplicated toComponents() code in UtcOffset

* Remove duplicated toComponents() code in Duration
@erikc5000
Copy link
Owner

First off, I just want to say that it's really cool to see someone taking a stab at JS support. Definitely an ambitious PR! 🙂

I did some investigation into JS support a while back. It's been a bit now, but as I recall, the Intl APIs available in JavaScript have come a long way over the years and are likely sufficient without external dependencies, like Moment. All the browsers now include a full time zone database to support the Intl APIs and I believe that's also the case in the newest versions of Node.

For example, to get the current time zone is pretty simple: Intl.DateTimeFormat().resolvedOptions().timeZone

Island Time's Locale can probably just be typealiased to Intl.Locale.

You can really do a lot with Intl.DateTimeFormat. Using formatToParts(), it should be possible to get at all of the localized text and time zone data that's needed. This is what Luxon does -- a newer library by one of the maintainers of Moment.

So basically, I'd prefer not to have a dependency on Moment in the Island Time core and instead rely on the built-in APIs -- even if they do have some limitations.

The overflow-safe math function implementations for Darwin might work for JS too. Not sure though. JS is funny when it comes to numbers.

As far as the test names go... yeah, they're a problem. I started out using back ticks for the nicer names and realized after that it would be a problem for JS someday, but I didn't feel like changing everything at that point. 😛 I wonder if it'd be possible to write a utility that automatically adds/updates a sanitized @JsName() to everything annotated with @Test. Compiler plugin would be even better, though likely more work. I can explore that.

Anyway, there's a lot of functionality required to get JS to parity with the other platforms and I think having that span multiple PRs would be acceptable. But it does have implications on how all the tests are organized if the common tests can no longer pass on JS.

Focusing on time zone database support first probably makes sense since it's a pretty critical piece. I worry that trying to just do fixed offsets doesn't offer enough value for the effort involved in shuffling tests around. All of the locale-related stuff could be replaced with TODO() and dealt with later. The tests that exercise locale should be simpler to isolate, I think.

It'll be easier to deal with differing capabilities between JS and JVM/Darwin when Kotlin 1.4 comes out with hierarchical project support. That doesn't necessarily mean that this should wait though.

@seyedjafariy
Copy link
Author

seyedjafariy commented Jun 8, 2020

Wow, thanks for the kind words.

You just gave me a lot of homework!! Finally some reason to start looking at JS! I also did not know anything about the Intl thanks for directing.

I looked at the Intl.Locale. looks good except that we need "cutting edge" browsers to run it. I'm sure the support will grow and As you said it's more than enough for our purpose.

So I'll study on JS and these APIs. I did not find any Kotlin wrapper so I have to write them too.
I'll try to update this branch as I go so everyone can have a look.

Finally, on Testing, I'll leave them, for now, to see how does the integration with Intl goes and then we cover them in this or next PRs per your suggestion.

The hierarchical project support is really interesting but I don't understand the relation with Island-time and especially JS?

@erikc5000
Copy link
Owner

Good point on Intl.Locale. It might be fine to just require anyone using Locale to use a polyfill like https://www.npmjs.com/package/intl when the support isn't there. But it might make sense to also add JS-only extension functions that take a String directly. For example:

fun DayOfWeek.localizedName(style: TextStyle, locale: String): String?

Hierarchical project support allows organizing the source sets like:

                common
       -----------|-------------
       |                       |
   jvmNative                  js
       |
  -----|-----
  |         |
jvm       darwin

You can then have expect declarations in a mid-level source set, like "jvmNativeMain" instead of "commonMain", making it easier to deal with differing capabilities across platforms. For example, if JavaScript can't get the localized first day of the week, the declaration goes into "jvmNativeMain" and just isn't available in common code for anyone targetting JS.

It also makes it easier to share test code between different targets. Technically, you can group source sets like that already, but it breaks some of the IDE integration.

@seyedjafariy
Copy link
Author

Thanks for the clarification on the Hierarchical project support.

So I learned JS yesterday! (the basics of course) and managed to create the Kotlin header file for DateTimeFormat and other Intl classes using Dukat.

The API looks pretty good and it can handle Locale. the only concern is that we can not get list of timeZones nor we can not check if a timezone is available (except a poor try/catch mechanism) and if there is, generating the TimeZoneRules object looks hard.
So I wonder what is your suggestion in this regard cause unless we go with FixedOffset as before we need to cover these cases.

@erikc5000
Copy link
Owner

Being able to get a list of available time zones isn't important, really. It's more informational and it's fine if availableRegionIds just returns an empty set. Even on iOS, it doesn't actually return everything that's recognized, so Island Time relies on hasRulesFor() instead.

I think the try-catch is fine. Can implement caching later if performance is an issue. This seems to be what Luxon does:

/**
   * Returns whether the provided string identifies a real zone
   * @param {string} zone - The string to check
   * @example IANAZone.isValidZone("America/New_York") //=> true
   * @example IANAZone.isValidZone("Fantasia/Castle") //=> false
   * @example IANAZone.isValidZone("Sport~~blorp") //=> false
   * @return {boolean}
   */
  static isValidZone(zone) {
    try {
      new Intl.DateTimeFormat("en-US", { timeZone: zone }).format();
      return true;
    } catch (e) {
      return false;
    }
  }

Reconstructing the transitions caused by daylight saving time is a bit more complicated. While the information is there as part of the IANA TZDB data that backs the Date and Intl APIs, it's not fully exposed, so some hacks will be involved to get at it. For example, to get the local time in zone from an Instant (UTC timestamp), you might have to do something like this:

// Date is a JavaScript Date
const parts = Intl.DateTimeFormat("en-US", { timeZone: zone, calendar: "iso8601", hourCycle: "h23", year : "numeric", month: "numeric", day: "numeric", hour: "numeric", minute: "numeric", second: "numeric" }).formatToParts(new Date(islandTimeInstant.millisecondOfUnixEpoch)

// Then take the difference between the local time represented by "parts" and
// the original time to figure out the UTC offset

Luxon has code that does at least some of this stuff. If you follow the logic stemming from DateTime.local(), you can see how they're able to figure out the offset from a local time (which would be represented by DateTime in Island Time).

https://github.com/moment/luxon/blob/918cecdd28da706586f2d409e88efe7255f07eec/src/datetime.js#L461

When trying to find the offset from a local time, you could end up in an overlap or a gap (which they refer to as a "hole" in Luxon). Identifying those transitions is important for Island Time's time zone related functionality.

I can't deny that it's a bit of a project to get this all working though.

added Intl kotlin wrapper
added Intl support for most of the functions
@seyedjafariy
Copy link
Author

I refactored most of the code to Intl API. and removed the moment library.
But unfortunately did not manage to complete the task on my own!
some of the important functions like transitions and isInDaylight still waiting.
I pushed the changes so you could have a look too.
Would you please help me with how to progress? I tried to extract code from luxon/moment as much as possible but now again have no clue.

@erikc5000
Copy link
Owner

This seems to be a good start! The transitions are probably a pain to reconstruct. I'll try to go through the code more closely soon and provide some better feedback.

Copy link
Owner

@erikc5000 erikc5000 left a comment

Choose a reason for hiding this comment

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

There's a lot to digest, but I've tried to provide some feedback. I probably missed a lot, but it's a start anyway.

override fun rulesFor(regionId: String): TimeZoneRules {
val lowered = regionId.toLowerCase()
return when {
lowered === "local" -> LocalRules()
Copy link
Owner

Choose a reason for hiding this comment

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

Having separate "Local" and "IANA" rules doesn't really work in Island Time. You always need to have a specific identifier. Hard-coding a "local" region ID is not good.

Copy link
Author

Choose a reason for hiding this comment

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

The reason for "local" is because DateTimeFormat might return "local" as the timezone from the system.
I saw this behavior in Luxon.
I'm not really sure how we can remove "local"

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think "local" is ever returned by the system -- it's just something Luxon uses internally. The zone that's returned in resolvedOptions should be an IANA region ID.

core/src/jsMain/kotlin/io/islandtime/zone/TimeZoneRules.kt Outdated Show resolved Hide resolved
core/src/jsMain/kotlin/io/islandtime/locale/Locale.kt Outdated Show resolved Hide resolved
actual object PlatformTimeZoneTextProvider : TimeZoneTextProvider {
override fun timeZoneTextFor(zone: TimeZone, style: TimeZoneTextStyle, locale: Locale): String? {
//TODO not sure how to support generic ones
val zoneStyle = when (style) {
Copy link
Owner

Choose a reason for hiding this comment

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

Generic can return null if unavailable. It seems like this just arbitrarily returns either standard or daylight based on the current local date though, which will need to be fixed.

Copy link
Author

Choose a reason for hiding this comment

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

I just refactored generics to return null for now



//TODO replace with Math functions
internal actual infix fun Long.floorMod(other: Long): Long = ((this % other) + other) % other
Copy link
Owner

Choose a reason for hiding this comment

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

The native implementations of these might work for at least some of these checked math functions.

Copy link
Author

Choose a reason for hiding this comment

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

I refactored the top ones to use the Kotlin APIs.
But not sure how this will work for IslandTime implementation.
please let me know if there is anyother math function that I can use here

core/src/jsMain/kotlin/io/islandtime/intl/dtf.Intl.kt Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
package io.islandtime

fun <T : Any> objectOf(body : T.() -> Unit = { }) =
Copy link
Owner

Choose a reason for hiding this comment

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

What's this for?

Copy link
Author

Choose a reason for hiding this comment

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

it's used to create JS object in Kotlin land.
I copied it from Date.dateLocaleOptions() Kotlin API.
please let me know if there is an easier/better way?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not very familiar with Kotlin/JS, so I'm not quite sure. It just seems like a very general function that I would expect to be part of the standard library.

import io.islandtime.objectOf
import kotlin.js.Date

class DateFormatSymbols private constructor(
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't need a DateFormatSymbols class. It seems like this is over-complicated by reusing parts of the JVM implementation of PlatformDateTimeTextProvider that really shouldn't be copied. Needs to be thoroughly refactored, I think.

Copy link
Author

Choose a reason for hiding this comment

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

Are you suggesting refactoring IslandTime common code to avoid this?

Copy link
Owner

Choose a reason for hiding this comment

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

No -- DateFormatSymbols isn't part of the common code. That's a Java API that just doesn't need to be mimicked in the JS implementation.

@seyedjafariy
Copy link
Author

Sorry for the long delay
I applied all your comments.
please tell me if you have any idea on how we can create those Transitions?

@erikc5000
Copy link
Owner

Without really digging in and trying to write some code, I'm not sure I can provide much guidance on how to figure out the transitions beyond my earlier comment. The tricks Luxon is doing give some idea of how it might be done, but it's not copy-paste.

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.

2 participants