dayjs icon indicating copy to clipboard operation
dayjs copied to clipboard

fix: offset calculation of timezone plugin

Open Pareder opened this issue 2 years ago • 7 comments

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).

Pareder avatar Feb 05 '23 19:02 Pareder

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%> (ø)

... and 5 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 11 '23 06:02 codecov[bot]

not sure if this is a correct fix, is there a way to add some test cases to prove it right?

iamkun avatar Feb 11 '23 06:02 iamkun

@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?

Pareder avatar Feb 12 '23 16:02 Pareder

@iamkun Could you please review it?

Pareder avatar Sep 20 '23 12:09 Pareder

any update on this ?

KingAmo avatar Apr 07 '24 08:04 KingAmo

@iamkun Please merge this pull request

artemkhalygov avatar Jun 07 '24 11:06 artemkhalygov

Tried applying a patch but still does not work on iOS 😔

jgtorrejon avatar Jul 05 '24 23:07 jgtorrejon