dayjs
dayjs copied to clipboard
fix: offset calculation of timezone plugin
Update calculation of timezone offset. This change was caused by this issue https://github.com/iamkun/dayjs/issues/2207. The thing is that Hermes in React Native doesn't fully support Intl API (https://github.com/facebook/hermes/issues/23). Let me explain.
This code:
const target = date.toLocaleString('en-US', { timeZone: timezone })
works correct, it returns the date in en-US
format, but after that when returned string is passed to Date
constructor it returns Date { NaN }
:
const target = date.toLocaleString('en-US', { timeZone: timezone })
new Date(target) // Date { 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
).
Codecov Report
Patch coverage: 100.00%
and no project coverage change.
Comparison is base (
2254635
) 100.00% compared to head (4567b8d
) 100.00%.
:exclamation: Current head 4567b8d differs from pull request most recent head 5c7598a. Consider uploading reports for the commit 5c7598a to get more accurate results
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
Files Changed | Coverage Δ | |
---|---|---|
src/plugin/timezone/index.js | 100.00% <100.00%> (ø) |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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 Date { NaN }
behavior or what?
@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 😔