-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix: offset calculation of timezone plugin #2227
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #2227 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 183 183
Lines 2249 2112 -137
Branches 636 555 -81
==========================================
- Hits 2249 2112 -137
☔ View full report in Codecov by Sentry. |
not sure if this is a correct fix, is there a way to add some test cases to prove it right? |
@iamkun None of timezone tests is broken, what test do you want me to add? Do you want to simulate |
src/plugin/timezone/index.js
Outdated
@@ -96,7 +96,8 @@ export default (o, c, d) => { | |||
const oldOffset = this.utcOffset() | |||
const date = this.toDate() | |||
const target = date.toLocaleString('en-US', { timeZone: timezone }) | |||
const diff = Math.round((date - new Date(target)) / 1000 / 60) | |||
const targetDate = d(target, 'M/D/YYYY H:mm:ss A') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use d(target) directlly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we can't use it, because parseDate
method returns new Date
(https://github.com/iamkun/dayjs/blob/dev/src/index.js#L79), which is the reason of this PR and in case of using customParseFormat
plugin without passing format
argument it calls original parseDate
method under the hood https://github.com/iamkun/dayjs/blob/dev/src/plugin/customParseFormat/index.js#L257. So my fix will work only when using customParseFormat
plugin, what do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, that's my concern. If we implement it in this way, the timezone plugin must rely on customParseFormat plugin to work that our users may get confused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got your point. Maybe I can find a better solution to calculate timezone offset not to depend on some APIs support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamkun Could you please check the new solution? I am using formatToParts
method of Intl.DateTimeFormat
instance and getting year, month, day, etc. from its result. What do you think? I tried this in my React Native environment and it works perfectly. All tests are passed correctly as well.
@iamkun Could you please review it? |
any update on this ? |
@iamkun Please merge this pull request |
Tried applying a patch but still does not work on iOS 😔 |
I didnt see this PR and wrote a similar one that solves the same problem using dayjs to parse the date string. Here is it. Please approve either one of these @iamkun. Pleasssssse! Would hugely appreciate it. We are having to use the patch-package trick to overwrite these lines in our codebase until its fixed. |
Update calculation of timezone offset. This change was caused by this issue #2207. The thing is that Hermes in React Native doesn't fully support Intl API (facebook/hermes#23). Let me explain.
This code:
works correct, it returns the date in
en-US
format, but after that when returned string is passed toDate
constructor it returnsDate { NaN }
:Date constructor cannot recognise this format, so calculation of difference is broken. And I think that we can use
dayjs
to do backward formatting.Note: in my opinion we can improve somehow the calculation of timezone offset not to depend on some APIs support (get rid of
Date.toLocaleString
).