js-temporal / temporal-polyfill

Polyfill for Temporal (under construction)
ISC License
529 stars 28 forks source link

RangeError: Incorrect timeZone information provided #267

Closed lucasfronza closed 1 year ago

lucasfronza commented 1 year ago

Temporal.Now.plainDateISO(timezone) fails for non-canonical timezones, e.g. 'US/Pacific'

Error message is RangeError: Incorrect timeZone information provided.

I couldn't find if that is expected or not.

12wrigja commented 1 year ago

Can you provide a bit more information about the environment you ran that command in?

Running that function in the upstream spec polyfill (https://github.com/tc39/proposal-temporal/blob/3f82e8d48d3c062d07a4ae77dc175d65cc3be8ae/polyfill/package.json#L18) seems to indicate that works, and we have had reports recently that there are issues in newer browser builds that might cause this to fail (e.g. #264 ).

12wrigja commented 1 year ago

Running that command in Node v19.9 also works just fine for me.

lucasfronza commented 1 year ago

Thanks for the quick reply @12wrigja. Oh, indeed it works in Node. Just tested with v18.16. We are using Temporal on an Expo (React Native) app with Hermes.

12wrigja commented 1 year ago

This is probably because Hermes doesn't implement the Intl API surface:

https://hermesengine.dev/playground/

I ran

const a = Intl.DateTimeFormat('en-US').resolvedOptions().timeZone;

in that playground and got a ReferenceError:

Uncaught ReferenceError: Property 'Intl' doesn't exist
    at global (/tmp/hermes-input.js:1:11)

I don't think we will support such environments, given that the Intl API surface is standardized as the provider for ICU / Intl-related data (and shipping our own versions of this is something we definitely want to avoid given the code-size bloat).

It might be possible to get this working in Hermes if there was an Intl API polyfill.

12wrigja commented 1 year ago

A quick search indicates that the Intl API surface is a fairly common polyfill for React Native apps: https://stackoverflow.com/questions/41736735/react-native-and-intl-polyfill-required-on-android-device

lucasfronza commented 1 year ago

Interesting that it doesn't work in Hermes Playground, but Hermes does have support for Intl now, according to these:

https://github.com/facebook/hermes/blob/main/doc/Features.md#supported https://reactnative.dev/blog/2022/07/08/hermes-as-the-default#ios-intl https://hermesengine.dev/docs/intl/

And have been using Intl directly in our app for a while now.

But maybe there is indeed some part of the spec (https://hermesengine.dev/docs/intl/#not-yet-supported) that is needed for Temporal to work that is not working correctly in Hermes?

Can also confirm

const a = Intl.DateTimeFormat('en-US').resolvedOptions().timeZone;

does log Europe/London in our app.

Would you recommend raising this with Hermes instead?

12wrigja commented 1 year ago

Can you post the entire resolvedOptions value?

I don't see the polyfill source throwing an error that exactly matches that text, but we do throw a similar sounding error when timezone ID parsing fails: https://github.com/js-temporal/temporal-polyfill/blob/73ed89bdd9ecbc553972f030df3a1114d898c18a/lib/ecmascript.ts#L693 (RangeError: Invalid time zone).

Are you able to step through the operation with a debugger attached to find out what code causes the error to be thrown? That would help narrow down whether this is a problem in our code or in Hermes Intl / native platform support.

lucasfronza commented 1 year ago

Can you post the entire resolvedOptions value?

console.log(Intl.DateTimeFormat('en-US').resolvedOptions());
// => {"locale": "en-US", "timeZone": "Europe/London"}

Does this help? Screenshot 2023-10-16 at 17 01 11

But also, was able to find that the exact error message does come from Hermes: https://github.com/facebook/hermes/blob/main/lib/Platform/Intl/PlatformIntlApple.mm#L1431

12wrigja commented 1 year ago

This is probably caused by https://github.com/js-temporal/temporal-polyfill/blob/73ed89bdd9ecbc553972f030df3a1114d898c18a/lib/ecmascript.ts#L356-L366

new Intl.DateTimeFormat('en-us', {
      timeZone: String('Europe/London'),
      hour12: false,
      era: 'short',
      year: 'numeric',
      month: 'numeric',
      day: 'numeric',
      hour: 'numeric',
      minute: 'numeric',
      second: 'numeric'
    })

might also repro that error, and does seem to point to something missing/wrong with Hermes Intl support.

lucasfronza commented 1 year ago

Oh, quick test with Intl.DateTimeFormat('en-US', { timeZone: 'US/Pacific' }).format(new Date()) causes the crash. i.e. Definitely nothing to do with Temporal implementation, but just under the hood Intl one.

12wrigja commented 1 year ago

@lucasfronza is it OK to close this issue, as it doesn't appear to be a problem with this polyfill? Thanks.

lucasfronza commented 1 year ago

Yep! Thanks!