feat: add timeZoneName props #744

wood1986 · 2023-03-25T18:46:41Z

Summary

This PR tries to fix the followings timezone related bugs

timeZoneOffsetInMinutes does not support daylight saving.
timezone offset on android is broken #528
Issue with timezones on iOS #61

closes #547, closes #61, closes #528, closes #114

My solution to the timezone issues is to let OS interpret time zone by passing a timeZoneName props. JS side should not re-interpret or reconstruct JS Date object based on year, month, day, hour and minutes. Android is doing this and iOS is not. the JS should only need to care UTC timestamp like what iOS is doing.

All the timezone related operation should be handled on native side because OS has the full knowledge of all timezone and library.

The timeZoneName is based on IANA See https://www.iana.org/time-zones in both Android and iOS. But they are not completely identical. It has around 4XX in common. See https://gist.github.com/mteece/80fff3329074cf90d7991e55f4fc8de4 for NSTimeZone in Objective C and https://garygregory.wordpress.com/2013/06/18/what-are-the-java-timezone-ids/ for TimeZone in Java.

Test Plan

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

OS	Implemented
iOS	✅
Android	✅

Checklist

I have tested this on a device and a simulator
I added the documentation in README.md
I updated the typed files (TS and Flow)
I added a sample use of the API in the example project (example/App.js)
I have added automated tests, either in JS or e2e tests, as applicable

wood1986 · 2023-03-25T23:20:05Z

I want to get a feedback from maintainer before writing the test

wood1986 · 2023-03-29T12:54:21Z

Hey @vonovak could you please take a look? Thanks

vonovak · 2023-04-03T14:50:05Z

hello and thanks for the PR!

I'm a little swamped right now with other work that pays the bills and the PR is rather large so it will take me some time to get back to this.

Thank you for you patience! 🙂

wood1986 · 2023-04-14T22:05:52Z

Any update?

wood1986 · 2023-05-08T06:10:44Z

Hey @vonovak, It has been a month. Any update? Thanks

vonovak

Hello and thank you for your PR!
firstly, thank you for taking a stab at this long-term issue. It is one that I was planning on tackling but didn't manage to get to it due to being involved elsewhere. Also, thank you for your patience with getting back to you.

I do agree with the approach of removing JS from the equation and having the date logic live in the native land, so that's good!

I added a few comments that should result into making the code better.

When they are resolved, docs need to be updated as well. CI also needs to be green before merging.

To wrap up, this is a good direction AFAICT. Please address the review / reply to the comments if you disagree with them. Looking forward to merging! 🚀

vonovak · 2023-05-12T08:36:45Z

src/index.js

@@ -3,7 +3,6 @@
 * @flow strict-local
 */
 import RNDateTimePicker from './datetimepicker';
-export * from './eventCreators';


this would be a change that is more breaking than needed, because the event creators are recommended to be used in tests that people may have, and they are also used in https://github.com/react-native-datetimepicker/datetimepicker/blob/master/test/userlandTestExamples.test.js

I believe it makes sense to keep them and just modify them as needed

vonovak · 2023-05-12T08:38:37Z

src/types.js

+   * parameter, it is possible to force a certain timezone offset. For
+   * instance, to show times in Pacific Standard Time
+   */
+  timeZoneName?: string,


can you please add this to BaseProps so that it's not duplicated twice?

vonovak · 2023-05-12T10:22:24Z

src/index.d.ts

+    /**
+     * The tz database name in https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
+     */
+    timeZoneName?: string;


please move to BaseProps, now it's only in IOSNativeProps

vonovak · 2023-05-12T10:23:39Z

src/DateTimePickerAndroid.android.js

 } from './androidUtils';
 import pickers from './picker';
-import {
-  createDateTimeSetEvtParams,
-  createDismissEvtParams,


please keep using these creators, as per the comment in index.js

vonovak · 2023-05-12T10:59:19Z

ios/RNDateTimePicker.m

+(NSSet *)knownTimeZoneNames {
+  static NSSet *knownTimeZoneNames = nil;
+  if (knownTimeZoneNames == nil) {
+    knownTimeZoneNames = [NSSet setWithArray:[NSTimeZone knownTimeZoneNames]];


can we please do without this? It's just copying an array into a set. For the check of whether a TZ is supported (in RNDateTimePickerManager.m), you can check the constructor return value: https://developer.apple.com/documentation/foundation/nstimezone/1387227-timezonewithname#return_value

android/src/main/java/com/reactcommunity/rndatetimepicker/TimePickerModule.java

vonovak · 2023-05-12T14:04:03Z

android/src/main/java/com/reactcommunity/rndatetimepicker/RNDatePickerDialogFragment.java

-      c.set(Calendar.MILLISECOND, 999);
-      datePicker.setMaxDate(c.getTimeInMillis() - getOffset(c, timeZoneOffsetInMilliseconds));
-    }
+    datePicker.setMinDate(minDate);


can we please call these setters only when min / max date was set in JS?
I'm not sure what side effects there are when calling these setters and what bugs there are behind it - Android is quite unpredictable in this and I'd like to avoid potential issues, thanks

vonovak · 2023-05-12T14:09:50Z

android/src/main/java/com/reactcommunity/rndatetimepicker/RNDatePickerDialogFragment.java

+        calendar.set(year, monthOfYear, dayOfMonth, 0, 0, 0);
+        long timestamp = Common.clamp(calendar.getTimeInMillis(), maxDate, minDate);
+        calendar.setTimeInMillis(timestamp);
+        datePicker.updateDate(calendar.get(Calendar.YEAR), calendar.get(Calendar.MONTH), calendar.get(Calendar.DAY_OF_MONTH));


can you please double-check and comment that there is not a loop in updateDate -> OnDateChanged -> updateDate ... ? thanks

vonovak · 2023-05-12T14:14:41Z

android/src/main/java/com/reactcommunity/rndatetimepicker/Common.java

@@ -28,6 +35,7 @@ public class Common {
  public static final String NEGATIVE = "negative";
  public static final String LABEL = "label";
  public static final String TEXT_COLOR = "textColor";
+  private static final HashSet TIMEZONE_IDS = new HashSet<>(Arrays.asList(TimeZone.getAvailableIDs()));


can we please avoid this, and do the check in getTimeZone? I'm sure there's some way to avoid this copy, thx

vonovak · 2023-05-12T14:20:24Z

android/src/main/java/com/reactcommunity/rndatetimepicker/KeepDateInRangeListener.java

-import java.util.Calendar;
-
-// fix for https://issuetracker.google.com/issues/169602180
-// TODO revisit day, month, year with timezoneoffset fixes


less is more 👍

wood1986 · 2023-05-13T08:28:51Z

I fixed the all comments and have not finished yet. Now I can fix the test as you give me a green light and I will do it tomorrow

wood1986 · 2023-05-13T20:07:06Z

Hey @vonovak, I am unable to run e2e test both locally and remotely with the same error https://app.circleci.com/pipelines/github/react-native-datetimepicker/datetimepicker/1220/workflows/e7c6c6c6-6cf9-4fa5-8b63-d9cba4df6e6f/jobs/3823/parallel-runs/0/steps/0-115

vonovak · 2023-05-15T11:41:01Z

ios/RNDateTimePickerManager.m

+      return;
+    }
+  }
+  RCTLogWarn(@"'%@' does not exist in NSTimeZone.knownTimeZoneNames fallback to localTimeZone=%@", json, NSTimeZone.localTimeZone.name);


this warning will currently log even for cases when it shouldn't

vonovak · 2023-05-15T11:43:37Z

ios/fabric/RNDateTimePickerComponentView.mm

+        if (timeZone != nil) {
+            picker.timeZone = timeZone;
+        } else {
+            RCTLogWarn(@"'%@' does not exist in NSTimeZone.knownTimeZoneNames fallback to localTimeZone=%@", timeZoneName, NSTimeZone.localTimeZone.name);


I believe this will log even when it shouldn't

vonovak · 2023-05-15T12:00:21Z

Hey @vonovak, I am unable to run e2e test both locally and remotely with the same error

see #758 - that should fix it

vonovak · 2023-06-20T20:19:45Z

@wood1986 if you integrate the latest master, you should be able to run the example app and e2e tests. Let me know if you need any help, thanks! 👍

wood1986 · 2023-07-08T07:06:18Z

@vonovak, I am still unable to run the e2e test. It shows

vonovak · 2023-07-08T09:17:43Z

There are two ways:

remove the utilBinaryPaths entry in detoxrc
run the tests on a "vanilla" emulator without Google services installed. You can create one from the command line the same way it's done in circle ci, see the circle ci config file, android/create-avd step.

You'll see the exact command if you look at some previous ci runs in circle ci. I'm away from keyboard so cannot give the exact command.

Thank you! 👍

wood1986 · 2023-07-09T19:02:14Z

Hey @vonovak, Now, I can run the test and I have fixed all the tests except tz related test cases. Thanks for that.

I want to completely remove all tzOffsetMinutes as I do not want to spend time on writing test for tzOffsetMinutes. What is your thought?

vonovak · 2023-07-09T20:45:10Z

Please keep the code for tzOffsetMinutes the way it is (don't remove it, don't add tests). We'll remove the code in a later release.

What would be appreciated though is that if someone uses the tzOffsetMinutes prop, they get a depreciation warning, asking them to use the new prop instead. Thank you! 👍

wood1986 · 2023-07-10T07:11:46Z

Now, I cannot run e2e test for iOS. It crashes on launch but I can launch it without detox

vonovak · 2023-07-10T08:35:17Z

@wood1986 in CI, the ios tests appear to run just fine, so if they are crashing for you, it seems it's something that is present with you locally.
Is your branch up to date with master branch? It shouldn't really matter but I recently updated detox to latest stable... Not sure really.

Also, not sure why, as the tests are passing on Android, but I see failures with label not matching expectation (ax.id = "utcTime"; text = "2000-01-01T14:44:00Z", expected: toHaveText('2021-11-13T14:44:00Z') and trying to scroll com.facebook.react.views.scroll.ReactHorizontalScrollView (such component won't exists on ios).

In case you end up running out of time / patience, let me know and I can take it over the finish line, most of the work is done afaict. Thank you! 👍

wood1986 · 2023-07-10T15:27:54Z

Thanks @vonovak

I have not updated the test for iOS yet. android is completed except one test case in new architecture.

My branch is up to date and it has all the latest change from master. Please run it on your side and let me know how I can run it locally. I want to update all the tests. I was using iOS 16 and iPhone 14 simulator

wood1986 · 2023-07-12T12:19:46Z

Hey @vonovak, Finally, I have just updated all the the tests. The test is a bit unstable. Only you can rerun the test. Please re-review everything as I did a force push.

By the way, I am still unable to run the test on iOS and I was using the build machine result to fix the tests

vonovak · 2023-08-02T10:56:43Z

hello @wood1986 thank you for your work, and sorry to keep you waiting, I'm going to take a look at this next week! 👍

vonovak · 2023-08-29T13:18:15Z

@wood1986 thank you for your patience and your PR, it'll be released soon! 🙂 💯

# [7.5.0](v7.4.2...v7.5.0) (2023-08-29) ### Features * add timeZoneName prop ([#744](#744)) ([d136216](d136216))

vonovak · 2023-08-29T13:33:55Z

🎉 This issue has been resolved in version 7.5.0 🎉

If this package helps you, consider sponsoring us! 🚀

wood1986 force-pushed the feature/timezone-name branch from 47b3b08 to 288867e Compare March 25, 2023 18:47

wood1986 force-pushed the feature/timezone-name branch 5 times, most recently from a0afd05 to 5a9d512 Compare March 27, 2023 07:29

vonovak requested changes May 12, 2023

View reviewed changes

vonovak reviewed May 15, 2023

View reviewed changes

wood1986 force-pushed the feature/timezone-name branch from 16f9819 to 421cca0 Compare July 10, 2023 07:09

wood1986 force-pushed the feature/timezone-name branch from 421cca0 to 67bfb92 Compare July 10, 2023 07:43

wood1986 force-pushed the feature/timezone-name branch 3 times, most recently from 1c96f27 to 1e019c6 Compare July 11, 2023 05:08

wood1986 force-pushed the feature/timezone-name branch 2 times, most recently from af8a5b5 to d0a0bb0 Compare July 11, 2023 17:29

feat: add timeZoneName props

25fa696

wood1986 force-pushed the feature/timezone-name branch from d0a0bb0 to 25fa696 Compare July 12, 2023 11:47

wood1986 requested a review from vonovak July 12, 2023 12:19

vonovak added 4 commits August 24, 2023 21:12

code review

89c7878

refactor: e2e review

d96d64d

refactor: fix flow

86ab6b4

docs: readme

ecbe753

vonovak force-pushed the feature/timezone-name branch from dfc7ad5 to ecbe753 Compare August 29, 2023 11:58

vonovak added 2 commits August 29, 2023 14:20

docs: readme

585ce15

fix: tz prop handling

8613969

vonovak approved these changes Aug 29, 2023

View reviewed changes

vonovak merged commit d136216 into react-native-datetimepicker:master Aug 29, 2023
4 checks passed

vonovak pushed a commit that referenced this pull request Aug 29, 2023

chore(release): 7.5.0 [skip ci]

4770080

# [7.5.0](v7.4.2...v7.5.0) (2023-08-29) ### Features * add timeZoneName prop ([#744](#744)) ([d136216](d136216))

vonovak added the released label Aug 29, 2023

wood1986 deleted the feature/timezone-name branch August 29, 2023 16:59

punndcoder28 mentioned this pull request Sep 15, 2023

Deps: @react-native-community/datetimepicker timezone issue mmazzarolo/react-native-modal-datetime-picker#721

Open

bolatovumar mentioned this pull request Nov 10, 2023

Android not support below 1970 years #835

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add timeZoneName props #744

feat: add timeZoneName props #744

wood1986 commented Mar 25, 2023 •

edited by vonovak

wood1986 commented Mar 25, 2023

wood1986 commented Mar 29, 2023

vonovak commented Apr 3, 2023

wood1986 commented Apr 14, 2023

wood1986 commented May 8, 2023 •

edited

vonovak left a comment

vonovak May 12, 2023

vonovak May 12, 2023

vonovak May 12, 2023

vonovak May 12, 2023

vonovak May 12, 2023

vonovak May 12, 2023

vonovak May 12, 2023

wood1986 May 13, 2023

vonovak May 12, 2023

vonovak May 12, 2023

wood1986 commented May 13, 2023

wood1986 commented May 13, 2023

vonovak May 15, 2023

vonovak May 15, 2023

vonovak commented May 15, 2023

vonovak commented Jun 20, 2023

wood1986 commented Jul 8, 2023

vonovak commented Jul 8, 2023

wood1986 commented Jul 9, 2023

vonovak commented Jul 9, 2023 •

edited

wood1986 commented Jul 10, 2023

vonovak commented Jul 10, 2023

wood1986 commented Jul 10, 2023 •

edited

wood1986 commented Jul 12, 2023

vonovak commented Aug 2, 2023

vonovak commented Aug 29, 2023 •

edited

vonovak commented Aug 29, 2023

feat: add timeZoneName props #744

feat: add timeZoneName props #744

Conversation

wood1986 commented Mar 25, 2023 • edited by vonovak

Summary

Test Plan

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

Checklist

wood1986 commented Mar 25, 2023

wood1986 commented Mar 29, 2023

vonovak commented Apr 3, 2023

wood1986 commented Apr 14, 2023

wood1986 commented May 8, 2023 • edited

vonovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wood1986 commented May 13, 2023

wood1986 commented May 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vonovak commented May 15, 2023

vonovak commented Jun 20, 2023

wood1986 commented Jul 8, 2023

vonovak commented Jul 8, 2023

wood1986 commented Jul 9, 2023

vonovak commented Jul 9, 2023 • edited

wood1986 commented Jul 10, 2023

vonovak commented Jul 10, 2023

wood1986 commented Jul 10, 2023 • edited

wood1986 commented Jul 12, 2023

vonovak commented Aug 2, 2023

vonovak commented Aug 29, 2023 • edited

vonovak commented Aug 29, 2023

wood1986 commented Mar 25, 2023 •

edited by vonovak

wood1986 commented May 8, 2023 •

edited

vonovak commented Jul 9, 2023 •

edited

wood1986 commented Jul 10, 2023 •

edited

vonovak commented Aug 29, 2023 •

edited