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

Add date shim to fix cross browser issues with parsing ISO-8601 dates #723

Merged
merged 1 commit into from Apr 2, 2013

Conversation

klaaspieter
Copy link
Contributor

It turns out that my fix for #538 doesn't correctly handle timezones in dates. This fixes that behavior by overriding Date.parse with a custom implementation taken from: https://github.com/csnover/js-iso8601

@wagenet
Copy link
Member

wagenet commented Feb 23, 2013

@klaaspieter It seems like we shouldn't modify the Date prototype without a flag to turn it off as we do in Ember core. Otherwise, this seems like a reasonable change.

@klaaspieter
Copy link
Contributor Author

@wagenet updated the PR to use Ember.Date.parse in stead of Date.parse. It will only extend the prototype if EXTEND_PROTOTYPES is true.

I also removed two tests that weren't strict ES5 compliant. Any thoughts on how lenient we should be when parsing dates?

@nragaz
Copy link
Contributor

nragaz commented Mar 8, 2013

@wagenet @klaaspieter Any updates on this? The current incorrect timezone handling is a real pain.

@klaaspieter
Copy link
Contributor Author

I believe I added all @wagenet's feedback. @wagenet?

// before falling back to any implementation-specific date parsing, so that’s what we do, even if native
// implementations could be faster
// 1 YYYY 2 MM 3 DD 4 HH 5 mm 6 ss 7 msec 8 Z 9 ± 10 tzHH 11 tzmm
if ((struct = /^(\d{4}|[+\-]\d{6})(?:-(\d{2})(?:-(\d{2}))?)?(?:T(\d{2}):(\d{2})(?::(\d{2})(?:\.(\d{3}))?)?(?:(Z)|([+\-])(\d{2})(?::(\d{2}))?)?)?$/.exec(date))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be nice to extract this regex as a closed over "constant"

@stefanpenner
Copy link
Member

Time parsing is a bitch, this will bring some extra sanity into the mix. I for one think this is good. @wagenet since you had the initial concerns i'll defer to you, but if u think its good lets pull it in.

@nragaz
Copy link
Contributor

nragaz commented Apr 2, 2013

@wagenet I believe this is waiting on your feedback. Would be much appreciated if you could take a look and facilitate getting this merged.

tomdale added a commit that referenced this pull request Apr 2, 2013
Add date shim to fix cross browser issues with parsing ISO-8601 dates
@tomdale tomdale merged commit 693f1ee into emberjs:master Apr 2, 2013
@nragaz
Copy link
Contributor

nragaz commented Apr 2, 2013

Thanks @tomdale! ❤️

@klaaspieter klaaspieter deleted the date-fix branch April 1, 2014 14:31
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.

None yet

5 participants