island-time icon indicating copy to clipboard operation
island-time copied to clipboard

Basic Kotlin/Js support

Open seyedjafariy opened this issue 5 years ago • 9 comments

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

seyedjafariy avatar Jun 07 '20 11:06 seyedjafariy

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.

erikc5000 avatar Jun 08 '20 02:06 erikc5000

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?

seyedjafariy avatar Jun 08 '20 09:06 seyedjafariy

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.

erikc5000 avatar Jun 08 '20 17:06 erikc5000

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.

seyedjafariy avatar Jun 09 '20 12:06 seyedjafariy

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.

erikc5000 avatar Jun 10 '20 16:06 erikc5000

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.

seyedjafariy avatar Jun 21 '20 09:06 seyedjafariy

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.

erikc5000 avatar Jun 22 '20 23:06 erikc5000

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?

seyedjafariy avatar Jul 04 '20 09:07 seyedjafariy

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.

erikc5000 avatar Jul 05 '20 14:07 erikc5000