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

New Release & FFI #76

Open
dickermoshe opened this issue Jan 23, 2025 · 33 comments · May be fixed by #81
Open

New Release & FFI #76

dickermoshe opened this issue Jan 23, 2025 · 33 comments · May be fixed by #81

Comments

@dickermoshe
Copy link

Timezones have changed in the past 19 months
@Pante Can we get another release?

Additionally, would you be interested in a PR which would get Timezones from the system using FFI.

Having to update software whenever the tz databases changes is a big pain.
On the other hand, sugar works on devices that don't get system updates.

@Pante
Copy link
Member

Pante commented Jan 24, 2025

Sure! I'll be honest I'm surprised that there's people using Sugar. I'll probably skip 2024 and use the 2025 version.

Additionally, would you be interested in a PR which would get Timezones from the system using FFI.

Do you mean retrieving the TZ databases from the OS? I think having an option to do that would be awesome but I'm not too sure how difficult it will be.

@dickermoshe
Copy link
Author

I'm having a blast writing the FFI for it.

However, windows doesn't contain a full IANA database, so it won't be possible to add it to this package without breaking changes.

I'm going to add this to the timezone package instead because it doesn't expose all the info in the IANA db, after we'll see how it could be added here.

Also, because things in this package are int UintList to save space, it's hard to wrap my head around what goes where.

This is a great paackage, I'll try to add it here after.
ForUI is really nice too. Thanks!

(A Nested header widget wont show a title if a leading or trailing widget aren't provided tho. Too lazy to open a bug report)

@Pante
Copy link
Member

Pante commented Jan 24, 2025

Ah I think breaking changes are totally fine! The next slated release is actually Sugar 4.0.0 which introduces a couple of breaking changes, a few more won't hurt.

The UintList will probably change in the new release as I completely overlooked dart2js not supporting longs natively.

@dickermoshe
Copy link
Author

Also windows contains NO historical data

@dickermoshe
Copy link
Author

So i guess the ransition list is not super acurate

@Pante
Copy link
Member

Pante commented Jan 24, 2025

Perhaps it might make sense to default to the baked-in tzdb if the OS's tzdb cannot be found or something similar.

@dickermoshe
Copy link
Author

Also windows contains NO historical data

This means that windows thinks that there was DST in the USA in the year 1700.

@dickermoshe
Copy link
Author

This is more than just a breaking change in terms of syntax.
The timezone package already supports multiple timezone databases, one of which is one with no historical data.

@dickermoshe
Copy link
Author

Seems that sugar is the only library which even attempts to handle in-between DTS transitions, gonna work on this repo

@dickermoshe
Copy link
Author

Some issues near the transition point, but I think I've got it working!

@dickermoshe
Copy link
Author

@Pante
I found a bug in the the your library.

After the year 2038, DST is permanently set.

final ianaTz = Timezone.timezoneProvider['America/New_York']!;
var dt = DateTime.parse('2038-03-14 03:00:00.000');
while (dt.year < 2050) {
  final ianaSpan = ianaTz.span(at: dt.microsecondsSinceEpoch);
  print(ianaSpan.offset);
  dt = dt.add(const Duration(minutes: 30));
}

This will always print -5 hours.

@dickermoshe
Copy link
Author

I'm working on a replacement for the timezone library.

I have FFi working with java, and hopefully will have it working with objc too.

Linux support is much more complicated to integrate.
For now linux will use a included database until native assets goes stable and I can use another C library to offload the heavy lifting.

Windows doesn't include a IANA database, so that will use this included database permanently.

Not sure about web yet...

@dickermoshe
Copy link
Author

@Pante What's alos cool is that the results from all the implementations match exactly.

Sugar has a 'java' like API, and now the results from it match Java 10000%

@Pante
Copy link
Member

Pante commented Feb 5, 2025

I copied the timezone logic from Joda Time since I'm too dumb to figure it out on my own. That said, I think they made some really good API design choices. Specifically not relating local dates & timezoned dates.

@dickermoshe
Copy link
Author

The web interop is surprisingly easy, users will have to include a polyfill for temporal

https://www.npmjs.com/package/@js-temporal/polyfill

Eventually temporal will be included in every browser

@dickermoshe
Copy link
Author

I copied the timezone logic from Joda Time since I'm too dumb to figure it out on my own.

I did the same, reverse engineering someone else's code is way quicker and accurate that trying to figure it all out yourself.

@dickermoshe
Copy link
Author

dickermoshe commented Feb 6, 2025

This is the library which will replace timezone

https://github.com/dickermoshe/dateutil

Currently:

  • Android: Java interop.
  • iOS: Bundled Database. Interop is a WIP.
  • MacOS: Bundled Database. Interop is a WIP.
  • Web: Js Interop using Temporal Polyfill.
  • Windows: Bundled Database.

@Pante
Copy link
Member

Pante commented Feb 6, 2025

Doesn't jni_gen depend on flutter? I remember experimenting with it and it was a deal-breaker since Sugar should work without a dependency on Flutter.

@dickermoshe
Copy link
Author

jnigen works with vanilla dart too, it just needs a JVM to talk to.
On vanilla dart some setup is needed to make them talk to each other.
However on android it just works automatically.

The Java version will only be imported for Android platforms so this is not an issue.

Eventually everything will use ffi/jni and platform channels will be a thing of the past

@dickermoshe
Copy link
Author

Crap, objective_c introp requires flutter.
This will need to wait until native assets are stable.

Although, the timezone package as it currently stands is buggy, and mine is not.
It supports web and android natively and bundles a database for the other platforms.
lmk if you you want a PR to replace timezone

@Pante
Copy link
Member

Pante commented Feb 7, 2025

I'm a little confused as Sugar only depends on timezone for zicfile parsing. Does your package replace that?

@dickermoshe
Copy link
Author

dickermoshe commented Feb 7, 2025

This package offloads much of the timezone complexity to another package.
All this package has is a Timezone class which lists the available time zones and can get the offsets.

final allTImezones= Timezone.list();
final tz = Timezone("America/New_York");
final now = DateTime.utc();
final offset = tz.offset(now.millisecondsSinceEpoch);
final localTimezone = now + Duration(milliseconds: offset);

For platforms that we can get the timezones natively, we do that. Otherwise we bundle a timezone database.

It depends on the very mature https://github.com/kshetline/tubular_time_tzdb project to compiles the time zone database to JSON. The timezone package is buggy and should be avoided.

My package uses that generated JSON for platforms which we don't have a platform interop for. I've tested the results against Java from years -100 to 2500 in all time zones and everything is EXACTLY the same.

It's hard to get an idea what's going on in the repo because I don't check in the json into GitHub, it's generated by CI so that new versions of the IANA database will break tests.

@dickermoshe
Copy link
Author

GOOD NEWS

I managed to use C interop on MacOS, this should work for iOS too!

@Pante
Copy link
Member

Pante commented Feb 9, 2025

I think I'm going to publish Sugar 4 first, it seems like the outdated packages are causing dependency issues for end-users downstream in Forui.

@dickermoshe
Copy link
Author

Can you give me an hour to create a PR for sugar.
My universal implementation of timezones is ready.
FFI will be added later

@Pante
Copy link
Member

Pante commented Feb 9, 2025

I don't think it needs to be that rushed since Forui has a monthly release cadence. I assumed that the PR was still a long time awhile, and wanted to give a heads up.

That said, it would be great if you can submit a draft PR first so it's easier to understand the scope of the changes, and pivot if necessary.

@dickermoshe
Copy link
Author

Thanks

@dickermoshe
Copy link
Author

dickermoshe commented Feb 9, 2025

I opened a draft PR: #80
I still need to fix a couple broken tests.

There is still more work to do.
I don't want to keep people waiting.
Release the latest version of sugar you have and we'll work on this next

Side note:
Image

@Pante
Copy link
Member

Pante commented Feb 9, 2025

I took a look at the PR. It is safe to assume that it's meant to be a replacement for the low-level portions of the library? It might make more sense to spin-off Sugar's date-time library and merge it with your low-level portion under a new library.

@dickermoshe
Copy link
Author

@Pante Instead of me guessing what you want, let me ask you want you want.

The way I see it there are issues with the current state of sugar:

  1. You must include a copy of the timezone database with your app. The problems with this is:
    a. You must take care to update your app whenever timezones change. In some instances there can be very little time between a government changing their policies and the date they take effect.
    b. The size of of your is increased. This is especially problematic for Flutter Web.
  2. The timezone package which sugar relies on is broken. Dates after 2038 return invalid results.
  3. Currently sugar uses precalculated spans to find the offset for a specific date ([1680000000000,1689876547000,...]), However this has 2 problems:
    a. It's much larger than it needs to be. The initial timestamp should be stored and then the deltas for the next timestamps ([1680000000, 365000,312000,...])
    b. We need to calculate offset into the far distant future without using DST rules. For instance, for years after 2100, which we don't have any data for we use general rules to calculate the offset (e.g "2nd Sunday of March"). If we were to calculate every offset until the end of time, this would make the database 1000x lager.
  4. We need improved testing against a known working timezone library.

How do you want to solve this?


My thinking was that timezones and FFI/JNI are complex topics that should be offloaded to another package. This will especially make CI simpler because there are so many unique environments that each implementation need to be tested on.
We solve the above issues as follows.

  1. Use FFI/JNI where ever possible to avoid including a database with every app.
  2. Use a know working timezone compilation tool to do the heavy lifting of working with the database from the iana.
  3. Use deltas and DST rules to calculate the offset for the included database. This increases accuracy and limits the bundle size.
  4. Test against java.time which has no bugs.

I hate writing code that doesn't get used. Creating my own date-time would be a waste of resources if we can get sugar working.

@Pante
Copy link
Member

Pante commented Feb 10, 2025

You must include a copy of the timezone database with your app. The problems with this is:
a. You must take care to update your app whenever timezones change. In some instances there can be very little time between a government changing their policies and the date they take effect.

I think it's a trade-off, by embedding a database, you know exactly what tzdb you're working against. Relying on an external tzdb can cause bugs if the tzbd on a local device is different from the server's. There's also the issue of reliably finding the system's tzdb.

That said, choosing between an embedded tzdb and finding the system tzdb should not be mutually exclusive. I think we should offer both and let the user choose.

b. The size of of your is increased. This is especially problematic for Flutter Web.

I genuinely wonder how much the library size will increase by after tree-shaking. I personally don't think it will increase it too much but we'll need to measure it.

The timezone package which sugar relies on is broken. Dates after 2038 return invalid results.

I opened an issue upstream and expect this to be fixed. If they can't/don't, we'll reevaluate then.

Currently sugar uses precalculated spans to find the offset for a specific date ([1680000000000,1689876547000,...]), However this has 2 problems:
a. It's much larger than it needs to be. The initial timestamp should be stored and then the deltas for the next timestamps ([1680000000, 365000,312000,...])
b. We need to calculate offset into the far distant future without using DST rules. For instance, for years after 2100, which we don't have any data for we use general rules to calculate the offset (e.g "2nd Sunday of March"). If we were to calculate every offset until the end of time, this would make the database 1000x lager.

I think you brought up a fair point and it'll be much more compact if we calculate timestamps on the fly. I honestly have no clue how to do that and I'm open to suggestions.

My thinking was that timezones and FFI/JNI are complex topics that should be offloaded to another package. This will especially make CI simpler because there are so many unique environments that each implementation need to be tested on.
We solve the above issues as follows.

I think that merging the lower and higher levels together will actually make CI much easier since you can rely on the higher level types/utilities while writing tests. This will lead to less duplicated code. Not to mention they'll also serve as an indirect form of integration test between the higher and lower level parts.

Use a know working timezone compilation tool to do the heavy lifting of working with the database from the iana.

Do you have any tool in mind? I looked at tubular_time_tzdb. It seems like a relatively new library. Not to mention, the bus factor for that library seems really low, and whether the library will be maintained in the long run seems up in the air.

Use deltas and DST rules to calculate the offset for the included database. This increases accuracy and limits the bundle size.

I agree.

I hate writing code that doesn't get used. Creating my own date-time would be a waste of resources if we can get sugar working.

I'm open to most of the changes that you've proposed. My main objection is splitting the FFI portion and higher level portion into separate GitHub repositories. I think it will be detrimental to both sides.

@dickermoshe
Copy link
Author

dickermoshe commented Feb 10, 2025

Fantastic.

I'm going to create a pretty large PR for this

@dickermoshe
Copy link
Author

@Pante Thanks for getting that forui nested styles thing added!


Please take a look at this PR and give me your early thoughts.
If you don't like anything about it, I'll fix it.

I still need to add java testing to CI which will rigorously test sugar against java.time.
FWIW, on my personal computer it all passes.

@Pante Pante linked a pull request Feb 11, 2025 that will close this issue
1 task
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 a pull request may close this issue.

2 participants