Skip to content

Conversation

@ohrite
Copy link

@ohrite ohrite commented May 31, 2018

Hi there!

We really love react-native-app-auth, and we came across a problem parsing timestamps in our application.

ISO8601 has a few different formats, but we found that the UTC-zoned format "yyyy-mm-ddThh:mm:ssZ" is parseable by the Javascript side of our app. Specifically, Date.parse() in JSCore seems to be unhappy with the non-Z style.

This PR switches to UTC as the timezone, and explicitly sets the zone to Z. We would love any feedback you've got to offer!

Thank you.

@kadikraman
Copy link
Contributor

Hi @ohrite - thanks for the PR!

I wonder what is the implication of hardcoding the US Locale?

Looking at the Java docs, new Locale("en"); creates a generic english-speaking locale which could be more suitable. I think there's also a plain "en" NSLocale in Objective C. So if we can't get the locale from the environment, could we use a generic "english" locale?

@ohrite
Copy link
Author

ohrite commented May 31, 2018

Thanks for the feedback!

On the iOS side, we’re locking the locale as per Apple recommendations: https://developer.apple.com/library/content/qa/qa1480/_index.html

To paraphrase that article, Apple made a locale that is guaranteed to not change, even if the device locale or regional date formatting standard changes. Their code sample at the end of that article seems quite close to the use case in RNAppAuth.

The general rationale behind locking the locale to en_US in Android was similar, since it’s available on all devices, and it’s similar to the iOS behavior. However, it doesn’t specify the POSIX variant. I am not sure if this is significant.

Thoughts?

@kadikraman
Copy link
Contributor

Ah yes, timezone hardly matters if it's not a date that's displayed to the user, good point!

I've just loaded up your PR on our example app and it's not giving me any issues, which is great! But I tried to reproduce your original issue and got a bit stuck. Both Date.parse() and new Date() seem to work just fine with the date displayed in our example app. Would you mind walking me through the problem you had parsing timestamps?

@jparr
Copy link

jparr commented May 31, 2018

I ran into this problem parsing timezone offsets with +hhmm. JSCore date parsing wants hh:mm. It made it past my first round of unit tests because the javascript engine my test were running in were different than the engine my RN app had. (Logging out the result of Date.parse in ios simulator resulted in undefined) Here is an example of some discussion about this problem.(https://stackoverflow.com/questions/34590369/formatting-a-date-string-in-react-native) We thought the best solution was to make it UTC.

@kadikraman
Copy link
Contributor

Gotcha, makes sense. Well it's backwards compatible and doesn't change the behaviour for anyone using moment or date-fns so LGTM 👍

@kadikraman kadikraman merged commit f2c2011 into FormidableLabs:master May 31, 2018
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

Successfully merging this pull request may close these issues.

3 participants