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

tsOffset may fail depending on the Intl.DateTimeFormat implementation #23

Open
martin-trummer opened this issue Oct 16, 2024 · 12 comments

Comments

@martin-trummer
Copy link

tz/src/tzOffset/index.ts

Lines 18 to 23 in 2139037

export function tzOffset(timeZone: string | undefined, date: Date): number {
try {
const format = (offsetFormatCache[timeZone!] ||= new Intl.DateTimeFormat(
"en-GB",
{ timeZone, hour: "numeric", timeZoneName: "longOffset" }
).format);

The problem is that the format function is not bound to the class. So if the implementation for format() uses this, it will fail at runtime.

We can see the issue, when we use the Time Travel Chrom Extension which is very useful for testing

possible fix would be

function getBoundFormat(timeZone: string) {
  const dateTimeFormat = new Intl.DateTimeFormat("en-GB", {
    timeZone,
    hour: "numeric",
    timeZoneName: "longOffset"
  })
  return dateTimeFormat.format.bind(dateTimeFormat);
}

export function tzOffset(timeZone, date) {
  try {
    const format = offsetFormatCache[timeZone] ||= getBoundFormat(timeZone);
@Perdolique
Copy link

@kossnocorp which browsers are supported? I see that longOffset isn't supported in Safari below 15.4 and Chrome below 95, for example, but can't find a compatibility table in the documentation.

@csandman
Copy link

@Perdolique Thanks for the tip on longOffset, I'm currently dealing with some production bugs due to this, as I didn't realize it would return NaN for browsers before that. I'm currently looking into formatjs for a polyfill approach, but I'm not sure the best way to actually test on one of these older browsers.

@ryaa
Copy link

ryaa commented Feb 14, 2025

@Perdolique

I see that longOffset isn't supported in Safari below 15.4 and Chrome below 95, for example, but can't find a compatibility table in the documentation.

Yes, it seems to be a limitation in date-fns/tz package. The problem is that in tzOffset method the Intl.DateTimeFormat is constructed with options.timeZoneName set to longOffset (https://github.com/date-fns/tz/blob/main/src/tzOffset/index.ts#L22) which is supported in Safari 15.4+ (iOS 15.4 and above) - see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat#browser_compatibility or below Image

Image

@ryaa
Copy link

ryaa commented Feb 14, 2025

@csandman

I'm currently looking into formatjs for a polyfill approach, but I'm not sure the best way to actually test on one of these older browsers.

Have you managed to resolve this problem with older browsers with formatjs polyfills?
The alternative could be to make the change in https://github.com/date-fns/tz/blob/main/src/tzOffset/index.ts#L22 to feature-detect support for longOffset and use long in Safari 15.3 and below and longOffset in 15.4+

@ryaa
Copy link

ryaa commented Feb 18, 2025

I can confirm that formatjs polyfills does not fix the problem when formatting the date on iOS 15.4 or below. The error is RangeError: timeZoneName must be "short" or "long" and happens because timeZoneName: "longOffset"` which is not supported on iOS 15.4 or below - see https://github.com/date-fns/tz/blob/main/src/tzOffset/index.ts#L22

Please also see #38 and formatjs/formatjs#4804

@ryaa
Copy link

ryaa commented Feb 18, 2025

Until formatjs polyfills is fixed to correctly handle the above use case, I implemented a temporary fix for date-fns/tz package. The fix can be applied using patch-package and the attached file (copy the file to the appropriate director and run npm install to apply the patch)

@date-fns+tz+1.2.0.patch

@csandman
Copy link

csandman commented Feb 19, 2025

@ryaa Glad you found a workaround, sorry I meant to reply saying I didn't have any luck getting formatjs to do what I wanted either haha. I'll take a look into your patch! Do you have a snippet of the exact code you changed?

Also, does your patch actually allow dates to still be formatted in the specified timezone? My fallback approach was just to use a normal date instead, and users on older browsers would have to deal with it haha.

@ryaa
Copy link

ryaa commented Feb 20, 2025

@ryaa I'll take a look into your patch! Do you have a snippet of the exact code you changed?

This patch will update two files node_modules/@date-fns/tz/tzOffset/index.cjs and node_modules/@date-fns/tz/tzOffset/index.js. The resulting files are attached, so you should be able to easily compare to see the difference (just unzip them)

index.cjs.zip
index.js.zip

Also, does your patch actually allow dates to still be formatted in the specified timezone?

I believe yes. At least this is the requirement in my app and my patch seems to format the dates in any arbitrary timezone

@csandman
Copy link

I believe yes. At least this is the requirement in my app and my patch seems to format the dates in any arbitrary timezone

Nice, is there any reason not to PR this against the main project then? It seems like it would be generally helpful for people.

@ryaa
Copy link

ryaa commented Feb 22, 2025

Nice, is there any reason not to PR this against the main project then? It seems like it would be generally helpful for people.

The issue is with formatjs polyfills and the fix should be provided there, not in date-fns/tz

@csandman
Copy link

The issue is with formatjs polyfills and the fix should be provided there, not in date-fns/tz

I guess that depends on how you look at it, if this package could offer a fallback without requiring a polyfill, that could be a better option for some people.

ryaa added a commit to ryaa/tz that referenced this issue Feb 25, 2025
…entation

Changes:
- provided a potential fix to support iOS 15.4 and below (see date-fns#23 (comment))
@ryaa
Copy link

ryaa commented Feb 25, 2025

I guess that depends on how you look at it, if this package could offer a fallback without requiring a polyfill, that could be a better option for some people.

Here is the PR #45

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

No branches or pull requests

4 participants