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

Make JS TimeZone support easier to integrate #178

Open
wkornewald opened this issue Jan 10, 2022 · 12 comments
Open

Make JS TimeZone support easier to integrate #178

wkornewald opened this issue Jan 10, 2022 · 12 comments
Labels
timezone The model and API of timezones

Comments

@wkornewald
Copy link
Contributor

wkornewald commented Jan 10, 2022

Currently, in order to use time zones in JS you have to add an NPM dependency and custom code:

@JsModule("@js-joda/timezone")
@JsNonModule
external object JsJodaTimeZoneModule

private val jsJodaTz = JsJodaTimeZoneModule

Would it be possible to just provide a kotlinx-datetime-timezones module which does both steps automatically, so all you need to do is add the module dependency? Then no extra code would be needed and in theory this module could also provide time zone databases for other platforms where you'd also need extra dependencies and init code.

@dkhalanskyjb
Copy link
Contributor

The JS timezone support does not seem that cumbersome—as you show, it's just four lines, listed in our README.

Providing our own package for this, however, is not as straightforward as it seems. In JS, for example, there's sometimes the need to aggressively reduce the size of the package, given how it is transferred to the browser, so a reduced timezone database may be used: https://github.com/js-joda/js-joda/tree/master/packages/timezone#reducing-js-joda-timezone-file-size We haven't evaluated how much of a problem this is for Kotlin+JS projects, but if it is a problem, we would need to provide reduced-size packages as well. Additionally, the code is not ready for supporting timezone databases loaded from the outside, and we're not sure about the API for choosing which database to use—the system one, if present, or the loaded one, if present, or maybe something else?.. Should we publish a new library release each time a new timezone database is released? If so, we would need some automation for this. Etc.

So, not at all simple. There are more pressing matters, like formatting, adding LocalTime, etc., so spending all the effort just for this doesn't seem optimal.

@wkornewald
Copy link
Contributor Author

Absolutely, this is just a minor feature request. :)

@set321go
Copy link

It would be useful if this information was much clearer in the documentation, I only worked out why my timezone lookups were failing from this ticket

@fluidsonic
Copy link

Workaround no longer worked for me in Kotlin 1.6.20 (IR compiler).

This works:

@JsModule("@js-joda/timezone")
@JsNonModule
private external object JsJodaTimeZoneModule

@EagerInitialization
@OptIn(ExperimentalStdlibApi::class)
@Suppress("unused")
private val init = JsJodaTimeZoneModule

@dkhalanskyjb
Copy link
Contributor

Strange, in my toy project where I smoke-test datetime, this is not required, JS-IR + 1.6.20 recognizes the time zones just fine with the old trick, when testing in both Node and the browsers.

@fluidsonic
Copy link

fluidsonic commented Apr 11, 2022

Might have been because the respective module is loaded dynamically in my project.

@fluidsonic
Copy link

Also, it only happened when deploying the final build including Webpack & terser optimizations. The library was then optimized away.

@dkhalanskyjb
Copy link
Contributor

Does the problem persist if you don't mark the external object as private?

@fluidsonic
Copy link

Unfortunately I've got no time for additional tests here

@set321go
Copy link

This is how I have it setup (implementation(npm("@js-joda/timezone", "2.12.0")))

package lib.timezone

@JsModule("@js-joda/timezone")
@JsNonModule
external object JsJodaTimeZoneModule

val jsJodaTz = JsJodaTimeZoneModule

The generated js has require('@js-joda/timezone') which then internally is initialized by

  var jsJodaTz;
  var properties_initialized_JodaTimeZone_kt_1382912675;
  function init_properties_JodaTimeZone_kt_4282475903() {
    if (!properties_initialized_JodaTimeZone_kt_1382912675) {
      properties_initialized_JodaTimeZone_kt_1382912675 = true;
      jsJodaTz = JsJodaTimeZoneModule;
    }
  }

I've had problems like this with other libs and I find the best debugging path is to look at the generated code in build/js/packages

@brianguertin
Copy link

brianguertin commented Jul 13, 2022

For me, the instructions in the README are working for a debug build, but not in webpack release build. In the release build only, no time zones can be found.

The workaround that worked for me is to add a reference to the timezone module in the function where I'm accessing it. I suspect the top-level one in the README is not getting loaded in release mode?

val forceModuleImport = JsJodaTimeZoneModule
ZoneId.of(timezone)

@mikedawson
Copy link

I experienced the same issue and did some further testing: making the val jsJodaTz public (instead of private) does not fix it. Accessing the property from the main function fixed it - e.g.

fun main() {
    ...
    //Avoid joda time timezones being DCE'd out  in release-
    jsJodaTz
}

@EagerInitialization might be a fix: but I see this is already deprecated. Should this be filed as a case for keeping EagerInitialization?

In the meantime: the README should be updated. I can make a pull request for that if it helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timezone The model and API of timezones
Projects
None yet
Development

No branches or pull requests

7 participants
@fluidsonic @brianguertin @wkornewald @set321go @mikedawson @dkhalanskyjb and others